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

[Vladimir Oltean]

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 *.

> 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().

> > +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.

> > +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.

> > +	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.