| Message ID | 20241030172224.30548-1-pegro@friiks.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net,v2] i40e: Fix handling changed priv flags | expand |
Dear Peter, Am 30.10.24 um 18:22 schrieb pegro@friiks.de: > From: Peter Große <pegro@friiks.de> > > After assembling the new private flags on a PF, the operation to determine > the changed flags uses the wrong bitmaps. Instead of xor-ing orig_flags > with new_flags, it uses the still unchanged pf->flags, thus changed_flags > is always 0. > > Fix it by using the correct bitmaps. > > The issue was discovered while debugging why disabling source pruning > stopped working with release 6.7. Although the new flags will be copied to > pf->flags later on in that function, disabling source pruning requires > a reset of the PF, which was skipped due to this bug. > > Disabling source pruning: > $ sudo ethtool --set-priv-flags eno1 disable-source-pruning on > $ sudo ethtool --show-priv-flags eno1 > Private flags for eno1: > MFP : off > total-port-shutdown : off > LinkPolling : off > flow-director-atr : on > veb-stats : off > hw-atr-eviction : off > link-down-on-close : off > legacy-rx : off > disable-source-pruning: on > disable-fw-lldp : off > rs-fec : off > base-r-fec : off > vf-vlan-pruning : off > > Regarding reproducing: > > I observed the issue with a rather complicated lab setup, where > * two VLAN interfaces are created on eno1 > * each with a different MAC address assigned > * each moved into a separate namespace > * both VLANs are bridged externally, so they form a single layer 2 network > > The external bridge is done via a channel emulator adding packet loss and > delay and the application in the namespaces tries to send/receive traffic > and measure the performance. Sender and receiver are separated by > namespaces, yet the network card "sees its own traffic" send back to it. > To make that work, source pruning has to be disabled. Thank you for taking the time to write this up. > Fixes: 70756d0a4727 ("i40e: Use DECLARE_BITMAP for flags and hw_features fields in i40e_pf") > Signed-off-by: Peter Große <pegro@friiks.de> > --- > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > index c841779713f6..016c0ae6b36f 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > @@ -5306,7 +5306,7 @@ static int i40e_set_priv_flags(struct net_device *dev, u32 flags) > } > > flags_complete: > - bitmap_xor(changed_flags, pf->flags, orig_flags, I40E_PF_FLAGS_NBITS); > + bitmap_xor(changed_flags, new_flags, orig_flags, I40E_PF_FLAGS_NBITS); > > if (test_bit(I40E_FLAG_FW_LLDP_DIS, changed_flags)) > reset_needed = I40E_PF_RESET_AND_REBUILD_FLAG; Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Kind regards, Paul
On 10/30/24 18:35, Paul Menzel wrote: > Dear Peter, > > > Am 30.10.24 um 18:22 schrieb pegro@friiks.de: >> From: Peter Große <pegro@friiks.de> >> >> After assembling the new private flags on a PF, the operation to >> determine >> the changed flags uses the wrong bitmaps. Instead of xor-ing orig_flags >> with new_flags, it uses the still unchanged pf->flags, thus changed_flags >> is always 0. >> >> Fix it by using the correct bitmaps. >> >> The issue was discovered while debugging why disabling source pruning >> stopped working with release 6.7. Although the new flags will be >> copied to >> pf->flags later on in that function, disabling source pruning requires >> a reset of the PF, which was skipped due to this bug. >> >> Disabling source pruning: >> $ sudo ethtool --set-priv-flags eno1 disable-source-pruning on >> $ sudo ethtool --show-priv-flags eno1 >> Private flags for eno1: >> MFP : off >> total-port-shutdown : off >> LinkPolling : off >> flow-director-atr : on >> veb-stats : off >> hw-atr-eviction : off >> link-down-on-close : off >> legacy-rx : off >> disable-source-pruning: on >> disable-fw-lldp : off >> rs-fec : off >> base-r-fec : off >> vf-vlan-pruning : off >> >> Regarding reproducing: >> >> I observed the issue with a rather complicated lab setup, where >> * two VLAN interfaces are created on eno1 >> * each with a different MAC address assigned >> * each moved into a separate namespace >> * both VLANs are bridged externally, so they form a single layer 2 >> network >> >> The external bridge is done via a channel emulator adding packet loss and >> delay and the application in the namespaces tries to send/receive traffic >> and measure the performance. Sender and receiver are separated by >> namespaces, yet the network card "sees its own traffic" send back to it. >> To make that work, source pruning has to be disabled. > > Thank you for taking the time to write this up. > >> Fixes: 70756d0a4727 ("i40e: Use DECLARE_BITMAP for flags and >> hw_features fields in i40e_pf") >> Signed-off-by: Peter Große <pegro@friiks.de> Both the code change and the Fixes: tag are correct, thank you! Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> BTW, we obey netdev rules on IWL ML - next revision only after 24-48h and send as standalone series (instead of In-reply-to) - no need to repost this time of course >> --- >> drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/ >> net/ethernet/intel/i40e/i40e_ethtool.c >> index c841779713f6..016c0ae6b36f 100644 >> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c >> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c >> @@ -5306,7 +5306,7 @@ static int i40e_set_priv_flags(struct net_device >> *dev, u32 flags) >> } >> flags_complete: >> - bitmap_xor(changed_flags, pf->flags, orig_flags, >> I40E_PF_FLAGS_NBITS); >> + bitmap_xor(changed_flags, new_flags, orig_flags, >> I40E_PF_FLAGS_NBITS); >> if (test_bit(I40E_FLAG_FW_LLDP_DIS, changed_flags)) >> reset_needed = I40E_PF_RESET_AND_REBUILD_FLAG; > > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> > > > Kind regards, > > Paul
Am Thu, 31 Oct 2024 08:34:36 +0100 schrieb Przemek Kitszel <przemyslaw.kitszel@intel.com>: > >> Fixes: 70756d0a4727 ("i40e: Use DECLARE_BITMAP for flags and > >> hw_features fields in i40e_pf") > >> Signed-off-by: Peter Große <pegro@friiks.de> > > Both the code change and the Fixes: tag are correct, thank you! > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Great to hear :) Would this be material for stable? I hope to avoid building my own kernel and just use a hwe kernel of Ubuntu. > BTW, we obey netdev rules on IWL ML - next revision only after 24-48h > and send as standalone series (instead of In-reply-to) - no need to > repost this time of course Sorry, I'm new to submitting patches here. Is there anything else I need to do? Kind regards Peter
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c index c841779713f6..016c0ae6b36f 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c @@ -5306,7 +5306,7 @@ static int i40e_set_priv_flags(struct net_device *dev, u32 flags) } flags_complete: - bitmap_xor(changed_flags, pf->flags, orig_flags, I40E_PF_FLAGS_NBITS); + bitmap_xor(changed_flags, new_flags, orig_flags, I40E_PF_FLAGS_NBITS); if (test_bit(I40E_FLAG_FW_LLDP_DIS, changed_flags)) reset_needed = I40E_PF_RESET_AND_REBUILD_FLAG;