Re: [PATCH net-next v11 3/6] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] pm: PD_PIPE powers off on suspend breaking MAC WoL
- [Medium] pm: missing pm_runtime_disable() on driver unbind
- [Medium] pm: missing pm_runtime_disable() on driver unbind
- [Low] pm: unbalanced pm_runtime_enable() due to missing pm_runtime_disable()
--

commit 10f6065a84e9613f4b783ffc88353c7a176a3fcd
Author: Coia Prant <coiaprant@gmail.com>

    net: pcs: xpcs: add Rockchip RK3568 platform glue driver
    
    The RK3568 SoC integrates a Synopsys DesignWare XPCS that is accessed
    via APB3 memory-mapped registers.  This driver provides the glue logic
    to make the XPCS accessible to the generic pcs-xpcs core.

    Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 59, CRU_CLKSEL_CON29)
    Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)

> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> index 0000000000000..698a43b753341
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
[ ... ]
> +static void xpcs_rk_exit_clk(void *data)
> +{
> +	struct dw_xpcs_rk *pxpcs = data;
> +	struct device *dev = &pxpcs->pdev->dev;
> +
> +	pm_runtime_force_suspend(dev);
> +	clk_disable_unprepare(pxpcs->eee_clk);
> +}
> +
> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +	unsigned long rate;
> +	u64 mult;
> +	int ret;
[ ... ]
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_enable(dev);
> +
> +	ret = devm_add_action_or_reset(dev, xpcs_rk_exit_clk, pxpcs);

[Severity: Medium]
Will this leave the pm runtime disable_depth unbalanced when the driver is
unbound?

The driver calls pm_runtime_enable() in xpcs_rk_init_clk(), but the teardown
action xpcs_rk_exit_clk() doesn't call pm_runtime_disable() to unwind it.
Leaving runtime PM enabled after the device is unbound breaks the runtime PM
core's tracking and can trigger an "Unbalanced pm_runtime_enable!" warning
if the device is probed again.

Should devm_pm_runtime_enable() be used, or pm_runtime_disable() be
added to the cleanup function?

[ ... ]
> +static int xpcs_rk_system_suspend(struct device *dev)
> +{
> +	/*
> +	 * Keep the PD_PIPE power domain on during system suspend.
> +	 *
> +	 * PD_PIPE is shared with SATA/PCIe and would be powered down by
> +	 * genpd once all its consumers are suspended, killing the SerDes
> +	 * and breaking MAC WoL.  Mark the XPCS as part of the wakeup path
> +	 * so genpd keeps the domain on.  Unconditional because the XPCS
> +	 * core has no callback to convey the MAC WoL state.
> +	 */
> +	device_set_wakeup_path(dev);

[Severity: Medium]
Does genpd actually keep the power domain on when device_set_wakeup_path()
is called here?

The commit message notes this relies on the Rockchip power domain driver
setting GENPD_FLAG_ACTIVE_WAKEUP on PD_PIPE. However, looking at
drivers/pmdomain/rockchip/pm-domains.c:

rk3568_pm_domains[] {
	...
	[RK3568_PD_PIPE] = DOMAIN_RK3568("pipe", BIT(8), BIT(11), false, false),
	...
}

The wakeup parameter is false, which means GENPD_FLAG_ACTIVE_WAKEUP is not
set. Because this flag is missing, won't genpd ignore the wakeup path and
power off the domain anyway, breaking MAC WoL?

> +	return 0;
> +}

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