Re: [PATCH net-next v3 03/12] net: ethernet: cortina: Drain free queue IRQ before resize

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 (1) · High: 1 · Medium: 1 (2) · Low: 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 free queue drain is incomplete.
- [Medium] This patch adds two new reads of geth->port1 inside
  geth_resize_freeq(): disable_irq(geth->port1->irq) (line 1104) and…
- [Low] Comments about the initial free queue setup no longer match the
  code.

Pre-existing issues:
- [Critical] geth_cleanup_freeq() does `while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);` (line 1024).
- [Medium] Free queue pages are mapped with dma_map_single(...,
  PAGE_SIZE, DMA_FROM_DEVICE) in geth_freeq_alloc_map_page() (line 822).
- [Medium] Rebuilding the free queue at a smaller order does not reset
  the hardware SWFQ pointers.

--

net: ethernet: cortina: Drain free queue IRQ before resize

This patch disables port 1's Linux IRQ for the whole of geth_resize_freeq().
The aim is to drain the threaded free queue handler before the ring and page
metadata are torn down and rebuilt. It also adds a geth_set_freeq_irq()
helper, re-enables SWFQ_EMPTY_INT_BIT only when setup succeeds, and clears
the freed free queue state.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 15ad9f721dd52..a2daf22e76989 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -984,6 +984,8 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
>  	}
>  
>  	kfree(geth->freeq_pages);
> +	geth->freeq_pages = NULL;
> +	geth->num_freeq_pages = 0;
>  err_freeq:

[Severity: Medium]
This isn't a bug introduced by this patch, but the free queue pages are
mapped with PAGE_SIZE in geth_freeq_alloc_map_page():

    mapping = dma_map_single(geth->dev, page_address(page),
                             PAGE_SIZE, DMA_FROM_DEVICE);

The err_freeq_alloc unwind just above here in geth_setup_freeq() and the
teardown loop in geth_cleanup_freeq() both unmap with frag_len, which is
normally 2048:

    dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);

Shouldn't the unmap size match the PAGE_SIZE used when mapping? As written,
this would trigger DMA-API debug warnings. On swiotlb or non-coherent setups
it would also only partly unmap and sync the page.

The commit "net: ethernet: cortina: Correct free queue DMA mappings" later
in the series appears to fix this.

[ ... ]

> @@ -1024,10 +1026,28 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  	}
>  
>  	kfree(geth->freeq_pages);
> +	geth->freeq_pages = NULL;
> +	geth->num_freeq_pages = 0;

[Severity: Critical]
This is a pre-existing issue, but the loop just above in geth_cleanup_freeq()
drops every reference on each free queue page:

    gpage = &geth->freeq_pages[pn];
    while (page_ref_count(gpage->page) > 0)
        put_page(gpage->page);

gmac_rx() gives fragment references from these pages to skbs:

    skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);

Those skbs can still be in GRO or in socket receive queues when a resize
runs, for example on open after an RX ring change, or on ethtool -G.

Could this free pages that in-flight skbs still reference? The later skb
free would then call put_page() on a freed or reallocated page.

The commit "net: ethernet: cortina: Preserve in-flight free queue pages"
later in the series appears to fix this by giving each posted fragment its
own reference.

>  
>  	dma_free_coherent(geth->dev,
>  			  sizeof(*geth->freeq_ring) << geth->freeq_order,
>  			  geth->freeq_ring, geth->freeq_dma_base);
> +	geth->freeq_ring = NULL;
> +}

[Severity: Medium]
This is also a pre-existing issue. When the queue is rebuilt at a smaller
order, can the hardware SWFQ pointers end up outside the new ring?

geth_cleanup_freeq() only sets wptr equal to rptr and clears the base/size
register:

    writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
           geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
    writel(0, geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG);

The only place that writes 0 to GLOBAL_SWFQ_RWPTR_REG is
gemini_ethernet_init().

After geth_resize_freeq() sets freeq_order = new_order, geth_setup_freeq()
calls geth_fill_freeq(geth, false). That starts from the old pointer and does
not mask it with the new m_pn:

    pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
    ...
    gpage = &geth->freeq_pages[pn];

If rptr had advanced past the end of the smaller ring, wouldn't this index
past the newly allocated freeq_pages[] and write freeq_ring entries out of
bounds? One way to get there is to run traffic, take both ports down, and
shrink the RX rings with ethtool -G.

Whether this happens depends on the hardware keeping rptr when
GLOBAL_SW_FREEQ_BASE_SIZE_REG is reprogrammed, and the driver code doesn't
show that. The final state of the series still derives pn from an unmasked
wptr, so later patches don't seem to fix this.

[ ... ]

> @@ -1080,16 +1098,11 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	new_order = min(15, ilog2(new_size - 1) + 1);
>  	dev_dbg(geth->dev, "set shared queue to size %d order %d\n",
>  		new_size, new_order);
> -	if (geth->freeq_order == new_order)
> +	if (geth->freeq_ring && geth->freeq_order == new_order)
>  		return 0;

[Severity: Low]
This isn't a bug, but the new freeq_ring check forces the first setup
whatever freeq_order holds. That makes the sentinel in gemini_ethernet_init()
redundant, and its comment no longer matches the code:

    /* This makes the queue resize on probe() so that we
     * set up and enable the queue IRQ. FIXME: fragile.
     */
    geth->freeq_order = 1;

The kernel-doc for geth_resize_freeq() also still says "This gets called at
least once during probe()". The only callers are gmac_open() and
gmac_set_ringparam(). That probe() wording was there before this patch.

Could these comments be updated?

>  
> -	spin_lock_irqsave(&geth->irq_lock, flags);
> -
> -	/* Disable the software queue IRQs */
> -	en = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	en &= ~SWFQ_EMPTY_INT_BIT;
> -	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	spin_unlock_irqrestore(&geth->irq_lock, flags);
> +	disable_irq(geth->port1->irq);
> +	geth_set_freeq_irq(geth, false);

[Severity: High]
Does draining only port1->irq here cover every instance of the free queue
handler?

gemini_ethernet_port_probe() registers the same handler pair on both ports'
IRQ lines:

    ret = devm_request_threaded_irq(port->dev,
                                    port->irq,
                                    gemini_port_irq,
                                    gemini_port_irq_thread,
                                    IRQF_SHARED,
                                    port_names[port->id],
                                    port);

gemini_port_irq() only reads the global status and enable registers and
never checks port->id:

    val = readl(geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
    en = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);

    if (val & en & SWFQ_EMPTY_INT_BIT) {
        ...
        ret = IRQ_WAKE_THREAD;
    }

Port 0's line stays enabled during the resize. It is shared with gmac_irq,
which gmac_open() requests before the resize, and irqpoll or misrouted IRQ
polling can also run it through try_one_irq().

If that handler runs between disable_irq(port1) and
geth_set_freeq_irq(geth, false), it wakes port 0's thread. Nothing here waits
for that thread. The thread then runs:

gemini_port_irq_thread()
    geth_fill_freeq(geth, true)
        gpage = &geth->freeq_pages[pn];

This can run while geth_cleanup_freeq() frees freeq_pages and sets it to
NULL, or while geth_setup_freeq() is still sleeping in its allocations.

Afterwards the thread ORs SWFQ_EMPTY_INT_BIT back into
GLOBAL_INTERRUPT_ENABLE_4_REG unconditionally, even if setup failed. The next
SWFQ interrupt would then run geth_fill_freeq() with freeq_pages set to NULL.

The commit message says:

  "disable_irq() drains hard and threaded handlers and prevents a new
  thread from being woken."

Isn't that true only for port 1's registration? Would gemini_port_irq() need
to filter SWFQ handling by port->id, or would both ports' IRQs need to be
disabled? The final state of the series still drains only port1->irq with the
same handler on both lines.

>  
>  	/* Drop the old queue */
>  	if (geth->freeq_ring)
> @@ -1103,10 +1116,9 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	 * after probe(), this is where the interrupts get turned on
>  	 * in the first place.
>  	 */
> -	spin_lock_irqsave(&geth->irq_lock, flags);
> -	en |= SWFQ_EMPTY_INT_BIT;
> -	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	spin_unlock_irqrestore(&geth->irq_lock, flags);
> +	if (!ret)
> +		geth_set_freeq_irq(geth, true);
> +	enable_irq(geth->port1->irq);

[Severity: Medium]
Can geth->port1 be NULL by the time this line runs?

The only NULL check is at the top of geth_resize_freeq():

    if (!geth->port1)
        return -ENODEV;

Between that check and this enable_irq() there is a long window where the
task can sleep. disable_irq() waits for the threaded handler, then
geth_cleanup_freeq() and geth_setup_freeq() run, with dma_alloc_coherent()
and GFP_KERNEL allocations.

Only RTNL serializes geth_resize_freeq(). gemini_port_clear() sets
geth->port1 = NULL without holding RTNL in two places:

- gemini_port_remove(), after unregister_netdev() returns, or with no netdev
  at all for a PHY-less port 1
- the probe unprepare error path

devm then frees port 1's IRQ action and the port memory. For example:

CPU0 (eth0 open)                 CPU1 (port 1 unbind or probe failure)
geth_resize_freeq()
  geth->port1 is non-NULL
  disable_irq(geth->port1->irq)
  geth_setup_freeq() sleeps
                                 gemini_port_clear()
                                   geth->port1 = NULL;
                                 devm frees the IRQ and the port
  enable_irq(geth->port1->irq)   <- NULL dereference

Probe has a second window. gemini_ethernet_port_probe() sets
geth->port1 = port before it calls devm_request_threaded_irq(). A resize from
port 0 in that window calls disable_irq() on a descriptor with no action.

Won't the first __setup_irq() then reset the depth through irq_startup()?
That would cancel the disable, and the later enable_irq() would hit the
"Unbalanced enable for IRQ" warning.

Before this patch, geth->port1 was only read at the top of the function.
These two re-reads are unchanged in the final state of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-gemini-ethernet-fixes-3-v3-0-3e1f25890ae8%40kernel.org