[PATCH net] net: mscc: ocelot: fix incorrect IFH SRC_PORT field in ocelot_ifh_set_basic()

[PATCH net] net: mscc: ocelot: fix incorrect IFH SRC_PORT field in ocelot_ifh_set_basic()

[Vladimir Oltean]

Packets injected by the CPU should have a SRC_PORT field equal to the
CPU port module index in the Analyzer block (ocelot->num_phys_ports).

The blamed commit copied the ocelot_ifh_set_basic() call incorrectly
from ocelot_xmit_common() in net/dsa/tag_ocelot.c. Instead of calling
with "x", it calls with BIT_ULL(x), but the field is not a port mask,
but rather a single port index.

[ side note: this is the technical debt of code duplication :( ]

The error used to be silent and doesn't appear to have other
user-visible manifestations, but with new changes in the packing
library, it now fails loudly as follows:

------------[ cut here ]------------
Cannot store 0x40 inside bits 46-43 - will truncate
sja1105 spi2.0: xmit timed out
WARNING: CPU: 1 PID: 102 at lib/packing.c:98 __pack+0x90/0x198
sja1105 spi2.0: timed out polling for tstamp
CPU: 1 UID: 0 PID: 102 Comm: felix_xmit
Tainted: G        W        N 6.13.0-rc1-00372-gf706b85d972d-dirty #2605
Call trace:
 __pack+0x90/0x198 (P)
 __pack+0x90/0x198 (L)
 packing+0x78/0x98
 ocelot_ifh_set_basic+0x260/0x368
 ocelot_port_inject_frame+0xa8/0x250
 felix_port_deferred_xmit+0x14c/0x258
 kthread_worker_fn+0x134/0x350
 kthread+0x114/0x138

The code path pertains to the ocelot switchdev driver and to the felix
secondary DSA tag protocol, ocelot-8021q. Here seen with ocelot-8021q.

The messenger (packing) is not really to blame, so fix the original
commit instead.

Fixes: e1b9e80236c5 ("net: mscc: ocelot: fix QoS class for injected packets with "ocelot-8021q"")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
I only have one board which uses the ocelot-8021q DSA tagging protocol
by default, and I don't test that as often as the other LS1028A boards,
and when I did, it wasn't with Jake's series applied.

 drivers/net/ethernet/mscc/ocelot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 3d72aa7b1305..ef93df520887 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1432,7 +1432,7 @@ void ocelot_ifh_set_basic(void *ifh, struct ocelot *ocelot, int port,
 
 	memset(ifh, 0, OCELOT_TAG_LEN);
 	ocelot_ifh_set_bypass(ifh, 1);
-	ocelot_ifh_set_src(ifh, BIT_ULL(ocelot->num_phys_ports));
+	ocelot_ifh_set_src(ifh, ocelot->num_phys_ports);
 	ocelot_ifh_set_dest(ifh, BIT_ULL(port));
 	ocelot_ifh_set_qos_class(ifh, qos_class);
 	ocelot_ifh_set_tag_type(ifh, tag_type);
-- 
2.43.0

Re: [PATCH net] net: mscc: ocelot: fix incorrect IFH SRC_PORT field in ocelot_ifh_set_basic()

[Simon Horman]

On Thu, Dec 12, 2024 at 06:55:45PM +0200, Vladimir Oltean wrote:
> Packets injected by the CPU should have a SRC_PORT field equal to the
> CPU port module index in the Analyzer block (ocelot->num_phys_ports).
> 
> The blamed commit copied the ocelot_ifh_set_basic() call incorrectly
> from ocelot_xmit_common() in net/dsa/tag_ocelot.c. Instead of calling
> with "x", it calls with BIT_ULL(x), but the field is not a port mask,
> but rather a single port index.
> 
> [ side note: this is the technical debt of code duplication :( ]
> 
> The error used to be silent and doesn't appear to have other
> user-visible manifestations, but with new changes in the packing
> library, it now fails loudly as follows:
> 
> ------------[ cut here ]------------
> Cannot store 0x40 inside bits 46-43 - will truncate
> sja1105 spi2.0: xmit timed out
> WARNING: CPU: 1 PID: 102 at lib/packing.c:98 __pack+0x90/0x198
> sja1105 spi2.0: timed out polling for tstamp
> CPU: 1 UID: 0 PID: 102 Comm: felix_xmit
> Tainted: G        W        N 6.13.0-rc1-00372-gf706b85d972d-dirty #2605
> Call trace:
>  __pack+0x90/0x198 (P)
>  __pack+0x90/0x198 (L)
>  packing+0x78/0x98
>  ocelot_ifh_set_basic+0x260/0x368
>  ocelot_port_inject_frame+0xa8/0x250
>  felix_port_deferred_xmit+0x14c/0x258
>  kthread_worker_fn+0x134/0x350
>  kthread+0x114/0x138
> 
> The code path pertains to the ocelot switchdev driver and to the felix
> secondary DSA tag protocol, ocelot-8021q. Here seen with ocelot-8021q.
> 
> The messenger (packing) is not really to blame, so fix the original
> commit instead.
> 
> Fixes: e1b9e80236c5 ("net: mscc: ocelot: fix QoS class for injected packets with "ocelot-8021q"")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] net: mscc: ocelot: fix incorrect IFH SRC_PORT field in ocelot_ifh_set_basic()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 12 Dec 2024 18:55:45 +0200 you wrote:
> Packets injected by the CPU should have a SRC_PORT field equal to the
> CPU port module index in the Analyzer block (ocelot->num_phys_ports).
> 
> The blamed commit copied the ocelot_ifh_set_basic() call incorrectly
> from ocelot_xmit_common() in net/dsa/tag_ocelot.c. Instead of calling
> with "x", it calls with BIT_ULL(x), but the field is not a port mask,
> but rather a single port index.
> 
> [...]

Here is the summary with links:
  - [net] net: mscc: ocelot: fix incorrect IFH SRC_PORT field in ocelot_ifh_set_basic()
    https://git.kernel.org/netdev/net/c/2d5df3a680ff

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html