Re: [PATCH net-next v20 04/10] mfd: an8855: Add support for Airoha AN8855 Switch MFD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing 'select REGMAP' dependency for MDIO_REGMAP and devm_regmap_init().
- [High] Incorrect reset GPIO polarity logic actively leaves the switch in a reset state.
- [High] PHY page state is corrupted when switch regmap accesses interleave with PHY paged accesses.
- [High] Unprotected call to 'an8855_mii_set_page' in probe triggers a lockdep splat and data race.
- [Medium] Signed integer overflow when bit-shifting a 16-bit variable.
- [Medium] Duplicate virtual MII bus IDs if multiple switches exist.
- [Low] Selection of non-existent Kconfig symbol 'MDIO_DEVICE'.
--

--- Patch [4]: [PATCH net-next v20 04/10] mfd: an8855: Add support for Airoha AN8855 Switch MFD ---
commit 302d7f55459ecc6ab21029f136b77629fdc1e7cb
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: mfd: an8855: Add support for Airoha AN8855 Switch MFD

This commit adds core MFD support for the Airoha AN8855 switch, registering a 
DSA switch and an NVMEM provider. It implements an MDIO regmap driver for
the internal PHYs to handle the dual usage of PHYs as both PHY and switch
accessors.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 763ce6a34782b..4d763ed6e53d5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -54,6 +54,19 @@ 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 MDIO_DEVICE

[Severity: Low]
Is MDIO_DEVICE a valid Kconfig symbol in the tree, or was it intended to be
used as a C macro? Selecting a non-existent symbol has no effect and is
silently ignored by Kconfig.

[Severity: High]
Does this driver also need to select REGMAP? 

Since the driver relies heavily on the regmap subsystem and explicitly calls
devm_regmap_init(), failing to select it will cause the regmap APIs to
fallback to -EINVAL stubs if CONFIG_REGMAP isn't enabled by another subsystem.

> +	select MDIO_REGMAP
> +	depends on NETDEVICES && OF

[ ... ]

> diff --git a/drivers/mfd/airoha-an8855.c b/drivers/mfd/airoha-an8855.c
> new file mode 100644
> index 0000000000000..0e8b3550d9e5e
> --- /dev/null
> +++ b/drivers/mfd/airoha-an8855.c

[ ... ]

> +static int an8855_mii_set_page(struct an8855_core_priv *priv, u8 addr,
> +			       u8 page) __must_hold(&priv->bus->mdio_lock)
> +{
> +	struct mii_bus *bus = priv->bus;
> +	int ret;
> +
> +	ret = __mdiobus_write(bus, addr, AN8855_PHY_SELECT_PAGE, page);

[ ... ]

> +static int an8855_mii_read32(struct mii_bus *bus, u8 phy_id, u32 reg,
> +			     u32 *val) __must_hold(&bus->mdio_lock)
> +{

[ ... ]

> +	lo = __mdiobus_read(bus, phy_id, AN8855_PBUS_RD_DATA_LOW);
> +	if (lo < 0) {
> +		ret = lo;
> +		goto err;
> +	}
> +
> +	*val = ((u16)hi << 16) | ((u16)lo & 0xffff);

[Severity: Medium]
Could this bit-shift cause a signed integer overflow? The cast to (u16)
causes integer promotion to a signed int. If the highest bit of 'hi' is
set, shifting it by 16 bits overflows. Casting to u32 before the shift
would avoid this.

> +
> +	return 0;
> +err:

[ ... ]

> +static int an8855_regmap_phy_reset_page(struct an8855_core_priv *priv,
> +					int phy) __must_hold(&priv->bus->mdio_lock)
> +{
> +	/* Check PHY page only for addr shared with switch */
> +	if (phy != priv->switch_addr)
> +		return 0;
> +
> +	/* Don't restore page if it's not set to Switch page */
> +	if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
> +					    AN8855_PHY_PAGE_EXTENDED_4))
> +		return 0;
> +
> +	/*
> +	 * Restore page to 0, PHY might change page right after but that
> +	 * will be ignored as it won't be a switch page.
> +	 */
> +	return an8855_mii_set_page(priv, phy, AN8855_PHY_PAGE_STANDARD);

[Severity: High]
Could unconditionally restoring the page to STANDARD corrupt the PHY page
state if a switch regmap access interleaves with a PHY paged access?

If the PHY framework locks its virtual MDIO bus to perform a paged
access sequence, a concurrent switch access can interleave since it locks
the parent MDIO bus independently. When the PHY access resumes, forcing
the page to STANDARD here would overwrite the PHY's requested page,
leading to data corruption.

> +}

[ ... ]

> +static int an8855_phy_register(struct device *dev, struct an8855_core_priv *priv,
> +			       struct device_node *phy_np)
> +{
> +	struct mdio_regmap_config mrc = { };

[ ... ]

> +	mrc.regmap = regmap;
> +	mrc.parent = dev;
> +	mrc.valid_addr = addr;
> +	snprintf(mrc.name, MII_BUS_ID_SIZE, "an8855-phy%d-mii", addr);

[Severity: Medium]
Will this naming scheme cause duplicate sysfs MII bus IDs if multiple
switches exist on the system? 

Generating the ID using only the local MDIO address might lead to collisions 
if there is another switch on a different parent MDIO bus sharing the same 
local address. Incorporating the parent device name would ensure it is globally
unique.

> +
> +	bus = devm_mdio_regmap_register(dev, &mrc);
> +	if (IS_ERR(bus))

[ ... ]

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

[ ... ]

> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(reset_gpio))
> +		return PTR_ERR(reset_gpio);
> +
> +	if (reset_gpio) {
> +		usleep_range(100000, 150000);
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +		usleep_range(100000, 150000);
> +		gpiod_set_value_cansleep(reset_gpio, 1);

[Severity: High]
Does this sequence actively leave the switch hardware in a reset state?

Since gpiod_set_value_cansleep abstracts polarity (where 1 means asserted
and 0 means deasserted), calling it with 1 right before attempting to
poll the HWTRAP register will likely cause the initialization and subsequent
MDIO poll to fail.

> +
> +		/* Poll HWTRAP reg to wait for Switch to fully Init */
> +		ret = regmap_read_poll_timeout(regmap, AN8855_HWTRAP, val,
> +					       val, 20, 200000);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Reset PHY page for switch address */
> +	ret = an8855_mii_set_page(priv, priv->switch_addr,
> +				  AN8855_PHY_PAGE_STANDARD);

[Severity: High]
Should we hold the mdio_lock before calling an8855_mii_set_page() here?

an8855_mii_set_page() expects the lock to be held and uses the lockless 
__mdiobus_write() internal helper. Calling it unprotected in the probe 
sequence will trigger a lockdep splat and can race with concurrent
accesses on the same parent MDIO bus.

> +	if (ret)
> +		return ret;

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