Re: [PATCH v4 3/3] phy: airoha: Add support for Airoha AN7583 USB PHY

[Christian Marangi <ansuelsmth@gmail.com>]

On Tue, Sep 01, 2026 at 12:53:43PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Failure to propagate PHY lifecycle calls to the underlying SerDes PHY
> - [High] USB PHY instances are inaccessible due to incorrect PHY provider registration
> - [Medium] Use of uninitialized stack variable due to missing regmap_read return check
> - [Medium] Lockdep warning due to recursive locking of phy->mutex in set_mode callbacks
> - [Low] Missing select REGMAP_MMIO in Kconfig leading to link-time errors
> - [Low] Missing explicit inclusion of <linux/delay.h>
> --
> 
> commit 78bb8c49d95aeffce30cf8303d7ac8de16f47e67
> Author: Christian Marangi <ansuelsmth@gmail.com>
> 
>     phy: airoha: Add support for Airoha AN7583 USB PHY
>     
>     Add support for USB PHY for Airoha AN7583 SoC, this share some U2 init
>     from AN7581 but use a totally different init phase for PLL and
>     calibration for U3. Also on AN7583 there is a single USB PHY that init 2
>     U2 port and one U3 port. (the USB controller expose 2 hub, one with 2
>     usb 2.0 port and one with one 3.0 port)
>     
>     A second USB PHY is used for PCIe Gen2 controller and follow the same
>     calibration and setup logic of USB.
> 
> > diff --git a/drivers/phy/airoha/Kconfig b/drivers/phy/airoha/Kconfig
> > new file mode 100644
> > index 0000000000000..13e49ff947e3e
> > --- /dev/null
> > +++ b/drivers/phy/airoha/Kconfig
> > @@ -0,0 +1,11 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config PHY_AIROHA_AN7583_USB
> > +	tristate "Airoha AN7583 USB PHY Driver"
> > +	depends on ARCH_AIROHA || COMPILE_TEST
> > +	depends on OF
> > +	select GENERIC_PHY
> 
> [Severity: Low]
> Is a select for REGMAP_MMIO missing here? If COMPILE_TEST is enabled and no
> other driver implicitly selects REGMAP_MMIO, couldn't this cause a link-time
> error due to an undefined reference to __devm_regmap_init_mmio_clk?
> 
> > +	help
> > +	  Say 'Y' here to add support for Airoha AN7583 USB PHY driver.
> 
> > diff --git a/drivers/phy/airoha/phy-an7583-usb.c b/drivers/phy/airoha/phy-an7583-usb.c
> > new file mode 100644
> > index 0000000000000..46b5f09582cfd
> > --- /dev/null
> > +++ b/drivers/phy/airoha/phy-an7583-usb.c
> > @@ -0,0 +1,2023 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Author: Christian Marangi <ansuelsmth@gmail.com>
> > + */
> > +
> > +#include <dt-bindings/phy/phy.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/math.h>
> > +#include <linux/module.h>
> > +#include <linux/phy.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> 
> [Severity: Low]
> Does this code rely on linux/delay.h for mdelay() and usleep_range() without
> including it? This could lead to compilation failures on architectures or
> configurations where it isn't implicitly included by other headers.
> 
> [ ... ]
> 
> > +static bool an7583_usb_phy_u3_kband_is_calibrated(struct an7583_usb_phy_priv *priv)
> > +{
> > +	u32 val, res;
> 
> [ ... ]
> 
> > +	regmap_read(priv->regs[AIROHA_USB_PHY_PMA],
> > +		    AIROHA_USB_PMA_INTF_STS_9, &val);
> > +	res = FIELD_GET(AIROHA_USB_PMA_ADDR_INTF_STS_PLL_VCOCAL, val) << 4;
> 
> [Severity: Medium]
> If the regmap_read() fails, wouldn't the uninitialized stack variable val be
> accessed here? Could this result in undefined behavior and potentially bypass
> the KBand calibration loop?
> 
> [ ... ]
> 
> > +static int an7583_usb_phy_init(struct phy *phy)
> > +{
> > +	struct an7583_usb_phy_instance *instance = phy_get_drvdata(phy);
> 
> [Severity: High]
> Does this initialization omit a call to phy_init(instance->serdes_phy)? Since
> the wrapper PHY intercepts the init call, failing to forward it might leave
> the physical SerDes lines uninitialized and the PCIe and USB 3.0 interfaces
> non-functional.
> 
> > +
> > +	switch (instance->type) {
> > +	case PHY_TYPE_USB2:
> 
> [ ... ]
> 
> > +static int an7583_usb_phy_power_on(struct phy *phy)
> > +{
> > +	struct an7583_usb_phy_instance *instance = phy_get_drvdata(phy);
> 
> [Severity: High]
> Similarly, does this fail to call phy_power_on(instance->serdes_phy)? This
> seems to leave the physical SerDes disabled when a consumer attempts to power
> on the PHY.
> 
> > +
> > +	switch (instance->type) {
> > +	case PHY_TYPE_USB2:
> 
> [ ... ]
> 
> > +static int an7583_usb_phy_u3_set_mode(struct phy *phy, enum phy_mode mode,
> > +				      int submode)
> > +{
> > +	struct an7583_usb_phy_instance *instance = phy_get_drvdata(phy);
> > +	int ret;
> > +
> > +	switch (mode) {
> 
> [ ... ]
> 
> > +	}
> > +
> > +	ret = phy_set_mode_ext(instance->serdes_phy, mode, submode);
> 
> [Severity: Medium]
> Can this nested call to phy_set_mode_ext() on the child SerDes PHY cause a
> lockdep warning?
> 
> Since the parent PHY's mutex is already held by the framework, and all PHY
> mutexes are initialized with the same lockdep key, this might trigger a
> possible recursive locking deadlock warning. This pattern also appears in
> an7583_usb_phy_pcie_set_mode().
> 
> [ ... ]
> 
> > +static struct phy *an7583_usb_phy_xlate(struct device *dev,
> > +					const struct of_phandle_args *args)
> > +{
> > +	struct an7583_usb_phy_priv *priv = dev_get_drvdata(dev);
> > +	const struct an7583_usb_phy_match_data *data;
> > +	struct device_node *phy_np = args->np;
> > +	struct phy *phy = NULL;
> > +	int type, index;
> > +
> > +	data = of_device_get_match_data(dev);
> > +
> > +	if (args->args_count != 1) {
> > +		dev_err(dev, "invalid number of cells in 'phy' property\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	for (index = 0; index < data->max_instance; index++) {
> > +		if (!priv->phys[index])
> > +			continue;
> > +
> > +		if (phy_np == priv->phys[index]->dev.of_node) {
> > +			phy = priv->phys[index];
> > +			break;
> > +		}
> > +	}
> 
> [ ... ]
> 
> > +static int an7583_usb_phy_probe(struct platform_device *pdev)
> > +{
> 
> [ ... ]
> 
> > +	phy_provider = devm_of_phy_provider_register(dev, an7583_usb_phy_xlate);
> 
> [Severity: High]
> Does this incorrectly isolate the subnodes from the PHY core?
> 
> By registering the provider with devm_of_phy_provider_register(dev, ...)
> instead of using a variant that checks the children tree, the PHY core might
> fail to traverse to the child nodes when looking up the PHY.
> 
> Furthermore, in an7583_usb_phy_xlate(), comparing the parent node (args->np)
> against the child node stored in priv->phys[index]->dev.of_node would
> guarantee a mismatch. Would this make the USB PHY instances completely
> inaccessible to any consumer?
> 

Is AI getting drunk? Aside from the mostly unreadable message, this is
totally non-sense and out of complete speculation.

The scenario pointed out by AI is in the case where a consumer reference
the provider node in DT but this was never suggested and actually an
implementation error of the user writing the device tree.

The Documentation example instruct for USB nodes to put the phy cell in the
child node, NEVER in the provider node. Any kind of phandle will fail as
the correct cell property won't be found. So I'm not really understanding
the error pointed out here.

-- 
	Ansuel

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy