Message-ID: <20260131175243.1122906-2-kuba@kernel.org>
Date: Sat, 31 Jan 2026 09:52:44 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: daniel@...rotopia.org
Cc: Jakub Kicinski <kuba@...nel.org>,
	lxu@...linear.com,
	hkallweit1@...il.com,
	yweng@...linear.com,
	ajayaraman@...linear.com,
	andrew@...n.ch,
	netdev@...r.kernel.org,
	bxu@...linear.com,
	olteanv@...il.com,
	krzk+dt@...nel.org,
	linux-kernel@...r.kernel.org,
	lrosu@...linear.com,
	chad@...roe.io,
	conor+dt@...nel.org,
	devicetree@...r.kernel.org,
	robh@...nel.org,
	edumazet@...gle.com,
	pabeni@...hat.com,
	cezary.wilmanski@...ran.com,
	davem@...emloft.net,
	john@...ozen.org,
	frankwu@....de,
	jpovazanec@...linear.com,
	linux@...linux.org.uk,
	fchan@...linear.com,
	horms@...nel.org
Subject: Re: [net-next,v11,4/4] net: dsa: add basic initial driver for MxL862xx switches

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: add basic initial driver for MxL862xx switches

Add very basic DSA driver for MaxLinear's MxL862xx switches. In contrast
to previous MaxLinear switches the MxL862xx has a built-in processor that
runs a sophisticated firmware based on Zephyr RTOS. Interaction between
the host and the switch hence is organized using a software API of that
firmware rather than accessing hardware registers directly. Add
descriptions of the most basic firmware API calls to access the built-in
MDIO bus hosting the 2.5GE PHYs, basic port control as well as setting up
the CPU port. Implement a very basic DSA driver using that API which is
sufficient to get packets flowing between the user ports and the CPU port.

> diff --git a/drivers/net/dsa/mxl862xx/Kconfig b/drivers/net/dsa/mxl862xx/Kconfig
> new file mode 100644
> index 000000000000..3722260da7d8
> --- /dev/null
> +++ b/drivers/net/dsa/mxl862xx/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config NET_DSA_MXL862
> +	tristate "MaxLinear MxL862xx"
> +	depends on NET_DSA
> +	select MAXLINEAR_GPHY
> +	select NET_DSA_TAG_MXL_862XX
> +	help
> +	  This enables support for the MaxLinear MxL862xx switch family.
> +	  These switches have two 10GE SerDes interfaces, one typically
> +	  used as CPU port.
> +	   MxL86282 has eight 2.5 Gigabit PHYs
> +	   MxL86252 has five 2.5 Gigabit PHYs

This isn't a bug, but the last two lines have inconsistent indentation.
The help text lines describing MxL86282 and MxL86252 have three spaces
after the tab while the other help text lines have two spaces.

[Jakub] maybe add a "-" or such in front to make it clear this is a list?

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-api.h b/drivers/net/dsa/mxl862xx/mxl862xx-api.h
> new file mode 100644
> index 000000000000..535c37514455
> --- /dev/null
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-api.h

[ ... ]

> +/**
> + * struct mxl862xx_bridge_alloc - Bridge Allocation
> + * @bridge_id: If the bridge allocation is successful, a valid ID will be
> + *             returned in this field. Otherwise, INVALID_HANDLE is
> + *             returned. For bridge free, this field should contain a
> + *             valid ID returned by the bridge allocation. ID 0 is not
> + *             used for historic reasons.
> + *
> + * Used by MXL862XX_BRIDGE_ALLOC and MXL862XX_BRIDGE_FREE.
> + */
> +struct mxl862xx_bridge_alloc {
> +	__le16 bridge_id;
> +} __packed;

[ ... ]

> +/**
> + * struct mxl862xx_bridge_port_alloc - Bridge Port Allocation
> + * @bridge_port_id: If the bridge port allocation is successful, a valid ID
> + *                  will be returned in this field. Otherwise, INVALID_HANDLE
> + *                  is returned. For bridge port free, this field should
> + *                  contain a valid ID returned by the bridge port allocation.
> + *
> + * Used by MXL862XX_BRIDGE_PORT_ALLOC and MXL862XX_BRIDGE_PORT_FREE.
> + */
> +struct mxl862xx_bridge_port_alloc {
> +	__le16 bridge_port_id;
> +};

Is the __packed attribute missing here? The similar struct
mxl862xx_bridge_alloc has __packed, and both structures have identical
layout with a single __le16 field. Both are used for firmware
communication where padding could break the protocol.

[Jakub] please remove the __packed attribute on structs which are
        naturally packed according to C rules.

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
> new file mode 100644
> index 000000000000..83946b50ea5f
> --- /dev/null
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx.c

[ ... ]

> +static void mxl862xx_port_disable(struct dsa_switch *ds, int port)
> +{
> +	mxl862xx_port_state(ds, port, false);
> +}

Should the error return from mxl862xx_port_state() be logged?

The function mxl862xx_port_state() can fail and returns an error code,
but mxl862xx_port_disable() ignores this return value. While the DSA
framework defines port_disable as returning void so errors cannot be
propagated to the caller, silently ignoring errors could leave the port's
SDMA or FDMA enabled if the API call fails. The function
mxl862xx_port_fast_age() logs errors when its API call fails.

> +static int mxl862xx_add_single_port_bridge(struct dsa_switch *ds, int port)
> +{
> +	struct mxl862xx_bridge_port_config br_port_cfg = {};
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct mxl862xx_bridge_alloc br_alloc = {};
> +	int ret;
> +
> +	ret = MXL862XX_API_READ(ds->priv, MXL862XX_BRIDGE_ALLOC, br_alloc);
> +	if (ret) {
> +		dev_err(ds->dev, "failed to allocate a bridge for port %d\n", port);
> +		return ret;
> +	}
> +
> +	br_port_cfg.bridge_id = br_alloc.bridge_id;
> +	br_port_cfg.bridge_port_id = cpu_to_le16(port);
> +	br_port_cfg.mask = cpu_to_le32(MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_ID |
> +				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP |
> +				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING |
> +				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING);
> +	br_port_cfg.src_mac_learning_disable = true;
> +	br_port_cfg.vlan_src_mac_vid_enable = false;
> +	br_port_cfg.vlan_dst_mac_vid_enable = false;
> +	br_port_cfg.bridge_port_map[0] = cpu_to_le16(BIT(dp->cpu_dp->index));

Can this dereference NULL pointers?

The dsa_to_port() function can return NULL if the port is not found in
the switch tree. The code stores the result in dp but doesn't check if
dp is NULL before dereferencing dp->cpu_dp->index.

Looking at include/net/dsa.h, dsa_to_port() returns NULL when the port
is not found:

    static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p)
    {
        struct dsa_switch_tree *dst = ds->dst;
        struct dsa_port *dp;

        list_for_each_entry(dp, &dst->ports, list)
            if (dp->ds == ds && dp->index == p)
                return dp;

        return NULL;
    }

Additionally, even if dp is non-NULL, dp->cpu_dp could also be NULL if
the CPU port hasn't been properly assigned during initialization.

> +static int mxl862xx_setup_mdio(struct dsa_switch *ds)
> +{
> +	struct mxl862xx_priv *priv = ds->priv;
> +	struct device *dev = ds->dev;
> +	struct device_node *mdio_np;
> +	struct mii_bus *bus;
> +	static int idx;
> +	int ret;
> +
> +	bus = devm_mdiobus_alloc(dev);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->priv = priv;
> +	ds->user_mii_bus = bus;
> +	bus->name = KBUILD_MODNAME "-mii";
> +	snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);

Can concurrent probes race on the idx variable?

The static int idx is shared across all driver instances and accessed
without synchronization. If multiple instances of this driver are probed
concurrently on different CPU cores, they could race on the idx++
operation potentially leading to duplicate MII bus IDs. MII bus IDs must
be unique system-wide.

[ ... ]

> +static int mxl862xx_setup_cpu_bridge(struct dsa_switch *ds, int port)
> +{
> +	struct mxl862xx_bridge_port_config br_port_cfg = {};
> +	struct mxl862xx_priv *priv = ds->priv;
> +	u16 bridge_port_map = 0;
> +	struct dsa_port *dp;
> +
> +	/* CPU port bridge setup */
> +	br_port_cfg.mask = cpu_to_le32(MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP |
> +				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING |
> +				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING);
> +
> +	br_port_cfg.bridge_port_id = cpu_to_le16(port);
> +	br_port_cfg.src_mac_learning_disable = false;
> +	br_port_cfg.vlan_src_mac_vid_enable = true;
> +	br_port_cfg.vlan_dst_mac_vid_enable = true;
> +
> +	/* include all assigned user ports in the CPU portmap */
> +	dsa_switch_for_each_user_port(dp, ds) {
> +		if (dp->cpu_dp->index != port)
> +			continue;

Can dp->cpu_dp be dereferenced safely here?

The code dereferences dp->cpu_dp->index without checking if cpu_dp is
NULL. While the DSA framework should ensure user ports have a CPU port
assigned during initialization, there is no explicit NULL check. If
cpu_dp is NULL for any reason such as incomplete initialization or
configuration error, this will cause a NULL pointer dereference and
kernel crash.

> +	ret = dsa_register_switch(ds);
> +	if (ret)
> +		return ret;

Is dev_set_drvdata() called too early?

The function calls dev_set_drvdata(dev, ds) before calling
dsa_register_switch(ds). If dsa_register_switch() fails, the function
returns the error but leaves the driver data pointing to an unregistered
switch.

This creates a problem for cleanup paths. The mxl862xx_remove() function
retrieves the switch with dev_get_drvdata() and calls
dsa_unregister_switch(ds). If probe failed, ds is not NULL so the check
passes but the switch was never successfully registered. Similarly,
mxl862xx_shutdown() could be called and would also operate on an
unregistered switch. While dsa_switch_shutdown() has a check for
ds->setup flag that provides some protection, this is fragile and could
lead to inconsistent state or crashes in error scenarios.

[Jakub] AI is a bit pedantic about leaving the drvdata pointer in place
        but I guess it's not a bad thing to clean up
-- 
pw-bot: cr

Powered by blists - more mailing lists