Re: [PATCH net-next v1 1/4] net-e1000e: fix ethtool set_featurestaking new features into account too late

[David Decotigny]

Hello,

2011/11/21 MichaÅ MirosÅaw <mirqus@xxxxxxxxx>:
>> Âstatic int e1000_set_features(struct net_device *netdev,
>> - Â Â Â netdev_features_t features)
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â netdev_features_t features)
>> Â{
>> Â Â Â Âstruct e1000_adapter *adapter = netdev_priv(netdev);
>> Â Â Â Ânetdev_features_t changed = features ^ netdev->features;
>> + Â Â Â int retval = 1; Â/* telling netdev that we are updating
>> + Â Â Â Â Â Â Â Â Â Â Â Â * netdev->features by ourselves */
>> +
>> + Â Â Â netdev->features = features;
>>
>> Â Â Â Âif (changed & (NETIF_F_TSO | NETIF_F_TSO6))
>> Â Â Â Â Â Â Â Âadapter->flags |= FLAG_TSO_FORCE;
>>
>> Â Â Â Âif (!(changed & (NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_TX |
>> Â Â Â Â Â Â Â Â Â Â Â Â NETIF_F_RXCSUM)))
>> - Â Â Â Â Â Â Â return 0;
>> + Â Â Â Â Â Â Â return retval;
>>
>
> Would be less code if you set netdev->features here...
>
>> Â Â Â Âif (netif_running(netdev))
>> Â Â Â Â Â Â Â Âe1000e_reinit_locked(adapter);
>> Â Â Â Âelse
>> Â Â Â Â Â Â Â Âe1000e_reset(adapter);
>>
>> - Â Â Â return 0;
>> + Â Â Â return retval;
>
> ... and return 1 here, noting in a comment that e1000e_reinit_locked()
> might have changed netdev->features.

This would work, although I preferred the systematic approach for code
management reasons. But I will follow your recommendations. Waiting a
little (review of other patches) before sending the updated version.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/