Re: [net-next,v11,4/4] net: dsa: add basic initial driver for MxL862xx switches

[Daniel Golle <daniel@makrotopia.org>]

On Mon, Feb 02, 2026 at 11:44:05AM +0200, Vladimir Oltean wrote:
> On Sat, Jan 31, 2026 at 09:52:44AM -0800, Jakub Kicinski wrote:
> > > +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?
> 
> No. DSA will not make calls to ds->ops->port_setup() with "port" values
> for which dsa_to_port(ds, port) will not find a struct dsa_port *.

So I should again remove the check for !=NULL here and...

> > 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.
> 
> mxl862xx_add_single_port_bridge() has been called when all other port
> types except user ports have been excluded. All user and DSA ports have
> a non-NULL dp->cpu_dp pointer after dsa_tree_setup_cpu_ports() runs,
> i.e. also at the time of ds->ops->port_setup().

here, as well as in mxl862xx_setup_cpu_bridge(), right?

> 
> > > +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.
> > 
> > [ ... ]
> 
> Technically ds->ops->setup() runs under dsa2_mutex, but the "static int idx"
> is still not ideal due to the ever-increasing index upon unbinding and
> rebinding.

As mentioned in my reply to Jakub[1] many drivers follow this pattern and it would
imho be a good idea to agree on a convention or even provide a helper function
which names the MII bus for DSA drivers. What do you think?

[1]: https://patchwork.kernel.org/comment/26768088/

> > > +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.
> 
> No incompletely initialized ports are published to dst->ports.

Will remove the !=NULL check then again, see above.

> 
> > > +	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
> 
> Can somebody gently explain what is the point? It is easily testable
> that if mdio_driver :: probe() fails, .remove() or .shutdown() will not
> be called. It really does not matter whether dev_set_drvdata() was
> called or not.