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

["Badole, Vishal" <vishal.badole@amd.com>]

On 4/24/2025 2:24 PM, Paolo Abeni wrote:
> 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
> 
> 
Thank you for your observations. I agree with your points. For the first 
case, I will remove the automatic update of NETIF_F_RXHASH based on the 
NETIF_F_RXCSUM value, as checksum offloading functions correctly without 
it, and I will include this change in the next patch version. For the 
second case, I will retain the automatic updates for header split and 
virtual network, as they depend on NETIF_F_RXCSUM for consistent 
configuration and there is no alternative method to modify their state.

Thanks,
Vishal