Re: [PATCH net 2/4] net: bcmgenet: do not tear the datapath down twice after a failed resume

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With this patch alone, bcmgenet_resume() still drops the
  priv->clk reference in out_clk_disable when bcmgenet_init_dma() fails,…
- [Medium] With this patch alone, after a failed resume the netdev stays
  detached.
- [Medium] Before calling bcmgenet_init_dma(), bcmgenet_resume() always
  calls bcmgenet_hfb_restore(), which goes through bcmgenet_hfb_clear().
- [Low] The patch fixes two bugs from different origins but carries only
  one Fixes tag.
- [Low] The commit message and the new comment in bcmgenet_resume() (`/*
  A link event can enable the receiver after a failed resume, so check…

--

net: bcmgenet: do not tear the datapath down twice after a failed resume

This patch sets priv->rx_cbs and priv->tx_cbs to NULL when they are freed.
bcmgenet_close() and bcmgenet_suspend() then check priv->tx_cbs and skip the
datapath teardown after a failed bcmgenet_resume(). The WoL resume path also
stops reusing rings that are gone when it sees CMD_RX_EN set.

> Fixes: b6e978e50444 ("net: bcmgenet: add suspend/resume callbacks")

[Severity: Low]

This isn't a bug, but should the WoL hunk have a second Fixes tag?

The priv->tx_cbs check added to the CMD_RX_EN test in bcmgenet_resume()
guards the "Successfully exited WoL, just resume data flows" branch, which
reuses the rings. That branch came from 254f3239dd07 ("net: bcmgenet: revise
suspend/resume"), not b6e978e50444. The previous patch in this series, "net:
bcmgenet: stop the PHY when a WoL resume rebuilds the datapath", already uses
Fixes: 254f3239dd07 for the same block.

A Fixes: 254f3239dd07 tag would record where the WoL hunk comes from. It
would also show that the hunk only applies to trees that contain that commit.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 308716cddc077..fe3338ba81980 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -3493,7 +3498,9 @@ static int bcmgenet_close(struct net_device *dev)
>  
>  	netif_dbg(priv, ifdown, dev, "bcmgenet_close\n");
>  
> -	bcmgenet_netif_stop(dev, false);
> +	/* A failed resume has torn the datapath down already */
> +	if (priv->tx_cbs)
> +		bcmgenet_netif_stop(dev, false);
>  
>  	/* Really kill the PHY state machine and disconnect from it */
>  	phy_disconnect(dev->phydev);

[Severity: Medium]

Now that this check lets close go further, can priv->clk be released twice
after a failed resume?

When bcmgenet_init_dma() fails, bcmgenet_resume() still goes through
out_clk_disable, and the interface stays netif_running():

out_clk_disable:
	if (priv->internal_phy)
		bcmgenet_power_down(priv, GENET_POWER_PASSIVE);
	clk_disable_unprepare(priv->clk);
	return ret;

Before this patch, the next ifdown hung in napi_disable(). Now
bcmgenet_close() gets past the check and reaches this code again:

	if (priv->internal_phy)
		ret = bcmgenet_power_down(priv, GENET_POWER_PASSIVE);

	clk_disable_unprepare(priv->clk);

A second suspend has the same problem. The new early return in
bcmgenet_suspend() returns 0, so bcmgenet_suspend_noirq() calls
bcmgenet_power_down() and clk_disable_unprepare(priv->clk) once more.

Would this underflow the clk enable/prepare counts? And would the power-down
register writes (EXT_EXT_PWR_MGMT, or the WoL MPD/HFB/UMAC_CMD sequence) hit
a block that has no clock?

The next patch in the series, "net: bcmgenet: let the interface be reopened
after a failed resume", removes out_clk_disable and keeps the clock enabled
on this failure path. So this only affects this intermediate commit. Could
that change be folded in here or moved before this patch?

[Severity: Medium]

Is the netdev still detached once bcmgenet_close() completes after a failed
resume?

bcmgenet_suspend() called netif_device_detach(dev). The init_dma failure path
in bcmgenet_resume() returns without calling netif_device_attach(), and
bcmgenet_close() does not re-attach either. On the next ifup, __dev_open()
sees that the device is not present and returns -ENODEV.

The next patch in the series, "net: bcmgenet: let the interface be reopened
after a failed resume", adds netif_device_attach(dev) to bcmgenet_close()
after free_irq(). So this also only affects this intermediate commit.

[ ... ]

> @@ -4258,7 +4265,11 @@ static int bcmgenet_resume(struct device *d)
>  
>  	if (device_may_wakeup(d) && priv->wolopts) {
>  		reg = bcmgenet_umac_readl(priv, UMAC_CMD);
> -		if (reg & CMD_RX_EN) {
> +
> +		/* A link event can enable the receiver after a failed
> +		 * resume, so check the rings too
> +		 */

[Severity: Low]

Is a link event really what sets CMD_RX_EN here? The commit message gives
the same cause:

  But after a failed resume the MAC sits in software reset, and the
  next link event takes it out and enables it.

After a failed resume, the PHY state machine is not handling link changes:

- On the non-WoL path, bcmgenet_netif_stop(dev, true) in suspend already
  called phy_stop().
- On the WoL path, the fallback calls phy_stop() if the PHY was started.
  Otherwise bcmgenet_wol_power_down_cfg() leaves phydev->state = PHY_READY.

Either way, bcmgenet_mii_setup()->bcmgenet_mac_config() is not reached. That
is the code that clears CMD_SW_RESET and sets CMD_TX_EN | CMD_RX_EN on link
up.

The bit seems to come from the next WoL suspend instead. It takes the new
early return in bcmgenet_suspend(), and then runs
bcmgenet_suspend_noirq()->bcmgenet_power_down()->bcmgenet_wol_power_down_cfg():

	/* Can't suspend with WoL if MAC is still in reset */
	if (reg & CMD_SW_RESET)
		reg &= ~CMD_SW_RESET;

	/* Receiver must be enabled for WOL MP detection */
	reg |= CMD_RX_EN;

bcmgenet_wol_power_up_cfg() leaves RX_EN set when it puts the MAC back into
SW_RESET. bcmgenet_resume() then sees CMD_RX_EN with no rings.

The added priv->tx_cbs check covers this case anyway. Could the comment and
the commit message describe this path instead?

> +		if (priv->tx_cbs && (reg & CMD_RX_EN)) {
>  			/* Successfully exited WoL, just resume data flows */
>  			list_for_each_entry(rule, &priv->rxnfc_list, list)
>  				if (rule->state == BCMGENET_RXNFC_STATE_ENABLED)

[ ... ]

> @@ -4343,6 +4354,15 @@ static int bcmgenet_suspend(struct device *d)
>  	if (!netif_running(dev))
>  		return 0;
>  
> +	/* A failed resume has torn the datapath down already. Resume still
> +	 * enables irq1 on the WoL path, so it has to be disabled here.
> +	 */
> +	if (!priv->tx_cbs) {
> +		if (device_may_wakeup(d) && priv->wolopts)
> +			disable_irq(priv->irq1);
> +		return 0;
> +	}

[Severity: Medium]

With WoL enabled, can this early return arm the default filter and the
non-wake HFB filters as wake sources?

bcmgenet_resume() calls bcmgenet_hfb_restore() before the
bcmgenet_init_dma() call that can fail. That goes through
bcmgenet_hfb_clear(), which enables the catch-all filter 0 and sets
RBUF_HFB_EN. It then restores every runtime rxnfc filter. Nothing undoes
this when init_dma fails.

On the next suspend, this return skips the "Suspend non-wake Rx data flows"
block further down. That block normally keeps only the RX_CLS_FLOW_WAKE
rules enabled and clears RBUF_HFB_EN when there are none:

		} else {
			bcmgenet_hfb_reg_writel(priv, hfb_enable << 1,
						HFB_FLT_ENABLE_V3PLUS + 4);
		}
		if (!hfb_enable)
			reg &= ~RBUF_HFB_EN;

bcmgenet_suspend_noirq() still calls
bcmgenet_power_down(GENET_POWER_WOL_MAGIC), and
bcmgenet_wol_power_down_cfg() then does:

	reg = UMAC_IRQ_MPD_R;
	if (hfb_ctrl_reg & RBUF_HFB_EN)
		reg |=  UMAC_IRQ_HFB_SM | UMAC_IRQ_HFB_MM;

Could ordinary received traffic that matches filter 0 then wake the system,
even when only magic-packet wake was requested?

This looks like it is still there at the end of the series. The later
patches keep the clock on after a failed resume, so these register writes
take effect.

>  	netif_device_detach(dev);
>  
>  	if (device_may_wakeup(d) && priv->wolopts) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-nb-genet-fini-dma-net-v1-0-47e7c57f6834%40tipi-net.de