Message-ID: <20240927135318.duezjjl5l7xjfdla@skbuf>
Date: Fri, 27 Sep 2024 16:53:18 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Kacper Ludwinski <kac.ludwinski@...oud.com>
Cc: davem@...emloft.net, kuba@...nel.org, linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org, netdev@...r.kernel.org,
	pabeni@...hat.com, petrm@...dia.com, horms@...nel.org,
	edumazet@...gle.com, shuah@...nel.org
Subject: Re: [PATCH net v3] selftests: forwarding: no_forwarding: Fix issue
 related with assigning two different vids to the same interface.

Hi Kacper,

On Fri, Sep 27, 2024 at 08:28:24PM +0900, Kacper Ludwinski wrote:
> Fix typo.
> Currently, the second bridge command overwrites the first one.
> Fix this by adding this VID to the interface behind $swp2.
> 
> Fixes: 476a4f05d9b8 ("selftests: forwarding: add a no_forwarding.sh test")
> Signed-off-by: Kacper Ludwinski <kac.ludwinski@...oud.com>
> ---
>  tools/testing/selftests/net/forwarding/no_forwarding.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/no_forwarding.sh b/tools/testing/selftests/net/forwarding/no_forwarding.sh
> index 9e677aa64a06..694ece9ba3a7 100755
> --- a/tools/testing/selftests/net/forwarding/no_forwarding.sh
> +++ b/tools/testing/selftests/net/forwarding/no_forwarding.sh
> @@ -202,7 +202,7 @@ one_bridge_two_pvids()
>  	ip link set $swp2 master br0
>  
>  	bridge vlan add dev $swp1 vid 1 pvid untagged
> -	bridge vlan add dev $swp1 vid 2 pvid untagged
> +	bridge vlan add dev $swp2 vid 2 pvid untagged
>  
>  	run_test "Switch ports in VLAN-aware bridge with different PVIDs"
>  
> -- 
> 2.43.0
>

Thank you for the patch. More process problems though.

Please use the space under "---" to write a revision history for the
patches. I had to download your v2 and v3 to figure out that you changed
the Signed-off-by email address and fixed some whitespaces. Generally it
is the task of the patch submitter, not the reviewers, to record the
differences.

Annotating the revision history to links on lore.kernel.org of previous
patch submissions is also a sign of a well cared-for patch.

Secondly, Hangbin Liu left you a Reviewed-by: tag in a previous patch submission.
https://lore.kernel.org/netdev/ZvTDJyKatBVpgqMj@fedora/
When you resubmit, it is your duty to pick up such tags from previous
patch versions and keep them in your commit message. Some say to keep
your Signed-off-by: tag as the final line though. No empty lines between
tags (Fixes:, Reviewed-by:, Signed-off-by: etc).

Thirdly, the commit title is unnecessarily long. Please limit it to
around 80 characters, and only exceed if you cannot abbreviate anymore.
For example:

selftests: net: no_forwarding: fix VID for swp2 in one_bridge_two_pvids() test

Fourth, bug fixes as per the Documentation/process/stable-kernel-rules.rst
process must be real bugs with a visible impact. So your commit message
has to make it clear what that impact is, to pass the 'net' vs 'net-next'
triage. You can't leave that part for the reader to figure it out.

For example, I suggest this for the commit message body:

The one_bridge_two_pvids() test intends to check that there is no
leakage of traffic between bridge ports which have a single VLAN - the
PVID VLAN.

Because of a typo, port $swp1 is configured with a PVID twice (second
command overwrites first), and $swp2 isn't configured at all (and since
the bridge vlan_default_pvid property is set to 0, this port will not
have a PVID at all, so it will drop all untagged and priority-tagged
traffic).

So, instead of testing the configuration that was intended, we are
testing a different one, where one port has PVID 2 and the other has
no PVID. This incorrect version of the test should also pass, but is
ineffective for its purpose, so fix the typo.

This test was run on $<insert platform here> and this is the result:
<insert kselftest output here>.

Please resubmit with these changes.

Thanks.

pw-bot: changes-requested

Powered by blists - more mailing lists