Re: [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 14/12/2024 08:59, Simon Horman wrote:
> On Thu, Dec 12, 2024 at 12:53:42PM +1300, Chris Packham wrote:
> > Add a driver for the MDIO controller on the RTL9300 family of Ethernet
> > switches with integrated SoC. There are 4 physical SMI interfaces on the
> > RTL9300 but access is done using the switch ports so a single MDIO bus
> > is presented to the rest of the system.
> >
> > Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> ...
>
> > diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c
> ...
>
> > +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> > +{
> > +	u32 port_addr[5] = { };
> > +	u32 poll_sel[2] = { 0, 0 };
> > +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> > +	int i, err;
> > +
> > +	for (i = 0; i < MAX_PORTS; i++) {
> > +		int pos;
> > +
> > +		if (priv->smi_bus[i] > 3)
> > +			continue;
> > +
> > +		pos = (i % 6) * 5;
> > +		port_addr[i / 6] |=  priv->smi_addr[i] << pos;
> Hi Chris,
>
> The maximum index of port_addr accessed above is
> (MAX_PORTS - 1) / 6 = (32 - 1) / 6 = 5.
> But port_addr only has five elements (maximum index of 4).
> So this will overflow.
>
> Flagged by Smatch.

Drat. It's more complicated than that. The maximum number of _physical_ 
ports on the RTL9300 is 28 (i.e. 0-27). In some other places port 28 is 
used to mean the CPU port (i.e. the DMA interface) and in others it just 
uses 32 possible ports because that makes the tables entries align 
nicely. Since this is just the MDIO interface, setting MAX_PORTS to 28 
here seems like the sensible thing to do.

> > +
> > +		pos = (i % 16) * 2;
> > +		poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> > +	}
> > +
> > +	for (i = 0; i < MAX_SMI_BUSSES; i++) {
> > +		if (priv->smi_bus_isc45[i]) {
> > +			glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
> > +			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> > +		}
> > +	}
> > +
> > +	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
> > +				port_addr, 5);
> > +	if (err)
> > +		return err;
> > +
> > +	err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
> > +				poll_sel, 2);
> > +	if (err)
> > +		return err;
> > +
> > +	err = regmap_update_bits(priv->regmap, priv->reg_base + SMI_GLB_CTRL,
> > +				 glb_ctrl_mask, glb_ctrl_val);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +static int realtek_mdiobus_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct realtek_mdio_priv *priv;
> > +	struct fwnode_handle *child;
> > +	struct mii_bus *bus;
> > +	int err;
> > +
> > +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	bus->name = "Reaktek Switch MDIO Bus";
> > +	bus->read_c45 = realtek_mdio_read_c45;
> > +	bus->write_c45 =  realtek_mdio_write_c45;
> > +	bus->parent = dev;
> > +	priv = bus->priv;
> > +
> > +	priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> > +	if (IS_ERR(priv->regmap))
> > +		return PTR_ERR(priv->regmap);
> > +
> > +	err = device_property_read_u32(dev, "reg", &priv->reg_base);
> > +	if (err)
> > +		return err;
> > +
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> > +
> > +	device_for_each_child_node(dev, child) {
> > +		u32 pn, smi_addr[2];
> > +
> > +		err = fwnode_property_read_u32(child, "reg", &pn);
> > +		if (err)
> > +			return err;
> > +
> > +		if (pn > MAX_PORTS)
> > +			return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> > +
> > +		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> > +		if (err) {
> > +			smi_addr[0] = 0;
> > +			smi_addr[1] = pn;
> > +		}
> > +
> > +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> > +			priv->smi_bus_isc45[smi_addr[0]] = true;
> > +
> > +		priv->smi_bus[pn] = smi_addr[0];
> > +		priv->smi_addr[pn] = smi_addr[1];
> The condition about 15 lines above ensures that the maximum value of pn
> is MAX_PORTS. But if this is the case then the above will overflow
> both smi_bus and smi_addr as they each have MAX_PORTS elements
> (maximum index of MAX_PORTS - 1).
>
> I suspect the condition above should be updated to:
>
> 	if (pn >= MAX_PORTS)
> 		return ...
>
> Also flagged by Smatch.
Good catch thanks. Will fix in v2.
> > +	}
> > +
> > +	err = realtek_mdiobus_init(priv);
> > +	if (err)
> > +		return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
> > +
> > +	err = devm_of_mdiobus_register(dev, bus, dev->of_node);
> > +	if (err)
> > +		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> > +
> > +	return 0;
> > +}
> ...