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

[netdev-bot+sashiko@kernel.org]

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()).

> +
> +	/* 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?

> +
>  	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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909142056.1433875-1-vineeth.karumanchi%40amd.com