Message-ID: <20260112180005.4144056-1-kuba@kernel.org>
Date: Mon, 12 Jan 2026 10:00:05 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: bartosz.golaszewski@....qualcomm.com
Cc: Jakub Kicinski <kuba@...nel.org>,
	jbrunet@...libre.com,
	linux-stm32@...md-mailman.stormreply.com,
	devicetree@...r.kernel.org,
	neil.armstrong@...aro.org,
	unicorn_wang@...look.com,
	andrew+netdev@...n.ch,
	vkoul@...nel.org,
	peppe.cavallaro@...com,
	jernej.skrabec@...il.com,
	samin.guo@...rfivetech.com,
	david.wu@...k-chips.com,
	christophe.roullier@...s.st.com,
	linux-amlogic@...ts.infradead.org,
	linux-riscv@...ts.infradead.org,
	martin.blumenstingl@...glemail.com,
	robh@...nel.org,
	linux-imx@....com,
	andersson@...nel.org,
	joabreu@...opsys.com,
	s32@....com,
	swathi.ks@...sung.com,
	konradybcio@...nel.org,
	krzk+dt@...nel.org,
	keguang.zhang@...il.com,
	kernel@...gutronix.de,
	romain.gantois@...tlin.com,
	brgl@...nel.org,
	sophgo@...ts.linux.dev,
	netdev@...r.kernel.org,
	xiaoning.wang@....com,
	imx@...ts.linux.dev,
	dfustini@...storrent.com,
	davem@...emloft.net,
	jan.petrous@....nxp.com,
	magnus.damm@...il.com,
	lizhi2@...incomputing.com,
	linux-sunxi@...ts.linux.dev,
	linux-mips@...r.kernel.org,
	vineetha.g.jaya.kumaran@...el.com,
	matthew.gerlach@...era.com,
	wefu@...hat.com,
	geert+renesas@...der.be,
	edumazet@...gle.com,
	linux-kernel@...r.kernel.org,
	Frank.Li@....com,
	conor+dt@...nel.org,
	mripard@...nel.org,
	linux-arm-kernel@...ts.infradead.org,
	kernel@...il.dk,
	linux-renesas-soc@...r.kernel.org,
	alexandre.torgue@...s.st.com,
	inochiama@...il.com,
	minda.chen@...rfivetech.com,
	s.hauer@...gutronix.de,
	nobuhiro.iwamatsu.x90@...l.toshiba,
	prabhakar.mahadev-lad.rj@...renesas.com,
	fustini@...nel.org,
	richardcochran@...il.com,
	pabeni@...hat.com,
	shawnguo@...nel.org,
	wens@...nel.org,
	heiko@...ech.de,
	guoren@...nel.org,
	weishangjuan@...incomputing.com,
	mcoquelin.stm32@...il.com,
	khilman@...libre.com,
	festevam@...il.com,
	liangshuang@...incomputing.com,
	krzysztof.kozlowski@....qualcomm.com,
	linux-arm-msm@...r.kernel.org,
	konrad.dybcio@....qualcomm.com,
	linux-rockchip@...ts.infradead.org,
	samuel@...lland.org
Subject: Re: [RESEND,net-next,v6,7/7] net: stmmac: qcom-ethqos: add support for sa8255p

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least pausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: qcom-ethqos: add support for sa8255p

This commit adds support for the SA8255P SoC which uses power domains
managed via SCMI firmware, rather than the traditional clock/phy-based
power management used by other variants.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 8ba57bba3f2e..54f8ef3cfd7d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> +static void ethqos_disable_serdes(void *data)
> +{
> +	struct qcom_ethqos *ethqos = data;
> +
> +	qcom_ethqos_domain_on(ethqos, ETHQOS_PD_SERDES);
                          ^^

Should this be qcom_ethqos_domain_off() instead of qcom_ethqos_domain_on()?

This function is registered as a cleanup action via devm_add_action_or_reset()
to undo the qcom_ethqos_domain_on(ETHQOS_PD_SERDES) call during probe. Using
domain_on here would increment the PM runtime usage counter again rather than
decrementing it. The serdes power domain would never be properly turned off
on driver removal.

Compare with qcom_ethqos_pd_serdes_powerdown() which correctly uses
qcom_ethqos_domain_off().

> +}

[ ... ]

> +static int qcom_ethqos_pd_init(struct device *dev, void *priv)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +	int ret;
> +
> +	/*
> +	 * Enable functional clock to prevent DMA reset after timeout due
> +	 * to no PHY clock being enabled after the hardware block has been
> +	 * power cycled. The actual configuration will be adjusted once
> +	 * ethqos_fix_mac_speed() is called.
> +	 */
> +	ethqos_set_func_clk_en(ethqos);
> +
> +	ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_CORE);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_MDIO);
> +	if (ret) {
> +		qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

>  	if (pm_data && pm_data->use_domains) {
>  		ethqos->set_serdes_speed = ethqos_set_serdes_speed_pd;
>
> +		ret = devm_pm_domain_attach_list(dev, &pm_data->pd,
> +						 &ethqos->pd.pd_list);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to attach power domains\n");
> +
> +		plat_dat->clks_config = ethqos_pd_clks_config;
> +		plat_dat->serdes_powerup = qcom_ethqos_pd_serdes_powerup;
> +		plat_dat->serdes_powerdown = qcom_ethqos_pd_serdes_powerdown;
> +		plat_dat->exit = qcom_ethqos_pd_exit;
> +		plat_dat->init = qcom_ethqos_pd_init;
                             ^^^^^^^^^^^^^^^^^^^^^^^

> +		plat_dat->clk_ptp_rate = pm_data->clk_ptp_rate;
> +
> +		ret = qcom_ethqos_pd_init(dev, ethqos);
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is qcom_ethqos_pd_init() being called twice?

It is assigned to plat_dat->init above, and then called directly here. Later,
devm_stmmac_pltfr_probe() -> stmmac_pltfr_probe() -> stmmac_dvr_probe() will
call plat_dat->init() again (see stmmac_main.c stmmac_dvr_probe function).

Each call to qcom_ethqos_pd_init() increments the PM runtime usage counter
for ETHQOS_PD_CORE and ETHQOS_PD_MDIO via pm_runtime_resume_and_get(). With
two calls, the refcount would be 2 for each domain, but qcom_ethqos_pd_exit()
only decrements once per domain. This would leave the power domains with a
dangling reference.

Perhaps the direct call to qcom_ethqos_pd_init() here should be removed, and
let stmmac_dvr_probe() invoke it through the plat_dat->init callback?

> +		if (ret)
> +			return ret;

[ ... ]

Powered by blists - more mailing lists