Re: [PATCH net-next v2 2/4] net: macb: Move TC capability and PM checks to macb_setup_tc()

["Karumanchi, Vineeth" <vineeth@amd.com>]

Hi netdev sashiko,

On 9/10/2026 8:14 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 0 (1)
> 
> - [High] macb_setup_tc() only samples the runtime-PM state with
>   pm_runtime_suspended() (macb_main.c:4509) and never acquires a…
> - [Medium] macb_setup_tc()
>   (drivers/net/ethernet/cadence/macb_main.c:4494-4519) now runs the…
> 
> Pre-existing issues:
> - [Low] The relocated gate in macb_setup_tc() tests the immutable
>   capability mask (`netdev->hw_features & NETIF_F_HW_TC`,…
> 
> --
> 
> net: macb: Move TC capability and PM checks to macb_setup_tc()
> 
> This moves the NETIF_F_HW_TC capability test and the runtime PM suspend
> guard out of macb_setup_taprio() and into the shared macb_setup_tc()
> entry point so future TC qdisc types do not have to repeat them.
> 
> A few questions about the relocated checks below.
> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 8223ad81215b..ff7e02d3fab8 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -4476,18 +4476,8 @@ static void macb_taprio_destroy(struct net_device *netdev)
>>  static int macb_setup_taprio(struct net_device *netdev,
>>  			     struct tc_taprio_qopt_offload *taprio)
>>  {
>> -	struct macb *bp = netdev_priv(netdev);
>>  	int err = 0;
>>  
>> -	if (unlikely(!(netdev->hw_features & NETIF_F_HW_TC)))
>> -		return -EOPNOTSUPP;
>> -
>> -	/* Check if Device is in runtime suspend */
>> -	if (unlikely(pm_runtime_suspended(&bp->pdev->dev))) {
>> -		netdev_err(netdev, "Device is in runtime suspend\n");
>> -		return -EOPNOTSUPP;
>> -	}
>> -
>>  	switch (taprio->cmd) {
>>  	case TAPRIO_CMD_REPLACE:
>>  		err = macb_taprio_setup_replace(netdev, taprio);
> 
> [ ... ]
> 
>> @@ -4505,9 +4495,22 @@ static int macb_setup_taprio(struct net_device *netdev,
>>  static int macb_setup_tc(struct net_device *netdev, enum tc_setup_type type,
>>  			 void *type_data)
>>  {
>> +	struct macb *bp;
>> +
>>  	if (!netdev || !type_data)
>>  		return -EINVAL;
>>  
>> +	bp = netdev_priv(netdev);
>> +
>> +	if (unlikely(!(netdev->hw_features & NETIF_F_HW_TC)))
>> +		return -EOPNOTSUPP;
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, since the test is only being
> relocated here, but should this gate look at netdev->features rather than
> netdev->hw_features?
> 
> macb_probe() sets:
> 
> 	if (bp->caps & MACB_CAPS_TC)
> 		netdev->hw_features |= NETIF_F_HW_TC;
> 
> 	netdev->features = netdev->hw_features;
> 
> hw_features is never touched again, so this condition reduces to a
> MACB_CAPS_TC capability test. macb_set_features() only reacts to
> NETIF_F_HW_CSUM, NETIF_F_RXCSUM and NETIF_F_NTUPLE and returns 0 for a
> NETIF_F_HW_TC change, so after
> 
> 	ethtool -K ethX hw-tc-offload off
> 
> does a subsequent taprio replace still get accepted and programmed into
> the ENST registers, and does an already programmed configuration stay
> active?  The usual spelling for this gate elsewhere is
> dev->features & NETIF_F_HW_TC (see tc_can_offload()).

OK, as this patch is currently relocating the check, I will fix it as a
separate follow-up patch.

> 
>> +
>> +	/* Check if Device is in runtime suspend */
>> +	if (unlikely(pm_runtime_suspended(&bp->pdev->dev))) {
>> +		netdev_err(netdev, "Device is in runtime suspend\n");
>> +		return -EOPNOTSUPP;
>> +	}
> 
> [Severity: High]
> Since this is now presented as the precondition for all ndo_setup_tc
> paths, is a plain status read enough to keep the register clocks on for
> the duration of the offload?
> 
> pm_runtime_suspended() only reports runtime_status == RPM_SUSPENDED, so it
> returns false both when the device is RPM_ACTIVE with a pending
> autosuspend and when it is RPM_SUSPENDING. macb_close() drops the
> reference asynchronously:
> 
> 	pm_runtime_put(&bp->pdev->dev);
> 
> which arms the MACB_PM_TIMEOUT (100 ms) autosuspend. Can the timer then
> expire right after the check passes, so that
> 
> macb_runtime_suspend()
>     macb_clks_disable(bp->pclk, bp->hclk, bp->tx_clk, bp->rx_clk, bp->tsu_clk);
> 
> runs while macb_taprio_setup_replace() or macb_taprio_destroy() is still
> writing ENST_CONTROL and the per-queue ENST_START_TIME / ENST_ON_TIME /
> ENST_OFF_TIME registers?  macb_taprio_setup_replace() does sleeping work
> (kcalloc(GFP_KERNEL), phylink_ethtool_ksettings_get()) before it reaches
> the register block, so the window is not small, and bp->lock does not
> exclude the PM core.
> 
> Every other register-touching path in this driver brackets access with a
> usage reference instead, for example macb_mdio_read()/macb_mdio_write(),
> macb_open() and at91ether_open() use
> pm_runtime_resume_and_get() ... pm_runtime_put_autosuspend(). Would
> replacing the check with that pairing here be preferable to relocating
> it, given the MQPRIO and CBS handlers added later in the series inherit
> this precondition too?
> 

yes, will fix in next version.

>> +
>>  	switch (type) {
>>  	case TC_SETUP_QDISC_TAPRIO:
>>  		return macb_setup_taprio(netdev, type_data);
> 
> [Severity: Medium]
> The guard above now runs before the switch, so it also fires for
> tc_setup_type values this driver never handled. Is the netdev_err() on
> those paths intended?
> 
> net/sched/cls_api.c:tcf_block_offload_cmd() calls ndo_setup_tc
> unconditionally with non-NULL type_data:
> 
> 	if (dev->netdev_ops->ndo_setup_tc) {
> 		int err;
> 
> 		err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK, &bo);
> 		if (err < 0) {
> 			if (err != -EOPNOTSUPP)
> 				NL_SET_ERR_MSG(extack, "Driver ndo_setup_tc failed");
> 
> and -EOPNOTSUPP is a normal outcome there (tcf_block_offload_bind() falls
> back to no_offload_dev_inc() and returns 0).
> 
> So on MACB_CAPS_TC hardware whose interface is down and has autosuspended,
> does every clsact/ingress qdisc add and delete now print
> "Device is in runtime suspend" at error level while the command itself
> succeeds?  The message is not rate limited, so repeating the operation
> would repeat the log line. At this commit the same applies to
> qdisc_offload_query_caps():
> 
> 	if (ops->ndo_setup_tc)
> 		ops->ndo_setup_tc(dev, TC_QUERY_CAPS, &base);
> 
> The TC_QUERY_CAPS case is later moved ahead of the guard by the following
> patch in the series ("net: macb: Add MQPRIO qdisc hardware offload
> support"), but TC_SETUP_BLOCK still reaches it at the end of the series.
> 
> Before this patch these types went straight to the default case and
> returned -EOPNOTSUPP with no PM inspection and no log output. Could the
> commit message mention this, since it is described as centralizing
> existing preconditions?
> 

yes, will fix in next version.

-- 
🙏 Vineeth