Re: [PATCH RFC v3 06/12] phy: freescale: s32g: Add SerDes subsystem PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] mm: Use-After-Free in s32g_serdes_pcs_create() via lockless drvdata access
- [High] driver: Race condition exposing PHY provider before hardware initialization
- [High] pm: Sleep in atomic context via noirq sleep callbacks
- [Medium] driver: Infinite probe deferral on unconfigured XPCS instance
- [Medium] pm: Missing resource cleanup on resume error path
--

commit 179aba95589906304ef5b4ba6f8b771475155fa2
Author: Vincent Guittot <vincent.guittot@linaro.org>

    phy: freescale: s32g: Add SerDes subsystem PHY

    The S32G SoC family includes two SerDes subsystems, each made of one
    PCIe controller, two XPCS and a shared 2-lane combo PHY. The lanes can
    be configured to carry PCIe and/or SGMII.
    
    Add the SerDes subsystem PHY driver: the PCIe PHY, the shared combo-PHY
    bring-up, and the setup of the XPCS instances added by the previous
    patch.

> diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> new file mode 100644
> index 0000000000000..e81878137f2e1
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> @@ -0,0 +1,1057 @@

[ ... ]

> +static int s32g_serdes_init(struct s32g_serdes *serdes)
> +{
> +	struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> +	u32 reg0;
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(ctrl->nclks, ctrl->clks);
> +	if (ret) {
> +		dev_err(serdes->dev, "Failed to enable SerDes clocks\n");
> +		return ret;
> +	}

[ ... ]

> +	/* Wait for the selection of working mode (as per the manual specs) */
> +	usleep_range(100, 110);

[ ... ]

> +static int s32g_serdes_create_pcie_phy(struct s32g_serdes *serdes,
> +				       struct device_node *child_node)
> +{
> +	struct phy_provider *phy_provider;
> +	struct device *dev = serdes->dev;
> +	struct phy *phy;
> +
> +	phy = devm_phy_create(dev, child_node, &serdes_pcie_ops);
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
> +
> +	phy_set_drvdata(phy, serdes);
> +
> +	phy->attrs.mode = PHY_MODE_PCIE;
> +	serdes->pcie.phy = phy;
> +
> +	phy_provider = devm_of_phy_provider_register(&phy->dev,
> +						     s32g_serdes_phy_xlate);

[Severity: High]
Does this expose the PHY provider before the hardware is initialized? 

s32g_serdes_parse_lanes() registers the PHY provider via 
devm_of_phy_provider_register(), making it visible to consumers. The hardware 
clocks and resets aren't initialized until s32g_serdes_init() is called later 
in s32g_serdes_probe(). 

If a consumer probes concurrently and calls phy_power_on(), could it access 
unclocked registers and cause a synchronous external abort?

> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	return 0;
> +}

[ ... ]

> +static int s32g_serdes_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = s32g_serdes_parse_lanes(dev, serdes);
> +	if (ret)
> +		return ret;

[ ... ]

> +	ret = s32g_serdes_init(serdes);
> +	if (ret)
> +		return ret;

[ ... ]

> +static int s32g_serdes_resume(struct device *device)
> +{
> +	struct s32g_serdes *serdes = dev_get_drvdata(device);
> +	struct s32g_pcie_ctrl *pcie = &serdes->pcie;
> +	int ret;
> +
> +	ret = s32g_serdes_init(serdes);
> +	if (ret) {
> +		dev_err(device, "Failed to initialize\n");
> +		return ret;
> +	}
> +
> +	/* Restore PCIe phy power */
> +	if (pcie->powered_on) {
> +		ret = s32g_pcie_phy_power_on(serdes);
> +		if (ret)
> +			dev_err(device, "Failed to power-on PCIe phy\n");
> +	}
> +
> +	return ret;

[Severity: Medium]
Does this leak clock resources on the error path?

If s32g_pcie_phy_power_on() fails, the function returns an error without 
undoing the hardware initialization performed earlier in s32g_serdes_init(). 
The PM core will mark the device as having failed to resume, but the leaked 
clock enables might prevent the power domain from fully shutting down.

> +}

[ ... ]

> +struct phylink_pcs *s32g_serdes_pcs_create(struct device *dev,
> +					   struct device_node *np)
> +{

[ ... ]

> +	serdes = platform_get_drvdata(pdev);
> +	if (!serdes) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}

[Severity: High]
Can this race with driver unbind leading to a use-after-free?

This function is exposed to consumer drivers and reads drvdata locklessly. 
If the SerDes driver is unbound concurrently via sysfs, 
dev_set_drvdata(dev, NULL) is called and the devm_kzalloc'd serdes 
structure is freed by devres. 

If platform_get_drvdata() reads the pointer just before the unbind, won't 
accessing serdes->xpcs.phys[instance] below dereference freed memory?

> +
> +	if (!serdes->xpcs.phys[instance]) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);

[Severity: Medium]
Will this cause an infinite probe deferral loop if the instance is unconfigured?

If the SerDes has probed successfully (meaning drvdata is non-NULL) but 
intentionally did not populate serdes->xpcs.phys[instance] due to lane 
configuration mismatch, this instance will never be populated. Would it be 
better to return an error rather than -EPROBE_DEFER in this case?

> +	}

[ ... ]

> +/* noirq, like the PCIe controller using this PHY */
> +static const struct dev_pm_ops s32g_serdes_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(s32g_serdes_suspend,
> +				  s32g_serdes_resume)

[Severity: High]
Does this call sleepable functions in an atomic context?

The driver assigns s32g_serdes_resume() to the .resume_noirq callback, which 
executes with hardware interrupts disabled. However, s32g_serdes_resume() calls 
s32g_serdes_init(), which invokes clk_bulk_prepare_enable() (takes a mutex) 
and usleep_range() (sleeps via hrtimers).

> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919-s32g_serdes-v3-0-9d68868c1e89@oss.nxp.com?part=6