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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 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).

- [High] bcm_sysport_poll_controller()
  (drivers/net/ethernet/broadcom/bcmsysport.c:1199-1211, post-patch)…
- [Medium] With the dev_id argument fixed, bcm_sysport_poll_controller()
  now performs real MMIO against the SYSTEMPORT INTRL2 block from…

--

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

This corrects bcm_sysport_poll_controller() so that it hands the
struct net_device pointer, rather than the driver private pointer, to
bcm_sysport_rx_isr() and bcm_sysport_tx_isr(), both of which call
netdev_priv() on their dev_id argument.

A couple of questions about the surrounding code in that callback below.

> 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);
> -	bcm_sysport_rx_isr(priv->irq0, priv);
> +	bcm_sysport_rx_isr(priv->irq0, dev);
>  	enable_irq(priv->irq0);

[Severity: High]
Can disable_irq() be called from this callback at all?  ndo_poll_controller
is invoked by netpoll with hard IRQs disabled and under rcu_read_lock():

net/core/netpoll.c:netpoll_send_skb() {
	local_irq_save(flags);
	...__netpoll_send_skb(np, skb);
}

/* call with IRQ disabled */
static netdev_tx_t __netpoll_send_skb(...)
{
	...
	lockdep_assert_irqs_disabled();
	...
			/* tickle device maybe there is some cleanup */
			netpoll_poll_dev(np->dev);
	...
		WARN_ONCE(!irqs_disabled(), ...);
}

and netpoll_poll_dev() then does:

	ops = dev->netdev_ops;
	if (ops->ndo_poll_controller)
		ops->ndo_poll_controller(dev);

	poll_napi(dev);

while disable_irq() is a sleeping API:

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

Does this produce a "sleeping function called from invalid context" splat with
CONFIG_DEBUG_ATOMIC_SLEEP on every netpoll poll of a SYSTEMPORT interface?

There also looks to be a hang case.  If the netconsole message being
transmitted originates from inside the handler itself, for example:

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

then irq0 is IRQD_IRQ_INPROGRESS on this CPU when synchronize_irq() runs:

	while (irqd_irq_inprogress(&desc->irq_data))
		cpu_relax();
...
	wait_event(desc->wait_for_threads, !atomic_read(&desc->threads_active));

With local IRQs disabled the in-progress handler can never complete, so does
this spin forever?  And with force-threaded handlers (threadirqs or
PREEMPT_RT), does the wait_event() schedule while atomic?

Given that netpoll_poll_dev() already calls poll_napi() and drives the
driver's NAPI instances, would removing ndo_poll_controller from
bcm_sysport_netdev_ops be preferable to fixing the argument?  At a minimum,
would disable_irq_nosync() be more appropriate here?

>  
>  	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);
>  	}
>  }

[Severity: Medium]
Now that the register base is correct, these hand-calls perform real MMIO
against the INTRL2 block from netpoll context.  Both handlers read the status
latch and acknowledge it:

bcm_sysport_rx_isr() {
	priv->irq0_stat = intrl2_0_readl(priv, INTRL2_CPU_STATUS) &
			  ~intrl2_0_readl(priv, INTRL2_CPU_MASK_STATUS);
	intrl2_0_writel(priv, priv->irq0_stat, INTRL2_CPU_CLEAR);

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

bcm_sysport_tx_isr() {
	intrl2_1_writel(priv, 0xffffffff, INTRL2_CPU_CLEAR);

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

When netpoll is only tickling a stopped TX queue there is typically nothing
latched, so does this emit "spurious RX interrupt" / "spurious TX interrupt"
from inside the netconsole transmit path, feeding those warnings back into the
same path?

A second concern, which I could not confirm from the tree since it depends on
the brcmstb interrupt controller latching semantics: events consumed and
cleared here happen outside hardirq context, so if a parent-level request is
still pending after enable_irq(), the real handler would run against an
already-cleared status latch, warn and return IRQ_NONE.  Could repeated
IRQ_NONE returns be accounted by note_interrupt() and eventually escalate to
"nobody cared" disabling of the SYSTEMPORT line?

Deleting the callback and relying on poll_napi() would avoid both of these.

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