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

["Jens Emil Schulz Østergaard" <jensemil.schulzostergaard@microchip.com>]

On Tue Nov 10, 2026 at 12:28 PM CET, Paolo Abeni wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> --- Patch [4]: [PATCH net-next v16 4/4] net: dsa: lan9645x: add basic dsa driver for LAN9645X ---
> commit 2bc50b43f8638df7f8c7d09bfad9a789fecdd370
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
>
> net: dsa: lan9645x: add basic dsa driver for LAN9645X
>
> This adds a basic DSA driver for the LAN9645x switch. It covers switch
> init, named regmap requests from a parent device, NPI, CPU and front
> port setup, and phylink MAC configuration. Only the internal CuPHYs on
> ports 0-4 and RGMII on ports 7-8 are wired up, and the parent MFD
> driver that provides the regmaps is left for a later series.
>
>> 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..f91e825343353
>> --- /dev/null
>> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
>> @@ -0,0 +1,453 @@
>
> [ ... ]
>
>> +static int lan9645x_tag_npi_setup(struct dsa_switch *ds)
>> +{
>> +	struct dsa_port *dp, *first_cpu_dp = NULL;
>> +	struct lan9645x *lan9645x = ds->priv;
>> +	struct phylink_config c = {};
>> +
>> +	dsa_switch_for_each_user_port(dp, ds) {
>> +		if (dp->cpu_dp->ds != ds) {
>> +			dev_err(ds->dev,
>> +				"NPI port on a remote switch is not supported\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (first_cpu_dp && dp->cpu_dp != first_cpu_dp) {
>> +			dev_err(ds->dev, "Multiple NPI ports not supported\n");
>> +			return -EINVAL;
>> +		}
>
> [Severity: Medium]
> [Finding: main-1-1]
> [Sources: opus-5-5]
> Can the "Multiple NPI ports not supported" branch in
> lan9645x_tag_npi_setup() ever be taken?
>
> dsa_tree_setup_cpu_ports() runs before .setup. This driver has no
> preferred_default_local_cpu_port(), so that function gives every local
> user port the first local CPU port:
>
> net/dsa/dsa.c:dsa_tree_setup_cpu_ports() {
>     ...
> 		dsa_switch_for_each_port(dp, cpu_dp->ds) {
> 			/* Prefer the first local CPU port found */
> 			if (dp->cpu_dp)
> 				continue;
>
> 			if (dsa_port_is_user(dp) || dsa_port_is_dsa(dp))
> 				dp->cpu_dp = cpu_dp;
> 		}
>     ...
> }
>
> That makes dp->cpu_dp != first_cpu_dp unreachable. A second CPU port in
> DT is accepted without any message, and only the first CPU port goes
> through the phy_interface_empty() check.
>
> If the second CPU port is port 5 or 6, probe would then fail later in
> phylink_create(), and the driver's "Port %d has no usable interface
> mode" message would not be printed. The commit message says:
>
>   "Declared as a CPU port there is no such fallback, and the probe
>   fails, and our driver logs an error including the port number."
>
> If the second CPU port is port 7 or 8, DSA sets it up as a CPU port and
> attaches its conduit, but lan9645x_npi_port_init() is never called for
> it.
>
> Would it work better to count ports with dsa_switch_for_each_cpu_port()
> and reject anything above one?
>

Correct, the check cannot trigger here.

This driver has no tagging protocol change and no port_change_conduit,
so at setup every user port has the first local CPU port, and a second
CPU port in DT is accepted silently.

The next version counts the local CPU ports with
dsa_switch_for_each_cpu_port() and rejects anything but exactly one.

>> +
>> +		first_cpu_dp = dp->cpu_dp;
>> +	}
>
> [ ... ]
>
>> +static int lan9645x_reset_switch(struct lan9645x *lan9645x)
>> +{
>> +	u32 val = 0;
>> +	int err;
>> +
>> +	/* The parent must reset the whole chip before instantiating its
>> +	 * children, so the switch core is disabled when we get here. The
>> +	 * sequence below must not run more than once: initializing the
>> +	 * memories on a running core corrupts pointers inside the queue
>> +	 * system, which eventually fills up and stops passing traffic. If the
>> +	 * core is already enabled the chip was not reset, so skip it.
>> +	 */
>> +	if (SYS_RESET_CFG_CORE_ENA_GET(lan_rd(lan9645x, SYS_RESET_CFG)))
>> +		return 0;
>
> [Severity: Medium]
> [Finding: fable-5-1-7-9]
> [Sources: fable-5-1, opus-5-5]
> What happens here if the register read fails? lan_rd() discards the
> regmap_read() error and returns 0:
>
> drivers/net/dsa/microchip/lan9645x/lan9645x_main.h:lan_rd() {
> 	u32 addr, val = 0;
>     ...
> 	WARN_ON_ONCE(regmap_read(lan_tgt2rmap(lan9645x, t, tinst), addr, &val));
>
> 	return val;
> }
>
> If the read over the parent regmap fails (the intended parent is an SPI
> bridge like ocelot-spi), the result looks the same as CORE_ENA == 0. The
> RAM_INIT sequence would then run on a core that may already be enabled,
> which the comment above says corrupts the queue system.
>
> lan9645x_set_feat_dis() uses the same pattern:
>
> 	feat_dis = lan_rd(lan9645x, GCB_FEAT_DISABLE);
>
> A failed read there gives num_port_dis = 0, so the SKU port count check
> in lan9645x_setup() is skipped without any error.
>
> Should these two reads check the regmap_read() return value and fail
> the probe?
>

Dismissed, intentional.

SPI itself has no ack or checksum, so an unresponsive device or a broken
bus can complete the transfer successfully with all zeros or all ones,
and checking the return value of these reads would not catch it.

The parent verifies the register interface when it initializes it, by
reading back the interface configuration, so a device that does not
respond fails the parent probe before the switch is instantiated.

What remains is a transfer error reported by the SPI controller itself,
a host side failure, which lan_rd() reports with WARN_ON_ONCE().

>> +
>> +	lan_wr(SYS_RESET_CFG_CORE_ENA_SET(0), lan9645x, SYS_RESET_CFG);
>> +	lan_wr(SYS_RAM_INIT_RAM_INIT_SET(1), lan9645x, SYS_RAM_INIT);
>
> [ ... ]
>
>> +	/* Setup flooding PGIDs for IPv4/IPv6 multicast. Control and dataplane
>> +	 * use the same masks. Control frames are redirected to CPU, and
>> +	 * the network stack is responsible for forwarding these.
>> +	 * The dataplane is forwarding according to the offloaded MDB entries.
>> +	 */
>> +	lan_wr(ANA_FLOODING_IPMC_FLD_MC4_DATA_SET(PGID_MCIPV4) |
>> +	       ANA_FLOODING_IPMC_FLD_MC4_CTRL_SET(PGID_MC) |
>> +	       ANA_FLOODING_IPMC_FLD_MC6_DATA_SET(PGID_MCIPV6) |
>> +	       ANA_FLOODING_IPMC_FLD_MC6_CTRL_SET(PGID_MC),
>> +	       lan9645x, ANA_FLOODING_IPMC);
>
> [Severity: Low]
> [Finding: fable-5-1-2-5]
> [Sources: fable-5-1]
> This isn't a bug, but this comment in lan9645x_setup() doesn't seem to
> match what the patch does.
>
> lan9645x_switch_ops has no .port_mdb_add or .port_mdb_del. Nothing in
> this patch redirects IGMP/MLD control frames to the CPU either.
>
> The comment also says "Control and dataplane use the same masks", but
> control uses PGID_MC and data uses PGID_MCIPV4 and PGID_MCIPV6. These
> are three separate PGIDs. They only end up with the same value because
> of later writes in lan9645x_setup().
>
> Is this comment left over from the bridge and MDB patches that were
> dropped in v16?
>
> [ ... ]
>
>> +static int lan9645x_request_target_regmaps(struct lan9645x *lan9645x)
>> +{
>> +	const char *resource_name;
>> +	struct regmap *tgt_map;
>> +
>> +	for (int i = 0; i < NUM_TARGETS; i++) {
>> +		resource_name = lan9645x_resource_names[i];
>> +		if (!resource_name)
>> +			continue;
>> +
>> +		tgt_map = dev_get_regmap(lan9645x->dev->parent, resource_name);
>> +		if (IS_ERR_OR_NULL(tgt_map)) {
>> +			dev_err(lan9645x->dev, "Failed to get regmap=%s\n",
>> +				resource_name);
>> +			return -ENODEV;
>> +		}
>
> [Severity: Low]
> [Finding: fable-5-1-1-2]
> [Sources: fable-5-1, opus-5-5]
> This isn't a functional bug, but can this driver probe on any in-tree
> configuration?
>
> lan9645x_request_target_regmaps() needs 17 named regmaps from
> dev->parent, and lan9645x_reset_switch() relies on the parent having
> reset the chip. Nothing under drivers/mfd/ registers these regmaps or
> creates a child with the "microchip,lan96455s-switch" compatible.
>
> The commit message says:
>
>   "The MFD driver will come in a later series"
>
> Until that series lands, this driver and the tagger it selects can be
> built but never bound. Would it make sense to merge this together with
> the parent driver?
>
> [ ... ]