[PATCH net v2] net: stmmac: hold runtime PM reference in setup_tc

[PATCH net v2] net: stmmac: hold runtime PM reference in setup_tc

[Lorenzo Bianconi]

The qdisc offload callbacks invoked by stmmac_setup_tc() program
MTL/MAC registers, but they can be reached while the interface is down,
when stmmac_release() has dropped the runtime PM usage counter and the
device may be suspended with its clocks gated. Accessing the registers
in that state can trigger a bus error.

Hold a runtime PM reference while configuring the register-touching
qdisc offloads (mqprio, cbs and taprio) so the device is active, and its
clocks enabled, whenever the MTL/MAC registers are programmed.

The TC block callback stmmac_setup_tc_block_cb() programs the MTL/MAC
registers as well, but it runs asynchronously from stmmac_setup_tc(),
outside the runtime PM reference held there. Hold a runtime PM reference
for the whole stmmac_setup_tc_block_cb() call as well, covering the
cls_u32/cls_flower setup and the queue enable/disable accesses.

No reference is held for the TC_SETUP_BLOCK bookkeeping itself, the
TC_QUERY_CAPS query or the tc-etf path, since none of them touch the
registers synchronously. In particular the block bind/unbind must reach
flow_block_cb_setup_simple() even when the device is suspended, so the
driver never leaves a stale flow_block_cb on its block list.

Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
Fixes: 4dbbe8dde848 ("net: stmmac: Add support for U32 TC filter using Flexible RX Parser")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
Changes in v2:
- Return -EOPNOTSUPP in stmmac_setup_tc_block_cb() for unsupported TC
  blocks.
- Link to v1: https://lore.kernel.org/r/20260824-stmmac-setup-tc-enable-pm-v1-1-45172d241a4b@oss.qualcomm.com
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++++++++++++++---
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index efa35cfecc4f..90753d5af6d6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6393,9 +6393,13 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 				    void *cb_priv)
 {
 	struct stmmac_priv *priv = cb_priv;
-	int ret = -EOPNOTSUPP;
+	int ret;
 
 	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
+		return -EOPNOTSUPP;
+
+	ret = pm_runtime_resume_and_get(priv->device);
+	if (ret < 0)
 		return ret;
 
 	__stmmac_disable_all_queues(priv);
@@ -6408,10 +6412,13 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 		ret = stmmac_tc_setup_cls(priv, priv, type_data);
 		break;
 	default:
+		ret = -EOPNOTSUPP;
 		break;
 	}
 
 	stmmac_enable_all_queues(priv);
+	pm_runtime_put(priv->device);
+
 	return ret;
 }
 
@@ -6421,26 +6428,46 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
 			   void *type_data)
 {
 	struct stmmac_priv *priv = netdev_priv(ndev);
+	int ret;
 
 	switch (type) {
 	case TC_QUERY_CAPS:
 		return stmmac_tc_query_caps(priv, priv, type_data);
 	case TC_SETUP_QDISC_MQPRIO:
-		return stmmac_tc_setup_mqprio(priv, priv, type_data);
+		ret = pm_runtime_resume_and_get(priv->device);
+		if (ret < 0)
+			return ret;
+
+		ret = stmmac_tc_setup_mqprio(priv, priv, type_data);
+		break;
 	case TC_SETUP_BLOCK:
 		return flow_block_cb_setup_simple(type_data,
 						  &stmmac_block_cb_list,
 						  stmmac_setup_tc_block_cb,
 						  priv, priv, true);
 	case TC_SETUP_QDISC_CBS:
-		return stmmac_tc_setup_cbs(priv, priv, type_data);
+		ret = pm_runtime_resume_and_get(priv->device);
+		if (ret < 0)
+			return ret;
+
+		ret = stmmac_tc_setup_cbs(priv, priv, type_data);
+		break;
 	case TC_SETUP_QDISC_TAPRIO:
-		return stmmac_tc_setup_taprio(priv, priv, type_data);
+		ret = pm_runtime_resume_and_get(priv->device);
+		if (ret < 0)
+			return ret;
+
+		ret = stmmac_tc_setup_taprio(priv, priv, type_data);
+		break;
 	case TC_SETUP_QDISC_ETF:
 		return stmmac_tc_setup_etf(priv, priv, type_data);
 	default:
 		return -EOPNOTSUPP;
 	}
+
+	pm_runtime_put(priv->device);
+
+	return ret;
 }
 
 static u16 stmmac_select_queue(struct net_device *dev, struct sk_buff *skb,

---
base-commit: f967455fb2a5a2079b9eb5823e9ccf359174bf9f
change-id: 20260824-stmmac-setup-tc-enable-pm-149aa563d797

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Re: [PATCH net v2] net: stmmac: hold runtime PM reference in setup_tc

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 5942 bytes --]

> The qdisc offload callbacks invoked by stmmac_setup_tc() program
> MTL/MAC registers, but they can be reached while the interface is down,
> when stmmac_release() has dropped the runtime PM usage counter and the
> device may be suspended with its clocks gated. Accessing the registers
> in that state can trigger a bus error.
> 
> Hold a runtime PM reference while configuring the register-touching
> qdisc offloads (mqprio, cbs and taprio) so the device is active, and its
> clocks enabled, whenever the MTL/MAC registers are programmed.
> 
> The TC block callback stmmac_setup_tc_block_cb() programs the MTL/MAC
> registers as well, but it runs asynchronously from stmmac_setup_tc(),
> outside the runtime PM reference held there. Hold a runtime PM reference
> for the whole stmmac_setup_tc_block_cb() call as well, covering the
> cls_u32/cls_flower setup and the queue enable/disable accesses.
> 
> No reference is held for the TC_SETUP_BLOCK bookkeeping itself, the
> TC_QUERY_CAPS query or the tc-etf path, since none of them touch the
> registers synchronously. In particular the block bind/unbind must reach
> flow_block_cb_setup_simple() even when the device is suspended, so the
> driver never leaves a stale flow_block_cb on its block list.
> 
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Fixes: 4dbbe8dde848 ("net: stmmac: Add support for U32 TC filter using Flexible RX Parser")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Commenting on sashiko's report:
https://sashiko.dev/#/patchset/20260827-stmmac-setup-tc-enable-pm-v2-1-a9b8a5948f41%40oss.qualcomm.com

> ---
> Changes in v2:
> - Return -EOPNOTSUPP in stmmac_setup_tc_block_cb() for unsupported TC
>   blocks.
> - Link to v1: https://lore.kernel.org/r/20260824-stmmac-setup-tc-enable-pm-v1-1-45172d241a4b@oss.qualcomm.com
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index efa35cfecc4f..90753d5af6d6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6393,9 +6393,13 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  				    void *cb_priv)
>  {
>  	struct stmmac_priv *priv = cb_priv;
> -	int ret = -EOPNOTSUPP;
> +	int ret;
>  
>  	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
> +		return -EOPNOTSUPP;
> +
> +	ret = pm_runtime_resume_and_get(priv->device);
> +	if (ret < 0)
>  		return ret;
>  
>  	__stmmac_disable_all_queues(priv);
> @@ -6408,10 +6412,13 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  		ret = stmmac_tc_setup_cls(priv, priv, type_data);
>  		break;
>  	default:
> +		ret = -EOPNOTSUPP;
>  		break;
>  	}
>  
>  	stmmac_enable_all_queues(priv);
> +	pm_runtime_put(priv->device);
> +
>  	return ret;
>  }
>  
> @@ -6421,26 +6428,46 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>  			   void *type_data)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> +	int ret;
>  
>  	switch (type) {
>  	case TC_QUERY_CAPS:
>  		return stmmac_tc_query_caps(priv, priv, type_data);
>  	case TC_SETUP_QDISC_MQPRIO:
> -		return stmmac_tc_setup_mqprio(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_mqprio(priv, priv, type_data);
> +		break;
>  	case TC_SETUP_BLOCK:
>  		return flow_block_cb_setup_simple(type_data,
>  						  &stmmac_block_cb_list,
>  						  stmmac_setup_tc_block_cb,
>  						  priv, priv, true);
>  	case TC_SETUP_QDISC_CBS:
> -		return stmmac_tc_setup_cbs(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_cbs(priv, priv, type_data);
> +		break;
>  	case TC_SETUP_QDISC_TAPRIO:
> -		return stmmac_tc_setup_taprio(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_taprio(priv, priv, type_data);
> +		break;
>  	case TC_SETUP_QDISC_ETF:
>  		return stmmac_tc_setup_etf(priv, priv, type_data);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> +
> +	pm_runtime_put(priv->device);
> +

- Does configuring TC offloads while the interface is down result in silent
  data loss when the interface is later brought up?
  By wrapping these accesses in pm_runtime_resume_and_get(), the driver now
  successfully programs the MAC/MTL registers while offline. However, when
  the interface is brought up, stmmac_open() calls stmmac_hw_setup(), which
  performs a hardware reset via stmmac_reset().
  This hardware reset wipes all MAC and MTL registers back to default values.
  Since the driver lacks a mechanism to restore the TAPRIO, MQPRIO, or RXP
  configurations during stmmac_open() or system resume, the TC offloads are
  silently wiped from the hardware, leaving the software TC state completely
  diverged from the hardware state.
  - This issues have not been introduced by this patch and they should be
    addressed with a dedicated patches. Moreover, EST issue has been already
    addressed in this patch:
    https://lore.kernel.org/netdev/20260829-stmmac-est-reapply-after-open-v2-1-5e5ccb185e92@oss.qualcomm.com/

Regards,
Lorenzo

> +	return ret;
>  }
>  
>  static u16 stmmac_select_queue(struct net_device *dev, struct sk_buff *skb,
> 
> ---
> base-commit: f967455fb2a5a2079b9eb5823e9ccf359174bf9f
> change-id: 20260824-stmmac-setup-tc-enable-pm-149aa563d797
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net v2] net: stmmac: hold runtime PM reference in setup_tc

[Maxime Chevallier]

Hi,

On 8/27/26 09:12, Lorenzo Bianconi wrote:
> The qdisc offload callbacks invoked by stmmac_setup_tc() program
> MTL/MAC registers, but they can be reached while the interface is down,
> when stmmac_release() has dropped the runtime PM usage counter and the
> device may be suspended with its clocks gated. Accessing the registers
> in that state can trigger a bus error.
> 
> Hold a runtime PM reference while configuring the register-touching
> qdisc offloads (mqprio, cbs and taprio) so the device is active, and its
> clocks enabled, whenever the MTL/MAC registers are programmed.
> 
> The TC block callback stmmac_setup_tc_block_cb() programs the MTL/MAC
> registers as well, but it runs asynchronously from stmmac_setup_tc(),
> outside the runtime PM reference held there. Hold a runtime PM reference
> for the whole stmmac_setup_tc_block_cb() call as well, covering the
> cls_u32/cls_flower setup and the queue enable/disable accesses.
> 
> No reference is held for the TC_SETUP_BLOCK bookkeeping itself, the
> TC_QUERY_CAPS query or the tc-etf path, since none of them touch the
> registers synchronously. In particular the block bind/unbind must reach
> flow_block_cb_setup_simple() even when the device is suspended, so the
> driver never leaves a stale flow_block_cb on its block list.
> 
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Fixes: 4dbbe8dde848 ("net: stmmac: Add support for U32 TC filter using Flexible RX Parser")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> Changes in v2:
> - Return -EOPNOTSUPP in stmmac_setup_tc_block_cb() for unsupported TC
>   blocks.
> - Link to v1: https://lore.kernel.org/r/20260824-stmmac-setup-tc-enable-pm-v1-1-45172d241a4b@oss.qualcomm.com
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index efa35cfecc4f..90753d5af6d6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6393,9 +6393,13 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  				    void *cb_priv)
>  {
>  	struct stmmac_priv *priv = cb_priv;
> -	int ret = -EOPNOTSUPP;
> +	int ret;
>  
>  	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
> +		return -EOPNOTSUPP;
> +
> +	ret = pm_runtime_resume_and_get(priv->device);
> +	if (ret < 0)
>  		return ret;
>  
>  	__stmmac_disable_all_queues(priv);
> @@ -6408,10 +6412,13 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  		ret = stmmac_tc_setup_cls(priv, priv, type_data);
>  		break;
>  	default:
> +		ret = -EOPNOTSUPP;
>  		break;
>  	}
>  
>  	stmmac_enable_all_queues(priv);
> +	pm_runtime_put(priv->device);
> +
>  	return ret;
>  }
>  
> @@ -6421,26 +6428,46 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>  			   void *type_data)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> +	int ret;
>  
>  	switch (type) {
>  	case TC_QUERY_CAPS:
>  		return stmmac_tc_query_caps(priv, priv, type_data);
>  	case TC_SETUP_QDISC_MQPRIO:
> -		return stmmac_tc_setup_mqprio(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_mqprio(priv, priv, type_data);
> +		break;
>  	case TC_SETUP_BLOCK:
>  		return flow_block_cb_setup_simple(type_data,
>  						  &stmmac_block_cb_list,
>  						  stmmac_setup_tc_block_cb,
>  						  priv, priv, true);
>  	case TC_SETUP_QDISC_CBS:
> -		return stmmac_tc_setup_cbs(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_cbs(priv, priv, type_data);
> +		break;
>  	case TC_SETUP_QDISC_TAPRIO:
> -		return stmmac_tc_setup_taprio(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_taprio(priv, priv, type_data);
> +		break;
>  	case TC_SETUP_QDISC_ETF:
>  		return stmmac_tc_setup_etf(priv, priv, type_data);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> +
> +	pm_runtime_put(priv->device);
> +
> +	return ret;
>  }
>  
>  static u16 stmmac_select_queue(struct net_device *dev, struct sk_buff *skb,
> 
> ---
> base-commit: f967455fb2a5a2079b9eb5823e9ccf359174bf9f
> change-id: 20260824-stmmac-setup-tc-enable-pm-149aa563d797
> 
> Best regards,

Re: [PATCH net v2] net: stmmac: hold runtime PM reference in setup_tc

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 10652 bytes --]

> The qdisc offload callbacks invoked by stmmac_setup_tc() program
> MTL/MAC registers, but they can be reached while the interface is down,
> when stmmac_release() has dropped the runtime PM usage counter and the
> device may be suspended with its clocks gated. Accessing the registers
> in that state can trigger a bus error.
> 
> Hold a runtime PM reference while configuring the register-touching
> qdisc offloads (mqprio, cbs and taprio) so the device is active, and its
> clocks enabled, whenever the MTL/MAC registers are programmed.
> 
> The TC block callback stmmac_setup_tc_block_cb() programs the MTL/MAC
> registers as well, but it runs asynchronously from stmmac_setup_tc(),
> outside the runtime PM reference held there. Hold a runtime PM reference
> for the whole stmmac_setup_tc_block_cb() call as well, covering the
> cls_u32/cls_flower setup and the queue enable/disable accesses.
> 
> No reference is held for the TC_SETUP_BLOCK bookkeeping itself, the
> TC_QUERY_CAPS query or the tc-etf path, since none of them touch the
> registers synchronously. In particular the block bind/unbind must reach
> flow_block_cb_setup_simple() even when the device is suspended, so the
> driver never leaves a stale flow_block_cb on its block list.
> 
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Fixes: 4dbbe8dde848 ("net: stmmac: Add support for U32 TC filter using Flexible RX Parser")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> ---
> Changes in v2:
> - Return -EOPNOTSUPP in stmmac_setup_tc_block_cb() for unsupported TC
>   blocks.
> - Link to v1: https://lore.kernel.org/r/20260824-stmmac-setup-tc-enable-pm-v1-1-45172d241a4b@oss.qualcomm.com
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 4 deletions(-)

commenting on sashiko's report:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260827-stmmac-setup-tc-enable-pm-v2-1-a9b8a5948f41%40oss.qualcomm.com

> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index efa35cfecc4f..90753d5af6d6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6393,9 +6393,13 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  				    void *cb_priv)
>  {
>  	struct stmmac_priv *priv = cb_priv;
> -	int ret = -EOPNOTSUPP;
> +	int ret;
>  
>  	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
> +		return -EOPNOTSUPP;
> +
> +	ret = pm_runtime_resume_and_get(priv->device);
> +	if (ret < 0)
>  		return ret;

- Can this new early return leak entries from priv->tc_entries and
  priv->flow_entries?
  When pm_runtime_resume_and_get() fails (for example clk_prepare_enable()
  failing inside stmmac_bus_clks_config(), reached via
  stmmac_runtime_resume()), a TC_CLSU32_DELETE_KNODE or FLOW_CLS_DESTROY
  command never reaches tc_delete_knode()/tc_del_flow() in stmmac_tc.c, which
  are the only places that clear entry->in_use in the bounded pools allocated
  by tc_init()/tc_rfs_init().
  The tc core does not retry the destroy - fl_hw_destroy_filter() returns
  void and ignores the result of tc_setup_cb_destroy():
  net/sched/cls_flower.c:fl_hw_destroy_filter() {
  	...
  	tc_setup_cb_destroy(block, tp, TC_SETUP_CLSFLOWER, &cls_flower, false,
  			    &f->flags, &f->in_hw_count, rtnl_held);
  	...
  }
  The same applies to the new early returns in stmmac_setup_tc() for
  TAPRIO_CMD_DESTROY and for CBS teardown, where the qdisc layer frees its
  own state regardless of the returned error and priv->est / CBS programming
  stays as it was. Before this change the delete path always executed.
  Would it be better for teardown commands either to run unconditionally, or
  to update the software bookkeeping even when the register programming has
  to be skipped?
  - I think this issue is not introduced by this patch
    (tc_cls_can_offload_and_chain0() alreaday bails out early in case of
    error) and it needs to be fixed with a dedicated patch.

>  
>  	__stmmac_disable_all_queues(priv);
> @@ -6408,10 +6412,13 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  		ret = stmmac_tc_setup_cls(priv, priv, type_data);
>  		break;
>  	default:
> +		ret = -EOPNOTSUPP;
>  		break;
>  	}
>  
>  	stmmac_enable_all_queues(priv);

- The commit message says this callback "can be reached while the interface is
  down". If that is the case, can __stmmac_disable_all_queues() hang here?
  napi_disable() is not idempotent. __stmmac_disable_all_queues() calls it for
  every configured channel without any state check:
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__stmmac_disable_all_queues() {
  	...
  		if (queue < rx_queues_cnt)
  			napi_disable(&ch->rx_napi);
  		if (queue < tx_queues_cnt)
  			napi_disable(&ch->tx_napi);
  	...
  }
  and napi_disable_locked() waits without a timeout:
  net/core/dev.c:napi_disable_locked() {
  	...
  		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
  			usleep_range(20, 200);
  			val = READ_ONCE(n->state);
  		}
  	...
  }
  Nothing clears SCHED/NPSVC for a NAPI instance that is already disabled.
  The instances are created at probe by stmmac_napi_add() and only removed at
  remove/queue reconfigure, and __stmmac_release() calls
  stmmac_disable_all_queues() without re-enabling them, so they carry those
  bits both before the first stmmac_open() and after ifdown.
  - This issue is not introduced by this patch and it needs to be fixed with a
    dedicated patch

> +	pm_runtime_put(priv->device);
> +
>  	return ret;
>  }
>  
> @@ -6421,26 +6428,46 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>  			   void *type_data)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> +	int ret;
>  
>  	switch (type) {
>  	case TC_QUERY_CAPS:
>  		return stmmac_tc_query_caps(priv, priv, type_data);
>  	case TC_SETUP_QDISC_MQPRIO:
> -		return stmmac_tc_setup_mqprio(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_mqprio(priv, priv, type_data);
> +		break;
>  	case TC_SETUP_BLOCK:
>  		return flow_block_cb_setup_simple(type_data,
>  						  &stmmac_block_cb_list,
>  						  stmmac_setup_tc_block_cb,
>  						  priv, priv, true);
>  	case TC_SETUP_QDISC_CBS:
> -		return stmmac_tc_setup_cbs(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_cbs(priv, priv, type_data);
> +		break;
>  	case TC_SETUP_QDISC_TAPRIO:
> -		return stmmac_tc_setup_taprio(priv, priv, type_data);
> +		ret = pm_runtime_resume_and_get(priv->device);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = stmmac_tc_setup_taprio(priv, priv, type_data);
> +		break;

- The commit message states the reference makes the device "active, and its
  clocks enabled, whenever the MTL/MAC registers are programmed". Is that
  accurate for the taprio path?
  stmmac_runtime_resume() goes through stmmac_bus_clks_config(), which only
  enables stmmac_clk, pclk and the optional platform hook:
  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:stmmac_bus_clks_config() {
  	...
  		ret = clk_prepare_enable(plat_dat->stmmac_clk);
  		...
  		ret = clk_prepare_enable(plat_dat->pclk);
  		...
  		if (plat_dat->clks_config)
  			ret = plat_dat->clks_config(plat_dat->bsp_priv, enabled);
  	...
  }
  priv->plat->clk_ptp_ref is not in that list. It is tied to open/close:
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_release_ptp() {
  	stmmac_ptp_unregister(priv);
  	clk_disable_unprepare(priv->plat->clk_ptp_ref);
  }
  - This issue is already fixed by the following patch:
    https://lore.kernel.org/netdev/20260829-stmmac-est-reapply-after-open-v2-1-5e5ccb185e92@oss.qualcomm.com/

- By resuming the device rather than rejecting or deferring, this now lets the
  EST and Flexible RX Parser programming succeed while the interface is down.
  Does that programming survive the next ifup?
  The next stmmac_open() does a core software reset:
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_init_dma_engine() {
  	ret = stmmac_reset(priv);
  	if (ret) {
  		netdev_err(priv->dev, "Failed to reset the dma\n");
  		return ret;
  	}
  	...
  }
  which clears the MAC/MTL registers. stmmac_est_configure() is only called
  from stmmac_tc.c and stmmac_ptp.c, and stmmac_rxp_config() only from
  tc_config_knode()/tc_delete_knode() in stmmac_tc.c, so neither the EST gate
  list nor the FRP table is re-applied from the open path. CBS by contrast is
  re-applied:
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_hw_setup() {
  	...
  	/* Configure CBS in AVB TX queues */
  	if (tx_queues_count > 1)
  		stmmac_configure_cbs(priv);
  	...
  }
  Meanwhile priv->est->enable/gcl, priv->tc_entries and priv->flow_entries
  keep saying the offload is installed. So after
    ip link set X down
    tc qdisc replace dev X root taprio ... flags 0x2
    (or tc filter add dev X ingress ... u32/flower skip_sw)
    ip link set X up
  does the hardware end up running with default MTL/FRP configuration while
  userspace and the driver both believe the schedule or filter is offloaded,
  with later incremental add/delete operations computing updates from a table
  that no longer matches the device?
  The wipe-at-open behaviour itself predates this patch, but powering the
  device up and programming anyway makes it the expected outcome of the path
  this patch enables. Would rejecting with -ENETDOWN, deferring, or
  re-applying EST/FRP from stmmac_hw_setup() be a better fit?
  - Similar to EST counterpart, this issue has not been introduced by this
    patch and it needs to be fixed with a dedicated patch.

Regards,
Lorenzo

>  	case TC_SETUP_QDISC_ETF:
>  		return stmmac_tc_setup_etf(priv, priv, type_data);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> +
> +	pm_runtime_put(priv->device);
> +
> +	return ret;
>  }
>  
>  static u16 stmmac_select_queue(struct net_device *dev, struct sk_buff *skb,
> 
> ---
> base-commit: f967455fb2a5a2079b9eb5823e9ccf359174bf9f
> change-id: 20260824-stmmac-setup-tc-enable-pm-149aa563d797
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net v2] net: stmmac: hold runtime PM reference in setup_tc

[Jakub Kicinski]

On Mon, 31 Aug 2026 16:51:50 +0200 Lorenzo Bianconi wrote:
>   - This issue is not introduced by this patch and it needs to be fixed with a
>     dedicated patch

Looks like the driver has multiple bugs in handling of TC callbacks
while down. Please fix all of them in one series, otherwise we may
end up changing approach.

Can you also confirm that the device programmed while down retains
all the necessary configuration thru the open path, and everything
works as expected?
-- 
pw-bot: cr

Re: [PATCH net v2] net: stmmac: hold runtime PM reference in setup_tc

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

> On Mon, 31 Aug 2026 16:51:50 +0200 Lorenzo Bianconi wrote:
> >   - This issue is not introduced by this patch and it needs to be fixed with a
> >     dedicated patch
> 
> Looks like the driver has multiple bugs in handling of TC callbacks
> while down. Please fix all of them in one series, otherwise we may
> end up changing approach.

ack, I will do.

> 
> Can you also confirm that the device programmed while down retains
> all the necessary configuration thru the open path, and everything
> works as expected?

there are some leftover issues I am looking at:
- https://lore.kernel.org/netdev/20260831-stmmac_tc_cls32_reconfigure-v1-1-21cb459e64ae@oss.qualcomm.com/
- https://lore.kernel.org/netdev/20260829-stmmac-est-reapply-after-open-v2-1-5e5ccb185e92@oss.qualcomm.com/

Regards,
Lorenzo

> -- 
> pw-bot: cr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]