Re: [PATCH net-next 8/9] net: stmmac: qcom-ethqos: add per-platform NOC clock voting

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 2 (1)

- [High] Incorrect power sequencing order in ethqos_clks_config() error
  path drops OPP performance state before disabling dependent clock.
- [Medium] Missing PM_OPP Kconfig dependency and undisclosed OPP/DT
  requirement: qcom_ethqos_init_noc_clks()…
- [Medium] Probe-time OPP/performance-state vote is never withdrawn on
  later probe failures.
- [Low] ETHQOS_MAX_NOC_CLKS is introduced but never enforced:
  qcom_ethqos_init_noc_clks() (dwmac-qcom-ethqos.c:809-815) copies…
- [Low] Concern that the added NOC-clock/OPP infrastructure (struct
  ethqos_noc_clk_cfg, the new driver-data and runtime fields,…

Pre-existing issues:
- [Low] Concern that
  Documentation/devicetree/bindings/net/qcom,ethqos.yaml documents…

--

net: stmmac: qcom-ethqos: add per-platform NOC clock voting

Adds struct ethqos_noc_clk_cfg plus driver-data and runtime fields so each
compatible can declare its own set of System NOC clocks with per-clock
rates.  The clocks are acquired in probe via qcom_ethqos_init_noc_clks()
and enabled/disabled next to the link clock in ethqos_clks_config(), with
the OPP framework used to carry the VDD_CX performance state.

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 4f9bef662c579..a31541506a4c9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -7,6 +7,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/phy.h>
>  #include <linux/phy/phy.h>
> +#include <linux/pm_opp.h>
>  
>  #include "stmmac.h"
>  #include "stmmac_platform.h"
> @@ -84,11 +85,18 @@
>  
>  #define SGMII_10M_RX_CLK_DVDR			0x31
>  
> +#define ETHQOS_MAX_NOC_CLKS			3
> +
>  struct ethqos_emac_por {
>  	unsigned int offset;
>  	unsigned int value;
>  };
>  
> +struct ethqos_noc_clk_cfg {
> +	const char *id;
> +	unsigned long rate;
> +};
> +
>  struct ethqos_emac_driver_data {
>  	const struct ethqos_emac_por *rgmii_por;
>  	unsigned int num_rgmii_por;
> @@ -98,6 +106,8 @@ struct ethqos_emac_driver_data {
>  	const char *link_clk_name;
>  	struct dwmac4_addrs dwmac4_addrs;
>  	bool needs_sgmii_loopback;
> +	const struct ethqos_noc_clk_cfg *noc_clk_cfg;
> +	unsigned int num_noc_clks;
>  };
>  
>  struct qcom_ethqos {
> @@ -112,6 +122,10 @@ struct qcom_ethqos {
>  	bool rgmii_config_loopback_en;
>  	bool has_emac_ge_3;
>  	bool needs_sgmii_loopback;
> +
> +	struct clk_bulk_data noc_clks[ETHQOS_MAX_NOC_CLKS];
> +	unsigned long noc_clk_rates[ETHQOS_MAX_NOC_CLKS];
> +	int num_noc_clks;
>  };
>  
>  static u32 rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
> @@ -691,15 +705,51 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  static int ethqos_clks_config(void *priv, bool enabled)
>  {
>  	struct qcom_ethqos *ethqos = priv;
> +	unsigned int i;
>  	int ret = 0;
>  
>  	if (enabled) {
> +		if (ethqos->num_noc_clks) {

[Severity: Low]
This isn't a bug, but as submitted every new block here and in
qcom_ethqos_init_noc_clks() is gated on ethqos->num_noc_clks, and no
ethqos_emac_driver_data entry in this commit sets .noc_clk_cfg or
.num_noc_clks, so none of the new code runs yet.  The consumer arrives
later in the series with "net: stmmac: qcom-ethqos: add Shikra EMAC
support", which adds shikra_noc_clks[] and
{ .compatible = "qcom,shikra-ethqos", .data = &shikra_data }, so the
infrastructure does get exercised by the end of the series.  Was the split
intentional?

> +			ret = dev_pm_opp_set_rate(&ethqos->pdev->dev,
> +						  ethqos->noc_clk_rates[0]);
> +			if (ret) {
> +				dev_err(&ethqos->pdev->dev,
> +					"NOC OPP rate set failed: %d\n", ret);
> +				return ret;
> +			}
> +

[Severity: High]
The unwind order here looks inverted with respect to the disable
path added a few lines below.  When clk_bulk_prepare_enable() on
ethqos->noc_clks fails, this drops the performance state with
dev_pm_opp_set_rate(&ethqos->pdev->dev, 0) first and only then calls
clk_disable_unprepare(ethqos->link_clk), so between the two calls the
link clock is still running while the VDD_CX vote taken by the earlier
dev_pm_opp_set_rate(dev, ethqos->noc_clk_rates[0]) has already been
released.  The !enabled branch of the same function does it the other
way around - clk_bulk_disable_unprepare(), clk_disable_unprepare(link_clk),
then dev_pm_opp_set_rate(dev, 0) - which is the sequence I would expect
if the clocks require the vote to be held while they are ungated.

The same concern applies to the clk_prepare_enable(ethqos->link_clk)
failure just above, although there nothing else is enabled yet so the
ordering is moot.

Is the ordering in the error path deliberate, or should these two calls
be swapped so that every unwind mirrors the disable sequence?  If the
hardware genuinely does not care about the relative order of the vote
and the link clock, it would still be worth keeping the error path and
the disable path identical so the intended sequencing is obvious to the
next reader.

[ ... ]

> @@ -736,6 +791,46 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
>  	netdev_dbg(priv->dev, "PTP rate %lu\n", plat_dat->clk_ptp_rate);
>  }
>  
> +/* Some SoCs gate NOC access behind dedicated clocks. Acquire them here
> + * so ethqos_clks_config() can enable/disable them at runtime. The OPP
> + * table is used to propagate the required VDD_CX performance state via
> + * dev_pm_opp_set_rate().
> + */
> +static int qcom_ethqos_init_noc_clks(struct qcom_ethqos *ethqos,
> +				     const struct ethqos_emac_driver_data *data)
> +{
> +	struct device *dev = &ethqos->pdev->dev;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!data->num_noc_clks)
> +		return 0;
> +
> +	for (i = 0; i < data->num_noc_clks; i++) {
> +		ethqos->noc_clks[i].id = data->noc_clk_cfg[i].id;
> +		ethqos->noc_clk_rates[i] = data->noc_clk_cfg[i].rate;
> +	}
> +	ethqos->num_noc_clks = data->num_noc_clks;

[Severity: Low]
Should this loop check data->num_noc_clks against ETHQOS_MAX_NOC_CLKS?
The macro is added by this patch but never enforced anywhere, while the
destinations are fixed-size members of the devm_kzalloc'd struct:

	struct clk_bulk_data noc_clks[ETHQOS_MAX_NOC_CLKS];
	unsigned long noc_clk_rates[ETHQOS_MAX_NOC_CLKS];

Today the only table added by the series (shikra_noc_clks[]) has exactly
three entries, so nothing overflows noc_clks[] or noc_clk_rates[] right
now.  A future driver-data entry with four clocks would write past both
arrays, and a table that sets .num_noc_clks without .noc_clk_cfg would
dereference NULL at data->noc_clk_cfg[i].  Would a static_assert() on each
table, or an explicit

	if (data->num_noc_clks > ETHQOS_MAX_NOC_CLKS)
		return -EINVAL;

be worth adding here?

> +
> +	ret = devm_clk_bulk_get(dev, ethqos->num_noc_clks, ethqos->noc_clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get NOC clocks\n");
> +
> +	ret = devm_pm_opp_set_clkname(dev, data->noc_clk_cfg[0].id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set OPP clock name\n");
> +
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add OPP table\n");

[Severity: Medium]
Does this need a Kconfig dependency on PM_OPP?

With CONFIG_PM_OPP=n, include/linux/pm_opp.h provides stubs that return
-EOPNOTSUPP for devm_pm_opp_set_clkname() (via devm_pm_opp_set_config()),
devm_pm_opp_of_add_table() and dev_pm_opp_set_rate(), and each call here
is turned into a probe failure by dev_err_probe().

config DWMAC_QCOM_ETHQOS in drivers/net/ethernet/stmicro/stmmac/Kconfig
currently has only:

	depends on OF && (ARCH_QCOM || COMPILE_TEST)

and config PM_OPP in drivers/opp/Kconfig is a promptless "bool", i.e.
select-only, so PM_OPP=n is a legal configuration for this driver.  In
that build, any compatible declaring NOC clocks would fail to probe and
no netdev would be created.  Should DWMAC_QCOM_ETHQOS select PM_OPP, or
should these calls be made conditional?

Also, devm_pm_opp_of_add_table() makes an operating-points-v2 node a hard
requirement for such compatibles.  The commit message says:

  "The clocks are acquired during probe and enabled/disabled alongside
   the existing link clock in ethqos_clks_config()."

Could the new mandatory OPP table / device tree requirement be mentioned
there too?

> +
> +	ret = dev_pm_opp_set_rate(dev, data->noc_clk_cfg[0].rate);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set initial NOC OPP rate\n");
> +
> +	return 0;
> +}
> +
>  static int qcom_ethqos_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -795,6 +890,12 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ethqos->has_emac_ge_3 = data->has_emac_ge_3;
>  	ethqos->needs_sgmii_loopback = data->needs_sgmii_loopback;
>  
> +	if (data->num_noc_clks) {
> +		ret = qcom_ethqos_init_noc_clks(ethqos, data);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");
>  	if (IS_ERR(ethqos->link_clk))
>  		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),

[Severity: Medium]
Can the performance-state vote taken by the dev_pm_opp_set_rate() at the
end of qcom_ethqos_init_noc_clks() leak if probe fails right after?

The only code that drops it is ethqos_clks_config(priv, false) with its
dev_pm_opp_set_rate(dev, 0), and that path is installed only later in
probe:

qcom_ethqos_probe()
	qcom_ethqos_init_noc_clks()	/* dev_pm_opp_set_rate(dev, rate) */
	devm_clk_get(dev, "rgmii")	/* can return -EPROBE_DEFER */
	ethqos_clks_config(ethqos, true)
	devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos)

If devm_clk_get() defers or ethqos_clks_config(true) fails, probe returns
before ethqos_clks_disable is registered.  Devres teardown does not appear
to compensate: _opp_table_kref_release() and _opp_clear_config() in
drivers/opp/core.c do not call _set_opp_level(dev, NULL) or
_set_required_opps(..., NULL), and on the genpd side only the default
pstate is dropped:

drivers/pmdomain/core.c:genpd_dev_pm_detach() {
	...
	/* Drop the default performance state */
	if (dev_gpd_data(dev)->default_pstate) {
		dev_pm_genpd_set_performance_state(dev, 0);
		dev_gpd_data(dev)->default_pstate = 0;
	}
	...
}

so the VDD_CX vote requested through the OPP would stay in place for a
device that never bound, and genpd_remove_device() unlinks the device
without re-evaluating genpd->performance_state.

Is this initial dev_pm_opp_set_rate() needed at all?
ethqos_clks_config(ethqos, true) applies the same rate a few lines later
and _set_opp() early-returns when the OPP is unchanged, so dropping it (or
pairing it with its own devm action) would close the window.

[Severity: Low]
This isn't a bug introduced by this patch, but at this point in the series
Documentation/devicetree/bindings/net/qcom,ethqos.yaml already documents
qcom,shikra-ethqos with the axi-noc / pcie-tile-axi-noc clocks and a
required operating-points-v2 property, while qcom_ethqos_match[] has no
matching entry and no driver data setting .noc_clk_cfg / .num_noc_clks.
The pair becomes consistent with the later commit "net: stmmac:
qcom-ethqos: add Shikra EMAC support", which adds both the compatible and
shikra_data, so this is only a transient mismatch inside the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com