Re: [PATCH net-next v7 16/18] net: ethernet: cortina: Drain RX coalescing timer on stop

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not explain that moving hrtimer_setup()
  in gmac_open() (drivers/net/ethernet/cortina/gemini.c) fixes a…
- [Low] The commit message's reason for keeping the first
  hrtimer_cancel() in gmac_stop()…

--

net: ethernet: cortina: Drain RX coalescing timer on stop

This patch moves hrtimer_setup() for the RX coalescing timer in
gmac_open() so it runs before request_irq(). It also adds a second
hrtimer_cancel() in gmac_stop() after free_irq(). The aim is that no RX
interrupt can leave the timer queued after the port state has been freed.

> Initialize the timer before requesting the IRQ, and cancel it again
> after free_irq() has drained every handler that can rearm it. Keep the
> first cancellation to prevent the timer callback from scheduling NAPI
> while the poll instance is being disabled.

[Severity: Low]
Is this description of the first cancellation accurate? The paragraph
before it says an RX interrupt racing with that cancellation can rearm
the timer. That still seems possible while napi_disable() runs.

In gmac_stop(), the RX interrupt is only masked after napi_disable():

gmac_stop() {
	hrtimer_cancel(&port->rx_coalesce_timer);
	netif_tx_stop_all_queues(netdev);
	gmac_disable_tx_rx(netdev);
	gmac_stop_dma(port);
	napi_disable(&port->napi);
	...
	gmac_enable_irq(netdev, 0);
	...
}

A NAPI poll that finishes at the same time as the first cancel turns
the RX interrupt back on:

gmac_napi_poll() {
	...
	if (!reschedule && received < budget &&
	    napi_complete_done(napi, received))
		gmac_enable_rx_irq(napi->dev, 1);
	...
}

After that, gmac_irq() can call hrtimer_start() again. Then
gmac_coalesce_delay_expired()->napi_schedule() can run while
napi_disable() is still in progress. When rx_coalesce_nsecs is 0,
gmac_irq() calls napi_schedule() directly in the same window.

This does not cause a functional problem. A napi_schedule() on a NAPI
instance that is being disabled is either a no-op or gets waited out.
Still, the first cancel only handles a timer that is already pending. It
does not handle the rearm case.

Could the commit message say the early cancel is only best-effort?
Another option is to mask the RX interrupt before napi_disable().

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 59e6aeb07e0b4..58d6ed81a59c8 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2201,6 +2201,10 @@ static int gmac_open(struct net_device *netdev)
>  	struct gemini_ethernet_port *port = netdev_priv(netdev);
>  	int err;
>  
> +	hrtimer_setup(&port->rx_coalesce_timer,
> +		      &gmac_coalesce_delay_expired,
> +		      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +
>  	err = request_irq(netdev->irq, gmac_irq,
>  			  IRQF_SHARED, netdev->name, netdev);
>  	if (err) {
> @@ -2237,9 +2241,6 @@ static int gmac_open(struct net_device *netdev)
>  	gmac_enable_tx_rx(netdev);
>  	netif_tx_start_all_queues(netdev);
>  
> -	hrtimer_setup(&port->rx_coalesce_timer, &gmac_coalesce_delay_expired, CLOCK_MONOTONIC,
> -		      HRTIMER_MODE_REL);
> -
>  	netdev_dbg(netdev, "opened\n");
>  
>  	return 0;

[Severity: Low]
This move also seems to fix a separate race on the open side, and the
commit message does not describe it. Should the commit message mention
it, or should it go in its own patch?

Before this patch, gmac_open() called gmac_enable_irq(netdev, 1) before
hrtimer_setup(). gmac_irq() arms the timer once the RX interrupt is
unmasked, if rx_coalesce_nsecs is non-zero:

	if (!port->rx_coalesce_nsecs) {
		napi_schedule(&port->napi);
	} else {
		...
		hrtimer_start(&port->rx_coalesce_timer, ktime,
			      HRTIMER_MODE_REL);
	}

The default is 0, but gmac_set_coalesce() lets rx-usecs be set while
the interface is down. In that case, an RX interrupt can arrive between
gmac_enable_irq(netdev, 1) and the old hrtimer_setup() call:

- On the first open, it would start a zeroed hrtimer that was never
  initialized.

- On a reopen, it would start a timer that hrtimer_setup() then
  re-initializes while it is still queued.

The stop-side fix does not depend on this move, because ndo_stop only
runs after a successful open. If the commit message does not explain the
move, backporters might treat this hunk as cosmetic.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-gemini-ethernet-fixes-3-v7-0-9c32a1a46aec%40kernel.org