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

[Badole, Vishal]

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