Re: [PATCH net v2 02/10] net: systemport: Fix invalid dev_id argument in bcm_sysport_poll_controller()

[netdev-bot+sashiko]

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

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

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] Now that bcm_sysport_poll_controller() passes a valid dev, the
  polled calls to bcm_sysport_rx_isr() and bcm_sysport_tx_isr() actually…

Pre-existing issues:
- [Medium] bcm_sysport_poll_controller()
  (drivers/net/ethernet/broadcom/bcmsysport.c:1196-1209) calls…

--

net: systemport: Fix invalid dev_id argument in bcm_sysport_poll_controller()

This commit makes bcm_sysport_poll_controller() pass dev instead of priv
to bcm_sysport_rx_isr() and bcm_sysport_tx_isr(). Both ISRs call
netdev_priv() on their dev_id argument, so passing priv crashed them when
netconsole or netpoll ran.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index db627cd15fb7a..8328fe824d152 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -1198,12 +1198,12 @@ static void bcm_sysport_poll_controller(struct net_device *dev)
>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>  
>  	disable_irq(priv->irq0);

[Severity: Medium]
This is a pre-existing issue, but is it safe to call disable_irq() here?

The only caller of ndo_poll_controller is netpoll_poll_dev(). It is called
from the retry loop in __netpoll_send_skb(), and that loop runs with IRQs
disabled:

net/core/netpoll.c:__netpoll_send_skb() {
    ...
	lockdep_assert_irqs_disabled();
    ...
}

netpoll_send_skb() calls local_irq_save() before this. For netconsole,
target_list_lock is also held.

disable_irq() can sleep:

kernel/irq/manage.c:disable_irq() {
	might_sleep();
	if (!__disable_irq_nosync(irq))
		synchronize_irq(irq);
}

With CONFIG_DEBUG_ATOMIC_SLEEP, would this print "sleeping function called
from invalid context" each time netpoll has to poll the device because TX
is busy or the queue is stopped?

The handlers are requested with flags 0. With threadirqs or PREEMPT_RT
they are force-threaded, so synchronize_irq() can wait_event() on the IRQ
thread with IRQs disabled.

Before this patch the path oopsed. Now it completes, so netconsole users on
this NIC would hit this problem next. No later patch in the series changes
the disable_irq() calls.

The driver already does TX completion in NAPI. netpoll_poll_dev() also
calls poll_napi() right after the callback:

	ops->ndo_poll_controller(dev);

	poll_napi(dev);

Could bcm_sysport_poll_controller() be removed entirely, as was done for
other NAPI drivers? Or could it at least use disable_hardirq(), as
e1000_netpoll() does?

> -	bcm_sysport_rx_isr(priv->irq0, priv);
> +	bcm_sysport_rx_isr(priv->irq0, dev);

[Severity: Medium]
Now that the ISR calls here actually run, will the masked status usually
be zero?

On the previous netpoll_poll_dev() iteration, poll_napi() already cleared
the status. bcm_sysport_desc_rx() clears INTRL2_0_RDMA_MBDONE, and
__bcm_sysport_tx_reclaim() clears each ring's INTRL2_1 bit:

	intrl2_1_writel(ring->priv, BIT(ring->index), INTRL2_CPU_CLEAR);

Sources whose NAPI is scheduled are also masked. In that case
bcm_sysport_rx_isr() takes this path:

	if (unlikely(priv->irq0_stat == 0)) {
		netdev_warn(priv->netdev, "spurious RX interrupt\n");
		return IRQ_NONE;
	}

On non-Lite hardware, bcm_sysport_tx_isr() prints "spurious TX interrupt"
in the same way.

While TX is busy, the retry loop in __netpoll_send_skb() calls
netpoll_poll_dev() up to jiffies_to_usecs(1)/USEC_PER_POLL times. Could
that fill the log with unratelimited spurious interrupt warnings? That is
up to 40 at HZ=1000 and 400 at HZ=100 each time txq goes from empty to
busy.

Each warning is also a new printk that netconsole tries to send over the
same congested interface.

Should the polled path skip this warning or ratelimit it, or should the
poll_controller be removed?

>  	enable_irq(priv->irq0);
>  
>  	if (!priv->is_lite) {
>  		disable_irq(priv->irq1);
> -		bcm_sysport_tx_isr(priv->irq1, priv);
> +		bcm_sysport_tx_isr(priv->irq1, dev);
>  		enable_irq(priv->irq1);
>  	}
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922232440.598918-1-florian.fainelli%40broadcom.com