Re: [PATCH net-next v23 4/9] mfd: an8855: Add support for Airoha AN8855 Switch

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 5 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds MFD_AIROHA_AN8855, and both the commit message
  and the Kconfig help say this MFD provides an NVMEM provider.
- [Medium] an8855_phy_bus_register() names each virtual MDIO bus with
  snprintf(mrc.name, MII_BUS_ID_SIZE, "an8855:%u-phy%u",…
- [Medium] an8855_phy_bus_register() reads the PHY address from DT "reg"
  and only checks `phy_offset = addr - priv->switch_addr; if (phy_offset…
- [Medium] In an8855_mdio_register(), of_node_put(mdio_np) comes before
  the 'exit:' label.
- [Medium] The MFD binding puts `reset-gpios` on the core `airoha,an8855`
  node, and its example wires `reset-gpios = <&pio 39 GPIO_ACTIVE_HIGH>`.
- [Low] The MFD binding (airoha,an8855.yaml) leaves the 'efuse' child
  optional: required is compatible, reg, mdio, ethernet-switch.
- [Low] Both examples in the AN8855 PHY binding (airoha,an8855-phy.yaml)
  use "ethernet-phy-idc0ff.0410", "ethernet-phy-ieee802.3-c45".
- [Low] an8855_core_probe() clears AN8855_RG_GPHY_AFE_PWD ("Release
  global PHY power down") before registering the PHYs and the MFD…

--

mfd: an8855: Add support for Airoha AN8855 Switch

This adds an MFD core driver for the Airoha AN8855 switch, which is
accessed over MDIO. It adds a paged regmap for the switch registers and
one mdio-regmap bus for each internal PHY. It also registers eFuse and
DSA switch child cells.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index e4fd4572472f9..afbdcec0fce8f 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -54,6 +54,20 @@ config MFD_ALTERA_SYSMGR
>  	  using regmap_mmio accesses for ARM32 parts and SMC calls to
>  	  EL3 for ARM64 parts.
>  
> +config MFD_AIROHA_AN8855
> +	tristate "Airoha AN8855 Switch Core"
> +	select MFD_CORE
> +	select REGMAP
> +	select PHYLIB
> +	select MDIO_REGMAP
> +	depends on NETDEVICES && OF
> +	help
> +	  Support for the Airoha AN8855 Switch Core. This is an SoC
> +	  that provides various peripherals, to count, i2c, an Ethrnet
> +	  Switch, a CPU timer, GPIO, eFUSE.
> +
> +	  Currently it provides a DSA switch and a NVMEM provider.

[Severity: Medium]
The help text and the commit message both say this provides an NVMEM
provider. However, NVMEM_AN8855_EFUSE in drivers/nvmem/Kconfig still
only has:

	depends on COMPILE_TEST

It was reduced to that by commit 4796eaafd6a1 ("nvmem: an8855: drop an
unused Kconfig symbol"), because MFD_AIROHA_AN8855 was referenced but
never defined.

Now that this patch defines the symbol, should the nvmem entry go back
to "depends on MFD_AIROHA_AN8855 || COMPILE_TEST"? NET_DSA_AN8855, added
later in the series, already uses that form.

As it stands, the eFuse driver can't be enabled when COMPILE_TEST is not
set. That leaves the an8855-efuse cell with no driver to bind. The PHY
nodes' nvmem-cells (tx_a..tx_d) then point at a provider that never
registers, so an8855_probe()->an8855_get_r50ohm_val() in the AN8855 PHY
driver can't get its calibration cells.

[ ... ]

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

[ ... ]

> +static const struct mfd_cell an8855_cells[] = {
> +	MFD_CELL_OF("an8855-efuse", NULL, NULL, 0, 0,
> +		    "airoha,an8855-efuse"),

[Severity: Low]
The MFD binding (airoha,an8855.yaml) makes the efuse child optional. Its
required list is compatible, reg, mdio and ethernet-switch. This cell is
still registered every time.

When the DT has no efuse node, won't mfd_add_device() print a "Failed to
locate of_node" warning and register the platform device anyway? The
efuse driver would then bind by platform name and register an NVMEM
device that the DT never described.

> +	MFD_CELL_OF("an8855-switch", NULL, NULL, 0, 0,
> +		    "airoha,an8855-switch"),
> +};

[ ... ]

> +static int an8855_phy_bus_register(struct device *dev, struct an8855_core_priv *priv,
> +				   struct device_node *phy_np,
> +				   struct an8855_phy_bus *phy_bus_info)
> +{

[ ... ]

> +	ret = of_property_read_u32(phy_np, "reg", &addr);
> +	if (ret)
> +		return ret;
> +
> +	phy_offset = addr - priv->switch_addr;
> +	if (phy_offset >= AN8855_MAX_PHY_PORT)
> +		return -EINVAL;

[Severity: Medium]
Is addr ever checked against PHY_MAX_ADDR? The only check here is
relative to switch_addr. If the switch sits at MDIO address 28..31, a
child reg of 32..35 passes.

That addr goes straight to fwnode_mdiobus_register_phy() in
an8855_core_probe(), which skips the of_mdio_parse_addr() check done on
the normal OF MDIO path. For a C22 compatible such as
ethernet-phy-idc0ff.0410, the call chain is:

an8855_core_probe()
  fwnode_mdiobus_register_phy(bus, node, addr)
    phy_device_create(bus, addr, ...)
      dev->irq = bus->irq[addr];
    fwnode_mdiobus_phy_device_register()
      mdio->irq[addr] = rc;
      mdio->phy_ignore_ta_mask |= 1 << addr;
      phy_device_register()
        mdiobus_register_device()
          mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev;

Could this read and write past the end of bus->irq[] and
bus->mdio_map[]? Those writes could overwrite later fields of struct
mii_bus, such as reset_delay_us, reset_post_delay_us or reset_gpiod.

Such a DT would violate mdio.yaml (reg maximum 31), but nothing enforces
that at runtime.

[ ... ]

> +	mrc.regmap = regmap;
> +	mrc.parent = dev;
> +	mrc.valid_addr = addr;
> +	snprintf(mrc.name, MII_BUS_ID_SIZE, "an8855:%u-phy%u", priv->switch_addr,
> +		 phy_offset);
> +
> +	bus = devm_mdio_regmap_register(dev, &mrc);
> +	if (IS_ERR(bus))
> +		return PTR_ERR(bus);

[Severity: Medium]
Can two AN8855 chips on different host MDIO buses end up with the same
mii_bus name? The name is built only from switch_addr and phy_offset.
Two switches at the same address on two MACs would therefore both
produce the same "an8855:<addr>-phy<n>" names.

devm_mdio_regmap_register() copies this into mii->id, and
__mdiobus_register() then does:

	dev_set_name(&bus->dev, "%s", bus->id);

The bus is in mdio_bus_class, so the second device_add() looks like it
would fail with -EEXIST. The whole second an8855_core_probe() would fail
with it. PHY device names built with PHY_ID_FMT would collide in the
same way.

Could the name include something unique to the parent, such as
dev_name(dev)?

[ ... ]

> +static int an8855_mdio_register(struct device *dev, struct an8855_core_priv *priv,
> +				struct an8855_phy_bus *phy_bus_infos)
> +{
> +	struct device_node *mdio_np;
> +	int ret = 0;
> +	int i = 0;
> +
> +	mdio_np = of_get_child_by_name(dev->of_node, "mdio");
> +	if (!mdio_np)
> +		return -ENODEV;
> +
> +	for_each_available_child_of_node_scoped(mdio_np, phy_np) {
> +		if (i >= AN8855_MAX_PHY_PORT) {
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
> +		ret = an8855_phy_bus_register(dev, priv, phy_np,
> +					      &phy_bus_infos[i]);
> +		if (ret)
> +			goto exit;
> +
> +		i++;
> +	}
> +
> +	of_node_put(mdio_np);
> +exit:

[Severity: Medium]
Does this leak the reference on mdio_np taken by
of_get_child_by_name()? of_node_put(mdio_np) sits above the exit label.
Both gotos inside the loop skip it: the one for too many children and
the one for an8855_phy_bus_register() failing.

This function also mixes goto-based cleanup with the __free(device_node)
scoped iterator from for_each_available_child_of_node_scoped().
include/linux/cleanup.h asks that goto and cleanup helpers not be mixed
in the same function.

Would declaring mdio_np with __free(device_node) and dropping the gotos
fix both?

[ ... ]

> +static int an8855_core_probe(struct mdio_device *mdiodev)
> +{

[ ... ]

> +	/* Reset PHY page for switch address */
> +	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	ret = an8855_mii_set_page(priv, priv->switch_addr,
> +				  AN8855_PHY_PAGE_STANDARD);

[Severity: Medium]
The MFD binding documents reset-gpios on the airoha,an8855 node, and its
example uses reset-gpios = <&pio 39 GPIO_ACTIVE_HIGH>. Does anything
request or deassert that GPIO before this first MDIO access?

an8855_core_probe() doesn't touch it. mdiobus_register_device() only
sets up reset for MDIO_DEVICE_FLAG_PHY devices, so mdio_device_reset()
in mdio_probe() does nothing for this mdio_device. The DSA driver at the
end of the series doesn't handle the reset line either.

There is also no wait for the chip to finish its internal init before
the page select, the CREV read and the AFE_PWD write.
linux/gpio/consumer.h is included but unused, and so is this define:

#define AN8855_HWTRAP			0x1000009c

Was a reset and HWTRAP poll sequence lost when this code moved into the
MFD? On boards where reset is still asserted at boot, it looks like
an8855_read_switch_id() would fail and the switch would be unusable.

> +	mutex_unlock(&priv->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: Low]
Nothing appears to set the AN8855_RG_GPHY_AFE_PWD bits again later.
fwnode_mdiobus_register_phy() or devm_mfd_add_devices() can still fail
after this write. The err_put_fwnodes path only drops fwnode references,
and there is no .remove callback or devm action for unbind.

Should power down be restored on those paths, so the PHY analog front
ends aren't left powered with no driver managing the ports?

> +
> +	/* 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);

[Severity: Low]
Both examples in Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml
use:

compatible = "ethernet-phy-idc0ff.0410",
             "ethernet-phy-ieee802.3-c45";

The buses created by devm_mdio_regmap_register() only provide C22
read/write callbacks. With that compatible, fwnode_mdiobus_register_phy()
does:

	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
		phy = get_phy_device(bus, addr, is_c45);

The explicit PHY ID is then ignored. get_phy_c45_ids() fails because
__mdiobus_c45_read() returns -EOPNOTSUPP when bus->read_c45 is not set.

Would a DT copied from that example make the whole an8855_core_probe()
fail? The MFD binding example uses the c22 compatible, so the two
bindings seem to disagree.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925182435.21207-1-ansuelsmth%40gmail.com