Re: [PATCH v2 2/2] mfd: an8855: Add support for Airoha AN8855 Switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mfd: an8855: missing reset-gpios handling in an8855_probe()
- [High] mfd: an8855: missing PHY_MAX_ADDR check in an8855_phy_bus_register()
- [Medium] mfd: an8855: register internal PHYs after an8855-efuse to avoid genphy fallback
--

commit b751ed12fefc10d7bebaac0f5731b709d5c0d2bd
Author: Christian Marangi <ansuelsmth@gmail.com>

mfd: an8855: Add support for Airoha AN8855 Switch

This commit adds an MFD core driver for the Airoha AN8855 switch, registering
virtual MDIO buses via mdio-regmap for the internal PHYs and MFD cells for the
DSA switch and NVMEM provider.

> diff --git a/drivers/mfd/airoha-an8855.c b/drivers/mfd/airoha-an8855.c
> new file mode 100644
> index 0000000000000..6a270555db5cf
> --- /dev/null
> +++ b/drivers/mfd/airoha-an8855.c
> @@ -0,0 +1,579 @@

[ ... ]

> +#include <linux/bitfield.h>
> +#include <linux/fwnode_mdio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/core.h>

[ ... ]

> +static int an8855_phy_bus_register(struct device *dev, struct an8855 *ddata,
> +				   struct device_node *phy_np,
> +				   struct an8855_phy_bus *phy_bus_info)
> +{
> +	struct mdio_regmap_config mrc = { };
> +	struct an8855_phy *phy_ddata;
> +	struct regmap *regmap;
> +	u32 addr, phy_offset;
> +	struct mii_bus *bus;
> +	int ret;
> +
> +	ret = of_property_read_u32(phy_np, "reg", &addr);
> +	if (ret)
> +		return ret;
> +
> +	phy_offset = addr - ddata->switch_addr;
> +	if (phy_offset >= AN8855_MAX_PHY_PORT)
> +		return -EINVAL;

[Severity: High]
Can this cause out-of-bounds accesses on bus->irq[] and bus->mdio_map[] if
ddata->switch_addr is between 28 and 31 and a child PHY node specifies a reg
value >= 32 (PHY_MAX_ADDR)?

In drivers/mfd/airoha-an8855.c, an8855_phy_bus_register() reads addr via
of_property_read_u32() and only checks that addr - ddata->switch_addr is less
than AN8855_MAX_PHY_PORT (5), without checking that addr < PHY_MAX_ADDR (32)
or using of_mdio_parse_addr().

When an8855_probe() subsequently passes phy_bus_infos[i].addr to
fwnode_mdiobus_register_phy() for a node with an ethernet-phy-id compatible
string, the call chain:

  an8855_probe()
    fwnode_mdiobus_register_phy()
      fwnode_mdiobus_phy_device_register()
        phy_device_register()
          mdiobus_register_device()

indexes mdio->irq[addr] and mdiodev->bus->mdio_map[mdiodev->addr] (both sized
PHY_MAX_ADDR) and evaluates 1 << addr without bounds checking:

  drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_phy_device_register() {
      ...
      if (rc > 0) {
          phy->irq = rc;
          mdio->irq[addr] = rc;
      } else {
          phy->irq = mdio->irq[addr];
      }

      if (fwnode_property_read_bool(child, "broken-turn-around"))
          mdio->phy_ignore_ta_mask |= 1 << addr;
      ...
  }

  drivers/net/phy/mdio_device.c:mdiobus_register_device() {
      if (mdiodev->bus->mdio_map[mdiodev->addr])
          return -EBUSY;
      ...
  }

Should an8855_phy_bus_register() also verify that addr < PHY_MAX_ADDR (or use
of_mdio_parse_addr()) before accepting the PHY address?

[ ... ]

> +static int an8855_probe(struct mdio_device *mdiodev)
> +{
> +	struct an8855_phy_bus phy_bus_infos[AN8855_MAX_PHY_PORT] = { };
> +	struct device *dev = &mdiodev->dev;
> +	struct regmap *regmap;
> +	struct an8855 *ddata;
> +	int num_phys;
> +	int i, ret;

[ ... ]

> +	/* Reset PHY page for switch address */
> +	mutex_lock_nested(&ddata->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	ret = an8855_mii_set_page(ddata, ddata->switch_addr,
> +				  AN8855_PHY_PAGE_STANDARD);
> +	mutex_unlock(&ddata->bus->mdio_lock);
> +	if (ret)
> +		goto err_put_fwnodes;
> +
> +	ret = an8855_read_switch_id(dev, regmap);
> +	if (ret)
> +		goto err_put_fwnodes;
> +
> +	/* Release global PHY power down */
> +	ret = regmap_write(regmap, AN8855_RG_GPHY_AFE_PWD, 0x0);
> +	if (ret)
> +		goto err_put_fwnodes;

[Severity: High]
Does an8855_probe() need to request and deassert reset-gpios before accessing
the switch registers over MDIO?

Documentation/devicetree/bindings/mfd/airoha,an8855.yaml defines reset-gpios
on the airoha,an8855 node, and drivers/mfd/airoha-an8855.c includes
linux/gpio/consumer.h, but never requests or deasserts the GPIO.

Because airoha,an8855 is a generic MDIO device rather than a PHY device,
__of_mdiobus_parse_phys() registers it via of_mdiobus_register_device()
without MDIO_DEVICE_FLAG_PHY set. In drivers/net/phy/mdio_device.c,
mdiobus_register_device() only registers reset lines for PHY devices:

  if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY) {
      err = mdio_device_register_reset(mdiodev);
      if (err)
          return err;

      /* Assert the reset signal */
      mdio_device_reset(mdiodev, 1);
  }

As a result, the MDIO core does not manage reset-gpios for this device. If the
reset line is asserted when an8855_probe() runs, the MDIO accesses in
an8855_mii_set_page() and an8855_read_switch_id() will fail; conversely, if a
child MFD driver toggles reset-gpios later during devm_mfd_add_devices(), it
would reset the switch and wipe out the AN8855_RG_GPHY_AFE_PWD write and
internal PHY state initialized here in an8855_probe().

> +
> +	/* Register PHYs */
> +	for (i = 0; i < num_phys; i++) {
> +		ret = fwnode_mdiobus_register_phy(phy_bus_infos[i].bus,
> +						  phy_bus_infos[i].node,
> +						  phy_bus_infos[i].addr);
> +		fwnode_handle_put(phy_bus_infos[i].node);
> +		phy_bus_infos[i].node = NULL;
> +
> +		if (ret)
> +			goto err_put_fwnodes;
> +	}
> +
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, an8855_cells,
> +				    ARRAY_SIZE(an8855_cells), NULL, 0,
> +				    NULL);

[Severity: Medium]
Can registering the internal PHYs before the an8855-efuse MFD cell cause
phy_attach_direct() to bind the fallback genphy_driver instead of the
dedicated PHY driver?

In Documentation/devicetree/bindings/mfd/airoha,an8855.yaml, each internal
ethernet-phy node consumes nvmem-cells from the an8855-efuse child node, which
causes fw_devlink to create supplier links from the efuse node to the PHY
nodes.

In an8855_probe(), fwnode_mdiobus_register_phy() is called for all internal
PHYs before devm_mfd_add_devices() creates the an8855-efuse and an8855-switch
devices. During fwnode_mdiobus_register_phy() -> phy_device_register() ->
device_add(), each PHY defers probe with -EPROBE_DEFER because its nvmem
supplier (an8855-efuse) has not been registered yet, leaving
phydev->mdio.dev.driver as NULL on deferred_probe_pending_list.

Immediately afterwards, devm_mfd_add_devices() registers an8855-efuse (which
queues deferred_probe_work asynchronously) and then synchronously registers
an8855-switch. Because phy-handle is not tracked by fw_devlink, an8855-switch
can probe and call dsa_register_switch() -> phylink_of_phy_connect() ->
phy_attach_direct() before deferred_probe_work runs. When phy_attach_direct()
sees that d->driver is NULL, it forcibly binds genphy_driver and removes the
PHY from the deferred probe list:

  drivers/net/phy/phy_device.c:phy_attach_direct() {
      ...
      if (!d->driver) {
          if (phydev->is_c45)
              d->driver = &genphy_c45_driver.mdiodrv.driver;
          else
              d->driver = &genphy_driver.mdiodrv.driver;

          phydev->is_genphy_driven = 1;
      }
      ...
  }

Should an8855-efuse be registered before fwnode_mdiobus_register_phy() (or the
PHYs registered before an8855-switch after an8855-efuse is available) so the
internal PHYs can bind to their dedicated driver without deferring?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103203034.12869-1-ansuelsmth@gmail.com?part=2