Re: [PATCH net V2] amd-xgbe: Fix to ensure dependent features are toggled with RX checksum offload

[Paolo Abeni]

On 4/23/25 9:57 AM, Badole, Vishal wrote:
> On 4/23/2025 3:50 AM, Jacob Keller wrote:
>> On 4/21/2025 7:04 AM, Vishal Badole wrote:
>>> According to the XGMAC specification, enabling features such as Layer 3
>>> and Layer 4 Packet Filtering, Split Header, Receive Side Scaling (RSS),
>>> and Virtualized Network support automatically selects the IPC Full
>>> Checksum Offload Engine on the receive side.
>>>
>>> When RX checksum offload is disabled, these dependent features must also
>>> be disabled to prevent abnormal behavior caused by mismatched feature
>>> dependencies.
>>>
>>> Ensure that toggling RX checksum offload (disabling or enabling) properly
>>> disables or enables all dependent features, maintaining consistent and
>>> expected behavior in the network device.
>>>
>>
>> My understanding based on previous changes I've made to Intel drivers,
>> the netdev community opinion here is that the driver shouldn't
>> automatically change user configuration like this. Instead, it should
>> reject requests to disable a feature if that isn't possible due to the
>> other requirements.
>>
>> In this case, that means checking and rejecting disable of Rx checksum
>> offload whenever the features which depend on it are enabled, and reject
>> requests to enable the features when Rx checksum is disabled.
> 
> Thank you for sharing your perspective and experience with Intel 
> drivers. From my understanding, the fix_features() callback in ethtool 
> handles enabling and disabling the dependent features required for the 
> requested feature to function correctly. It also ensures that the 
> correct status is reflected in ethtool and notifies the user.
> 
> However, if the user wishes to enable or disable those dependent 
> features again, they can do so using the appropriate ethtool settings.

AFAICS there are two different things here:

- automatic update of NETIF_F_RXHASH according to NETIF_F_RXCSUM value:
that should be avoid and instead incompatible changes should be rejected
with a suitable error message.

- automatic update of header split and vxlan depending on NETIF_F_RXCSUM
value: that could be allowed as AFAICS the driver does not currently
offer any other method to flip modify configuration (and make the state
consistent).

Thanks,

Paolo