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

[sashiko-bot@kernel.org]

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?

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

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