Re: [PATCH 2/2] clk: en7523: Add support for selecting the Serdes port in SCU

[sashiko-bot@kernel.org]

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

New issues:
- [High] The PHY set_mode callback incorrectly rejects USB host and OTG modes, breaking USB host functionality.

Pre-existing issues:
- [High] Missing spinlock in reset operations allows concurrent reset assertions to corrupt shared reset control registers.
- [High] Probe failure leaves unregistered clocks in the clock core, causing a Use-After-Free and memory leak when devres frees the clock structures.
--

--- Patch [2]: [PATCH 2/2] clk: en7523: Add support for selecting the Serdes port in SCU ---
commit e81bbf2efd265f6528ea23e2f2c71e904963fc17
Author: Christian Marangi <ansuelsmth@gmail.com>

clk: en7523: Add support for selecting the Serdes port in SCU

In the SCU register for clock and reset, there are also some register to
select the Serdes port mode. The Airoha AN7581 SoC have 4 different Serdes
that can switch between PCIe, USB or Ethernet mode.

Add a simple PHY provider that expose the .set_mode OP to toggle the
requested mode for the Serdes port.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 1ab0e2eca5d32..183cf7fe4bda6 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c

[ ... ]

> @@ -830,12 +861,179 @@ static int en7581_reset_register(struct device *dev, void __iomem *base,
>  	return devm_reset_controller_register(dev, &rst_data->rcdev);
>  }
>  
> +static int en7581_serdes_phy_set_mode(struct phy *phy, enum phy_mode mode,
> +				      int submode)
> +{

[ ... ]

> +	case AIROHA_SCU_SERDES_USB1:
> +		reg = REG_NP_SCU_SSR3;
> +		mask = REG_SSUSB_HSGMII_SEL_MASK;
> +
> +		if (mode != PHY_MODE_ETHERNET && mode != PHY_MODE_USB_DEVICE &&
> +		    mode != PHY_MODE_USB_DEVICE_SS)
> +			return -EINVAL;

[Severity: High]
Does this validation block USB host and OTG operations for the USB1 port?

If a USB controller driver calls phy_set_mode(phy, PHY_MODE_USB_HOST) or
PHY_MODE_USB_OTG during initialization, it appears this check will reject
it and return -EINVAL, bypassing the register write and leaving the Serdes
lanes uninitialized.

> +
> +		if (mode == PHY_MODE_ETHERNET)
> +			sel = REG_SSUSB_HSGMII_SEL_HSGMII;
> +		else
> +			sel = REG_SSUSB_HSGMII_SEL_USB;
> +
> +		break;
> +	case AIROHA_SCU_SERDES_USB2:
> +		reg = REG_NP_SCU_SSTR;
> +		mask = REG_USB_PCIE_SEL_MASK;
> +
> +		if (mode != PHY_MODE_PCIE && mode != PHY_MODE_USB_DEVICE &&
> +		    mode != PHY_MODE_USB_DEVICE_SS)
> +			return -EINVAL;

[Severity: High]
Similarly, does this logic improperly reject PHY_MODE_USB_HOST and
PHY_MODE_USB_OTG for the USB2 Serdes port?

[ ... ]

> @@ -853,9 +1053,12 @@ static int en7581_clk_hw_init(struct platform_device *pdev,
>  	val = readl(base + REG_NP_SCU_PCIC);
>  	writel(val | 3, base + REG_NP_SCU_PCIC);
>  
> -	return en7581_reset_register(&pdev->dev, base, en7581_rst_map,
> -				     ARRAY_SIZE(en7581_rst_map),
> -				     en7581_rst_ofs);
> +	ret = en7581_reset_register(&pdev->dev, base, en7581_rst_map,
> +				    ARRAY_SIZE(en7581_rst_map), en7581_rst_ofs);

[Severity: High]
This is a pre-existing issue, but does the lack of a spinlock in the reset
operations allow concurrent reset assertions to corrupt shared registers?

If multiple driver probes (e.g., PCIe, USB, Ethernet) call
en7523_reset_update() or en7523_pci_prepare() concurrently, both functions
perform unlocked read-modify-write sequences on REG_RESET_CONTROL1:

en7523_reset_update() {
    ...
    val = readl(addr);
    if (assert)
        val |= BIT(id % RST_NR_PER_BANK);
    else
        val &= ~BIT(id % RST_NR_PER_BANK);
    writel(val, addr);
}

Could a race condition here cause one IP block to be wedged in reset because
its update was lost?

> +	if (ret)
> +		return ret;
> +
> +	return en7581_serdes_phy_register(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but does this new error path expose a memory
leak and use-after-free of struct en_clk_gate in the clock core?

The clocks are allocated with devres but registered with unmanaged APIs:

en7523_register_pcie_clk() {
    ...
    cg = devm_kzalloc(dev, sizeof(*cg), GFP_KERNEL);
    ...
    err = clk_hw_register(dev, hw);
}

If en7581_serdes_phy_register() returns an error here, the probe fails and
devres will automatically free cg. However, because clk_hw_register() was
used, the clock remains registered in CCF.

Would this leave a dangling pointer in the clock core that causes a crash
on subsequent access to /sys/kernel/debug/clk?

>  }
>  
>  static enum en_hir get_hw_id(void __iomem *np_base)

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