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

On Wed, 29 Jul 2026, [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Concurrent accesses by the switch regmap and the PHY driver on the shared MDIO address corrupt the PHY's hardware page state, causing arbitrary out-of-bounds PHY register reads/writes.
> - [High] MDIO child buses and PHYs are registered before the switch is taken out of reset and initialized, causing PHY probe failures.
> - [High] The reset GPIO sequence incorrectly leaves the switch held in reset, and the subsequent poll loop is vulnerable to false successes.
> - [High] `an8855_regmap_update_bits` fails to mask `write_val`, risking modification of adjacent bits.
> - [Medium] The driver assumes the hardware page register is 0 at boot without synchronizing this assumption, potentially causing MDIO reads from the wrong page.
> - [Low] The patch violates multiple MFD subsystem guidelines regarding naming conventions and commit message formatting.

I'll leave these with you! :)

> --
> 
> --- Patch [4]: [PATCH net-next v19 04/10] mfd: an8855: Add support for Airoha AN8855 Switch MFD ---
> Please note that due to mailing list constraints on quoting large blocks of 
> code, this review is provided as a free-form summary without direct patch 
> quotes.
> 
> commit 9fc0a25ce584ed2b0b637f4a623a857dd535edcc
> Author: Christian Marangi <[email protected]>
> mfd: an8855: Add support for Airoha AN8855 Switch MFD
> 
> This patch adds support for the Airoha AN8855 Switch MFD, providing a DSA
> switch and an NVMEM provider. It introduces a regmap over MDIO for switch
> and PHY accesses.
> 
> [Severity: High]
> In an8855_regmap_read(), the switch regmap acquires the parent lock and sets
> the hardware page to EXTENDED_4. Does this interleave dangerously with the
> PHY driver's paged operations?
> 
> Since the PHY driver paged operations hold the child MDIO bus lock, the
> parent lock is acquired and released for each transaction (page write, data
> read, restore). Can a switch regmap operation interleave between the PHY's
> page write and data read, changing the hardware page?
> 
> Furthermore, when the PHY driver resumes, an855_regmap_phy_reset_page()
> resets the page to STANDARD (0), ignoring the PHY driver's intended page.
> Also, an8855_regmap_phy_write() writes the page without updating the cached
> priv->current_page state. Will this lead to arbitrary out-of-bounds PHY
> register reads and writes?
> 
> [Severity: High]
> In an8855_core_probe(), the driver calls an855_mdio_register() before
> toggling the reset GPIO and clearing the global PHY power-down.
> 
> Since fwnode_mdiobus_register_phy() triggers the MDIO subsystem to read
> the PHY ID registers to detect devices, won't these accesses fail or time
> out if the switch is still in reset and powered down? Should the MDIO and
> PHY registration be moved after the hardware initialization sequence is
> complete?
> 
> [Severity: High]
> Looking at the reset sequence in an8855_core_probe(), the code calls
> gpiod_set_value_cansleep(reset_gpio, 1) at the end. Since the gpiod API
> treats a logical 1 as asserted regardless of polarity, does this permanently
> hold the switch in reset?
> 
> Immediately after this, regmap_read_poll_timeout() waits for the HWTRAP
> register to be non-zero. Since the device is in reset, won't the 32-bit
> MDIO read reconstruct 0xffffffff (as __mdiobus_read returns 0xffff on error),
> causing the poll loop to instantly and falsely succeed?
> 
> [Severity: High]
> In an8855_regmap_update_bits(), the implementation performs
> val &= ~mask followed by val |= write_val.
> 
> Since the regmap core does not guarantee that write_val is already pre-masked
> by the caller, could this unintentionally corrupt adjacent bits in the switch
> registers if write_val contains bits outside the mask? Would it be safer to
> explicitly mask it using val |= (write_val & mask)?
> 
> [Severity: Medium]
> In an855_regmap_phy_reset_page(), the function skips restoring the page
> if priv->current_page is 0. Since priv is allocated with kzalloc(), this
> assumes the hardware page is also 0 at boot.
> 
> If the bootloader or a previous kernel execution left the hardware in a
> different page, such as EXTENDED_4, will subsequent PHY reads access the
> wrong hardware page because the software state does not match? Should the
> page register be explicitly initialized during probe?
> 
> [Severity: Low]
> This isn't a bug, but there are a few MFD subsystem style and naming
> guideline violations.
> 
> Does the commit message description unnecessarily include the string "MFD"?
> Also, the private structure is named struct an8855_core_priv with an
> instance variable named priv. The MFD guidelines prefer using the device
> name for the structure, such as struct an8855, and ddata for the instance.
> 
> Finally, are there typos in the function names an855_mdio_register and
> an855_regmap_phy_reset_page, missing the second 8 in AN8855?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

-- 
Lee Jones