Re: [PATCH net-next v9 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Source PGID mask drops all data-plane traffic to and from the CPU.
- [Low] Trailing backslash on the last line of a Makefile variable assignment.
- [Low] Redundant register write to QS_INJ_CTRL_GAP_SIZE.
--

--- Patch [4]: [PATCH net-next v9 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X ---
commit 3edb62282650bb2eeb57a08ce4dcd00f00d76a54
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>

net: dsa: lan9645x: add basic dsa driver for LAN9645X

Add the LAN9645X basic DSA driver with initialization, parent regmap
requests, port module initialization for NPI, CPU ports and front ports,
and phylink integration for MAC side configuration.

> diff --git a/drivers/net/dsa/microchip/lan9645x/Makefile b/drivers/net/dsa/microchip/lan9645x/Makefile
> new file mode 100644
> index 0000000000000..7cc0ae0ada402
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_NET_DSA_MICROCHIP_LAN9645X) += mchp-lan9645x.o
> +
> +mchp-lan9645x-objs := \
> +	lan9645x_main.o \
> +	lan9645x_npi.o \
> +	lan9645x_phylink.o \
> +	lan9645x_port.o \

[Severity: Low]
This isn't a bug, but should the trailing backslash on the last line be
removed? Leaving it could cause the build system to escape the EOF or
absorb appended lines into the variable in future patches.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> new file mode 100644
> index 0000000000000..b253813518c20
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c

[ ... ]

> @@ -0,0 +1,447 @@
> +static int lan9645x_setup(struct dsa_switch *ds)
> +{

[ ... ]

> +	/* Allow VLAN table to control whether cpu copy from the pgid table is
> +	 * enabled. Index PGID_ENTRIES is CPU src pgid, so we skip it as the
> +	 * configuration makes little sense here.
> +	 */
> +	for (int i = 0; i < PGID_ENTRIES; ++i)
> +		lan_wr(ANA_PGID_CFG_OBEY_VLAN_SET(1),
> +		       lan9645x, ANA_PGID_CFG(i));
> +
> +	/* Disable bridging by default */
> +	for (int p = 0; p < lan9645x->num_phys_ports; p++) {
> +		lan_wr(0, lan9645x, ANA_PGID(PGID_SRC + p));

[Severity: High]
Will this source PGID mask drop all data-plane traffic to and from the CPU?

By setting this to 0 for all ports in lan9645x_setup() and later in
lan9645x_update_fwd_mask(), it appears the user ports lack the CPU port
module bit.

Additionally, because the NPI port isn't part of any bridge, won't its mask
also be set to 0, causing frames injected by the CPU to be dropped by source
filtering before reaching the user ports?

> +
> +		/* Do not forward BPDU frames to the front ports and copy them
> +		 * to CPU
> +		 */
> +		lan_wr(ANA_CPU_FWD_BPDU_CFG_BPDU_REDIR_ENA,
> +		       lan9645x, ANA_CPU_FWD_BPDU_CFG(p));
> +	}

[ ... ]

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> new file mode 100644
> index 0000000000000..0642b59419c25
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c

[ ... ]

> @@ -0,0 +1,214 @@
> +void lan9645x_port_cpu_init(struct lan9645x *lan9645x)
> +{
> +	/* Map the 8 CPU extraction queues to the CPU port module (datasheet is
> +	 * wrong)
> +	 */
> +	lan_wr(0, lan9645x, QSYS_CPU_GROUP_MAP);
> +
> +	/* Set min-spacing of EOF to SOF on injected frames to 0, on cpu device
> +	 * 1. This is required when injecting with IFH.
> +	 * Default values emulates delay of std preamble/IFG setting on a front
> +	 * port.
> +	 */
> +	lan_rmw(QS_INJ_CTRL_GAP_SIZE_SET(0),
> +		QS_INJ_CTRL_GAP_SIZE,
> +		lan9645x, QS_INJ_CTRL(1));
> +
> +	/* Injection: Mode: manual injection | Byte_swap */
> +	lan_wr(QS_INJ_GRP_CFG_MODE_SET(1) |
> +	       QS_INJ_GRP_CFG_BYTE_SWAP_SET(1),
> +	       lan9645x, QS_INJ_GRP_CFG(1));
> +
> +	lan_rmw(QS_INJ_CTRL_GAP_SIZE_SET(0),
> +		QS_INJ_CTRL_GAP_SIZE,
> +		lan9645x, QS_INJ_CTRL(1));

[Severity: Low]
This isn't a bug, but is this second write to QS_INJ_CTRL_GAP_SIZE
intentional? It looks like it writes the exact same value as the register
modification a few lines above.

> +
> +	lan_wr(SYS_PORT_MODE_INCL_INJ_HDR_SET(1),
> +	       lan9645x, SYS_PORT_MODE(lan9645x->num_phys_ports + 1));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-dsa_lan9645x_switch_driver_base-v9-0-0d1512a326d7@microchip.com?part=4