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

[Daniel Golle <daniel@makrotopia.org>]

On Sat, Jan 31, 2026 at 09:52:44AM -0800, Jakub Kicinski wrote:
> > [...]
> > +/**
> > + * 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.

I'll do that for the upcoming submission of v12, but wonder if using
'#pragma pack(push, 1)' around the complete API header file wouldn't be
the better option, as anyway *everything* should always be packed there,
as the switch firmware expects data byte-aligned.

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

Do you think it's worth to fix this also in other drivers which follow
the same pattern? (bcm_sf2, mt7530 and mv88e6xxx if OF node is NULL --
if there is an OF node it uses %pOF, and I'm not sure that's such a good
idea either)

Apart from being racy, this also means the MII bus name depends on probe
order which isn't very nice either. Using

snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));

may result in a slightly more ugly-looking, but predictable and
non-racy (ie. always unique) name.

Other than that there is also
snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d.%d",
         KBUILD_MODNAME, ds->dst->index, ds->index);

The real question here is if we consider MII bus names to be ABI,
ie. if they should never change once a driver is part of the kernel.