Re: [PATCH v7 4/6] 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:
- [Critical] The `reg` and `mask` variables are left uninitialized when processing the `AIROHA_SCU_SERDES_PCIE2` case, leading to arbitrary memory/hardware corruption.
- [High] Concurrent read-modify-write operations on shared hardware registers lack spinlock protection, leading to race conditions and potential lost updates.
- [High] USB PHY instances unconditionally reject `PHY_MODE_USB_HOST` and `PHY_MODE_USB_OTG`, breaking USB host functionality.
--

commit 714fd2f968cbdacf2f17f7ead8459ffad971d307
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: clk: en7523: Add support for selecting the Serdes port in SCU

This commit adds support for selecting the Serdes port in the SCU register 
for clock and reset. It implements a simple PHY provider with a set_mode 
operation to toggle the requested mode (PCIe, USB, or Ethernet) for the 
Airoha AN7581 SoC Serdes ports.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 1ab0e2eca5d32..58ec071388a4d 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
[ ... ]
> @@ -830,12 +858,173 @@ static int en7581_reset_register(struct device *dev, void __iomem *base,
> +static int en7581_serdes_phy_set_mode(struct phy *phy, enum phy_mode mode,
> +				      int submode)
> +{
> +	struct en_serdes_phy_instance *instance = phy_get_drvdata(phy);
> +	struct en_clk_priv *priv = dev_get_drvdata(phy->dev.parent);
> +	u32 reg, mask, sel, val;
> +
> +	switch (instance->serdes_port) {
> +	case AIROHA_SCU_SERDES_PCIE1:
> +		reg = REG_NP_SCU_SSTR;
> +		mask = REG_PCIE_XSI0_SEL_MASK;
[ ... ]
> +		} else {
> +			sel = REG_PCIE_XSI0_SEL_PCIE;
> +		}
> +
> +		break;
> +	case AIROHA_SCU_SERDES_PCIE2:

[Severity: Critical]
Could this cause random memory or hardware corruption?

It looks like reg and mask are not initialized for AIROHA_SCU_SERDES_PCIE2
here. When the switch statement finishes, the function uses these
uninitialized stack variables for the readl and writel operations.

> +		if (mode != PHY_MODE_ETHERNET && mode != PHY_MODE_PCIE)
> +			return -EINVAL;
[ ... ]
> +		} else {
> +			sel = REG_PCIE_XSI1_SEL_PCIE;
> +		}
> +
> +		break;
> +	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]
Will this break USB host or dual-role functionality?

USB controllers operating in host or OTG modes often request
PHY_MODE_USB_HOST or PHY_MODE_USB_OTG. Since this PHY only acts as a
multiplexer, should it accept all valid USB modes when routing lines to the
USB controller?

> +
> +		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)
> +			sel = REG_USB_PCIE_SEL_PCIE;
> +		else
> +			sel = REG_USB_PCIE_SEL_USB;
> +
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	val = readl(priv->base + reg);
> +	val &= ~mask;
> +	val |= sel;
> +	writel(val, priv->base + reg);

[Severity: High]
Can this read-modify-write sequence race if multiple Serdes ports are
configured concurrently?

The PHY subsystem uses a per-PHY instance mutex, so the set_mode callbacks
for different Serdes ports could execute in parallel (for example, during
asynchronous probing). Because they share hardware registers like
REG_NP_SCU_SSTR, concurrent operations could lead to lost updates and route
the Serdes to the wrong subsystem.

Would it be safer to protect these accesses with a spinlock added to struct
en_clk_priv?

> +
> +	return 0;
> +}

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