Re: [PATCH v2 6/7] net: ravb: fix resource teardown ordering

[netdev-bot+sashiko@kernel.org]

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

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

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

- [High] The patch rests on the commit-message claim that "A running
  interface already holds a runtime PM reference ... A resume failure…
- [Medium] The commit says it fixes the probe error paths, but in the
  early ones the IRQ handler can still make an invalid access.
- [Medium] When pm_runtime_resume_and_get() fails in ravb_remove(), the
  patched code now carries on to ravb_mdio_release() (line 3161) instead…
- [Low] The first Fixes: tag is wrong.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but ravb_close()
  returns early when ravb_set_opmode(ndev, CCC_OPC_RESET) fails (lines…
- [Medium] This problem wasn't introduced by this patch, but if
  ravb_set_opmode(ndev, CCC_OPC_CONFIG) fails in ravb_probe() (lines…
- [Medium] This problem wasn't introduced by this patch, but
  ravb_interrupt() (and the other ravb handlers) call…
- [Low] This problem wasn't introduced by this patch, but the
  ravb_probe() error path calls pm_runtime_put(&pdev->dev) (async, line…

--

net: ravb: fix resource teardown ordering

This patch puts the ravb IRQs in a dedicated devres group. The group is
released before free_netdev() in ravb_remove() and in the ravb_probe()
error paths. ravb_remove() also keeps tearing down when
pm_runtime_resume_and_get() fails, and skips only the matching runtime
PM put.

> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

[Severity: Low]
Is c156633f1353 the right commit for the first Fixes: tag?

In c156633f1353 the open path took the IRQ with request_irq(), and the
close path released it with free_irq(ndev->irq, ndev). So
unregister_netdev() had already freed it before free_netdev() ran.

The window comes from the devm-managed IRQ that uses ndev as dev_id, in
ravb_setup_irq():

    error = devm_request_irq(dev, irq_num, handler, flags, devname, ndev);

git blame points at 32f012b8c01c ("net: ravb: Move getting/requesting
IRQs in the probe() method"). Should the tag be this instead?

Fixes: 32f012b8c01c ("net: ravb: Move getting/requesting IRQs in the probe() method")

The current tag selects stable trees back to v4.2. Those trees have no
ravb_setup_irqs() and free the IRQs in ndo_stop. The second tag,
48f894ab07c4, looks right for the early return in ravb_remove().

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791e..a25f5ac7062fc 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>  	}
>  
> +	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
> +		error = -ENOMEM;
> +		goto out_reset_assert;
> +	}
> +
>  	error = ravb_setup_irqs(priv);
>  	if (error)
> -		goto out_reset_assert;
> +		goto out_release_irqs;
> +
> +	devres_close_group(&pdev->dev, priv);
>  
>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		error = PTR_ERR(priv->clk);
> -		goto out_reset_assert;
> +		goto out_release_irqs;
>  	}

[Severity: Medium]
Can the IRQ handler still make an invalid access when this path reaches
out_release_irqs?

The IRQs are live once devres_close_group() returns. ravb_probe() maps
priv->addr and initialises priv->lock only later:

    priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
    ...
    spin_lock_init(&priv->lock);

These failures reach devres_release_group() while priv->addr is still
NULL:

- the clock lookup failures here, including -EPROBE_DEFER
- the pm_runtime_resume_and_get() failure

The ioremap failure also reaches it, with an ERR_PTR in priv->addr.

On single-IRQ variants, ravb_setup_irq() requests the line with
IRQF_SHARED. Under CONFIG_DEBUG_SHIRQ, __free_irq() calls the handler of
a shared action:

kernel/irq/manage.c:__free_irq() {
    ...
	if (action->flags & IRQF_SHARED) {
		local_irq_save(flags);
		action->handler(irq, dev_id);
		local_irq_restore(flags);
	}
    ...
}

A real interrupt from another device on the same line would do the
same.

ravb_interrupt() only checks pm_runtime_active(). That returns true when
runtime PM is not enabled yet, and also after out_rpm_disable has
disabled it:

ravb_interrupt() {
    ...
	pm_runtime_get_noresume(dev);

	if (unlikely(!pm_runtime_active(dev)))
		goto out_rpm_put;

	spin_lock(&priv->lock);
	/* Get interrupt status */
	iss = ravb_read(ndev, ISS);
    ...
}

The handler then takes an uninitialised spinlock and does ioread32()
through a NULL or ERR_PTR base. Before this patch the same call hit a
freed ndev.

Would it be better to request the IRQs after the MMIO mapping and
spin_lock_init()?

[ ... ]

> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);

[Severity: Low]
This isn't a bug introduced by this patch, but the out_rpm_put label just
above calls the async put and then disables runtime PM right away:

out_rpm_put:
	pm_runtime_put(&pdev->dev);
out_rpm_disable:
	pm_runtime_disable(&pdev->dev);

Can __pm_runtime_barrier() in pm_runtime_disable() cancel the queued
idle request? If so, ravb_runtime_suspend() and the genpd callbacks
never run before probe fails.

ravb_remove() already uses pm_runtime_put_sync_suspend() here. Should the
probe error path use a synchronous put too?

[Severity: Medium]
This is a pre-existing issue, but does ravb_probe() leak priv->desc_bat
when ravb_set_opmode(ndev, CCC_OPC_CONFIG) fails?

	error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
	if (error)
		goto out_rpm_put;

In the error path, the only dma_free_coherent() of desc_bat is under
out_reset_mode, which comes before out_rpm_put:

out_reset_mode:
	ravb_set_opmode(ndev, CCC_OPC_RESET);
	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
			  priv->desc_bat_dma);
out_rpm_put:

The allocation is not devres-managed and is not in the new IRQ group.

>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	clk_unprepare(priv->refclk);
> +out_release_irqs:
> +	devres_release_group(&pdev->dev, priv);
>  out_reset_assert:
>  	reset_control_assert(rstc);
>  out_free_netdev:
> @@ -3141,9 +3150,10 @@ static void ravb_remove(struct platform_device *pdev)
>  
>  	error = pm_runtime_resume_and_get(dev);
>  	if (error < 0)
> -		return;
> +		dev_warn(dev, "failed to resume device: %d\n", error);
>  
>  	unregister_netdev(ndev);

[Severity: High]
Can unregister_netdev() call ravb_close() a second time when the resume
above fails?

The commit message says:

  A running interface already holds a runtime PM reference, so the extra
  get cannot invoke a failing resume. A resume failure therefore occurs
  while the interface is down and ndo_stop() will not be called.

A failed system resume seems to break that assumption. With WoL
disabled, ravb_suspend() closes the interface directly and leaves
IFF_UP set:

ravb_suspend()
    netif_device_detach(ndev);
    ravb_close(ndev);
        napi_disable(&priv->napi[RAVB_BE]);
        pm_runtime_put_autosuspend(dev);
    pm_runtime_force_suspend(&priv->pdev->dev);
    reset_control_assert(priv->rstc);

If ravb_resume() then fails in reset_control_deassert(), it returns
before pm_runtime_force_resume() and ravb_open():

	ret = reset_control_deassert(priv->rstc);
	if (ret)
		return ret;

That leaves runtime PM disabled with RPM_SUSPENDED status. The
interface is still IFF_UP but closed, NAPI is disabled, and no PM
reference is held. A pm_runtime_force_resume() failure leaves a similar
state.

On a later unbind, pm_runtime_resume_and_get() here returns -EACCES
every time. Before this patch ravb_remove() returned at that point. It
now continues into unregister_netdev().

__dev_close_many() calls ndo_stop for any IFF_UP device, even after
netif_device_detach().

The second ravb_close() then does the following on an IP that is
clock-gated and held in reset:

- writes RIC0/RIC2/TIC/ECSIPR
- runs ravb_ptp_stop() and ravb_stop_dma()

It then calls napi_disable() on a NAPI instance that was never
re-enabled, and napi_disable_locked() loops forever:

	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}

Wouldn't that hang removal with RTNL and the netdev instance lock held?

Past that point, ravb_ring_free() would call page_pool_destroy() again
on the stale priv->rx_pool[q]. pm_runtime_put_autosuspend() would also
underflow the usage count.

Also, with runtime PM disabled, pm_runtime_active() returns true. The
shared-IRQ handlers are then no longer kept off the unpowered hardware
until devres_release_group() runs.

A similar double close could already happen when the resume here
succeeds after a failed system resume. The failing-resume path is new
with this patch. The if (error >= 0) check covers only ravb_remove()'s
own put, not these other states.

> +	devres_release_group(dev, priv);

[Severity: Medium]
This isn't a bug introduced by this patch, but is the runtime PM check in
the ravb IRQ handlers safe against a concurrent runtime suspend?

ravb_interrupt() and the other ravb handlers do this without holding
dev->power.lock:

	pm_runtime_get_noresume(dev);

	if (unlikely(!pm_runtime_active(dev)))
		goto out_rpm_put;

rpm_suspend() checks usage_count and sets RPM_SUSPENDING within one
dev->power.lock critical section. The handler's increment and status
read can both fall in the window between those two steps.

The handler then reads ISS and other registers while
ravb_runtime_suspend() calls clk_disable(priv->refclk) and genpd gates
the clocks.

ravb_runtime_suspend() also has no synchronize_irq(), even though the
main IRQ is IRQF_SHARED.

Would using pm_runtime_get_if_active() in the handlers and adding
synchronize_irq() to ravb_runtime_suspend() close this?

>  	if (info->nc_queues)
>  		netif_napi_del(&priv->napi[RAVB_NC]);
>  	netif_napi_del(&priv->napi[RAVB_BE]);

[Severity: Medium]
When pm_runtime_resume_and_get() has failed, what happens to the MDIO
accesses in the ravb_mdio_release() call that follows?

ravb_mdio_release() calls mdiobus_unregister(), and PHY teardown issues
MDIO transactions:

ravb_mdio_release()
  mdiobus_unregister()
    phy_remove()
      phy_leds_unregister()
        led_classdev_unregister()
          led_set_brightness(led_cdev, LED_OFF)
      phydev->drv->remove(phydev)

On ravb, MDIO is bit-banged through the AVB PIR register:

ravb_mdio_ctrl()
	ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);

ravb_get_mdio_data()
	return (ravb_read(priv->ndev, PIR) & PIR_MDI) != 0;

Because the resume failed, the module clock/refclk is still gated. After
a failed system resume the reset may also still be asserted.

In that state, writes to PIR would be lost. Reads would return garbage
that mdio-bitbang treats as valid. On SoCs that fault on access to a
stopped module, this could raise a bus error.

The commit message only argues that ndo_stop() will not be called.
Should this path skip or otherwise handle the register accesses made by
MDIO teardown?

> @@ -3153,7 +3163,8 @@ static void ravb_remove(struct platform_device *pdev)
>  	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>  			  priv->desc_bat_dma);
>  
> -	pm_runtime_put_sync_suspend(&pdev->dev);
> +	if (error >= 0)
> +		pm_runtime_put_sync_suspend(&pdev->dev);

[Severity: Medium]
This is a pre-existing issue, but ravb_close() returns early when the
switch to reset opmode fails:

	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
	if (error)
		return error;

	pm_runtime_put_autosuspend(dev);

Does that leak the runtime PM reference taken by
pm_runtime_resume_and_get() in ravb_open()?

The core ignores the return value of ndo_stop. The interface goes down
while the device stays RPM_ACTIVE.

The get/put pair here in ravb_remove() only balances its own reference.
The device would then never be runtime-suspended before
pm_runtime_disable().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927144741.1320558-1-yaojiale02%40163.com