Re: [Intel-wired-lan] [PATCH iwl-next v1] ice: Add E830 checksum support

["Greenwalt, Paul" <paul.greenwalt@intel.com>]

On 9/3/2024 8:29 PM, Greenwalt, Paul wrote:

>>>  
>>> @@ -6237,6 +6243,7 @@ ice_fix_features(struct net_device *netdev, netdev_features_t features)
>>>  	struct ice_netdev_priv *np = netdev_priv(netdev);
>>>  	netdev_features_t req_vlan_fltr, cur_vlan_fltr;
>>>  	bool cur_ctag, cur_stag, req_ctag, req_stag;
>>> +	netdev_features_t changed;
>>>  
>>>  	cur_vlan_fltr = netdev->features & NETIF_VLAN_FILTERING_FEATURES;
>>>  	cur_ctag = cur_vlan_fltr & NETIF_F_HW_VLAN_CTAG_FILTER;
>>> @@ -6285,6 +6292,29 @@ ice_fix_features(struct net_device *netdev, netdev_features_t features)
>>>  		features &= ~NETIF_VLAN_STRIPPING_FEATURES;
>>>  	}
>>>  
>>> +	if (!ice_is_feature_supported(np->vsi->back, ICE_F_GCS) ||
>>> +	    !(features & NETIF_F_HW_CSUM))
>>> +		return features;
>>> +
>>> +	changed = netdev->features ^ features;
>>> +
>>> +	/* HW checksum feature is supported and set, so enforce mutual
>>> +	 * exclusivity of TSO and GCS.
>>> +	 */
>>> +	if (features & NETIF_F_ALL_TSO) {
>>
>> 	if (!(features & ALL_TSO))
>> 		return features;
>>
>> to reduce indent level and avoid huge `if` blocks.
>>

Hi Olek and Tony, you both had feedback related to the change to
ice_fix_features().

Olek: to reduce indent level and avoid huge `if` blocks.

Tony: This prevents adding any other feature checks below this in the
future.
Seems like we should rework this into the feature being checked so that
we don't have this restriction.

Would using two helper functions for fixing DVM, and another for GCS and
TSO address your feedback? By adding feature checks are the top of the
helper functions and returning early, this could reduce the indent level
and avoid the huge 'if' block, while removing the restriction of adding
feature checks below these in the future.

Thanks,
Paul

>>> +		if (changed & NETIF_F_HW_CSUM && changed & NETIF_F_ALL_TSO) {
>>> +			netdev_warn(netdev, "Dropping TSO and HW checksum. TSO and HW checksum are mutually exclusive.\n");
>>> +			features &= ~NETIF_F_HW_CSUM;
>>> +			features &= ~((features & changed) & NETIF_F_ALL_TSO);