[Intel-wired-lan] [PATCH iwl-net] i40e: Fix handling changed priv flags

[Intel-wired-lan] [PATCH iwl-net] i40e: Fix handling changed priv flags

[pegro]

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 corrent 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, source pruning requires a reset of the PF,
which was skipped due to this bug.

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;
-- 
2.34.1

Re: [Intel-wired-lan] [PATCH iwl-net] i40e: Fix handling changed priv flags

[Paul Menzel]

[Cc: +Przemek who succeeded Jesse]

Dear Peter,


Thank you very much for your patch. Some minor comments.

Am 30.10.24 um 17:06 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.

It’d be great if you reflowed for 75 characters per line.

> Fix it by using the corrent bitmaps.

corre*c*t

> 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, source pruning requires a reset of the PF,
> which was skipped due to this bug.

If you have the actual commands handy to reproduce it, that’d be great 
to have in the commit message.

> 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;

With the style fixes above:

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH iwl-net] i40e: Fix handling changed priv flags

[Paul Menzel]

[Cc: -Jesse 550 #5.1.0 Address rejected.]

Am 30.10.24 um 17:34 schrieb Paul Menzel:
> [Cc: +Przemek who succeeded Jesse]
> 
> Dear Peter,
> 
> 
> Thank you very much for your patch. Some minor comments.
> 
> Am 30.10.24 um 17:06 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.
> 
> It’d be great if you reflowed for 75 characters per line.
> 
>> Fix it by using the corrent bitmaps.
> 
> corre*c*t
> 
>> 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, source pruning requires a reset of the PF,
>> which was skipped due to this bug.
> 
> If you have the actual commands handy to reproduce it, that’d be great 
> to have in the commit message.
> 
>> 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;
> 
> With the style fixes above:
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul

[Intel-wired-lan] [PATCH iwl-net v2] i40e: Fix handling changed priv flags

[pegro]

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.

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;
-- 
2.34.1

Re: [Intel-wired-lan] [PATCH iwl-net v2] i40e: Fix handling changed priv flags

[Paul Menzel]

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

Re: [Intel-wired-lan] [PATCH iwl-net v2] i40e: Fix handling changed priv flags

[Przemek Kitszel]

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

Re: [Intel-wired-lan] [PATCH iwl-net v2] i40e: Fix handling changed priv flags

[Peter Große]

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

Re: [Intel-wired-lan] [PATCH iwl-net v2] i40e: Fix handling changed priv flags

[Przemek Kitszel]

On 10/31/24 17:11, Peter Große wrote:
> 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.

I believe so. Especially that you have a Fixes: tag, it's easy to tell
if this patch should be backported to given stable tree.
This happens outside of the Intel, after the patch will be eventually
applied to net tree.

There is an option to CC stable directly, or even point out to what
kernels your patch should be backported to. But in easy cases, I like to
keep it "automatic", thanks to Sasha Levin and Greg KH.

Than OS Vendors will get it down from there.

> 
> 
>> 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?

for IWL patches: Intel VAL will test it out (will reach to you in case
of any issues), then Tony will bundle it together with other fixes to
send to net (process is the same for next-, with minor details)

so you could just wait for this fix to be backported to stable

> 
> Kind regards
> Peter
> 

Re: [Intel-wired-lan] [PATCH iwl-net v2] i40e: Fix handling changed priv flags

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of pegro@friiks.de
> Sent: 30 October 2024 22:52
> To: intel-wired-lan@lists.osuosl.org; Paul Menzel <pmenzel@molgen.mpg.de>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: netdev@vger.kernel.org; Peter Große <pegro@friiks.de>
> Subject: [Intel-wired-lan] [PATCH iwl-net v2] i40e: Fix handling changed priv flags
>
> 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.
>
> 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(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)