Re: [PATCH net-next 1/3] net: ethernet: cortina: Keep shared free queue parent-owned

[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] Claim that after removing the queue teardown from
  gemini_port_remove(), unbinding only the eth1 port device leaves the…

Pre-existing issues:
- [High] gemini_port_remove() (the function this patch edits) never
  clears geth->port0 / geth->port1, and geth_resize_freeq() dereferences…
- [High] geth_resize_freeq() re-arms SWFQ_EMPTY_INT_BIT in
  GLOBAL_INTERRUPT_ENABLE_4_REG unconditionally, ignoring…
- [Medium] geth_cleanup_freeq() drops every reference on each queue page
  (`while (page_ref_count(gpage->page) > 0) put_page(gpage->page);`),…

--

net: ethernet: cortina: Keep shared free queue parent-owned

The shared software free queue was torn down by every ethernet-port child
removal and then once more by the parent removal. This patch moves the
teardown to the parent only, depopulates the child devices first, masks the
free-queue interrupt after the managed handlers are gone, and makes
geth_cleanup_freeq() a no-op when no queue was allocated.

Resolves: https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2fe7fd0202d2..31bcd41c17fa 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1004,6 +1004,9 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  	unsigned int pages = len >> fpp_order;
>  	unsigned int pn;
>  
> +	if (!geth->freeq_ring)
> +		return;
> +
>  	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);

[Severity: High]
This is a pre-existing issue and not introduced here, but since the new
guard keys on geth->freeq_ring it may be worth looking at what the rest of
the free-queue state looks like when that pointer is NULL.

geth_setup_freeq() clears only freeq_ring on the error paths:

	kfree(geth->freeq_pages);
err_freeq:
	dma_free_coherent(geth->dev,
			  sizeof(*geth->freeq_ring) << geth->freeq_order,
			  geth->freeq_ring, geth->freeq_dma_base);
	geth->freeq_ring = NULL;
	return -ENOMEM;

geth->freeq_pages keeps pointing at the kfree()d array, and
geth_resize_freeq() re-arms the free-queue interrupt without looking at the
return value:

	geth->freeq_order = new_order;
	ret = geth_setup_freeq(geth);

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

	return ret;

So after a failing gmac_open() -> geth_resize_freeq() (dma_alloc_coherent(),
kzalloc_objs(), geth_freeq_alloc_map_page() or a zero return from
geth_fill_freeq()), GLOBAL_SW_FREEQ_BASE_SIZE_REG has just been written to 0
by geth_cleanup_freeq() while SWFQ_EMPTY_INT_BIT stays enabled, and nothing
in the gmac_open() unwind masks it again.

When the free-queue-empty status then asserts, gemini_port_irq() sees
val & en & SWFQ_EMPTY_INT_BIT and returns IRQ_WAKE_THREAD, and
gemini_port_irq_thread() -> geth_fill_freeq(geth, true) walks the freed
array:

		gpage = &geth->freeq_pages[pn];
		page = gpage->page;
		...
		if (page_ref_count(page) > 1) {

Should the interrupt only be re-enabled when geth_setup_freeq() succeeded,
and should the error paths also set geth->freeq_pages to NULL?

> @@ -2391,7 +2394,6 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
>  		unregister_netdev(port->netdev);
>  	}
>  	clk_disable_unprepare(port->pclk);
> -	geth_cleanup_freeq(port->geth);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but since this is now the whole
of the child teardown, is anything supposed to clear the parent's
back-pointer to the port?

geth->port0 / geth->port1 are only ever assigned, in
gemini_ethernet_port_probe():

	/* Assign pointer in the main state container */
	if (!id)
		geth->port0 = port;
	else
		geth->port1 = port;

The port object is the netdev private area from
devm_alloc_etherdev_mqs(), so devres frees it right after
gemini_port_remove() returns, while geth->portN still points at it.

geth_resize_freeq() then dereferences the sibling slot before any NULL
test:

	if (netdev->dev_id == 0)
		other_netdev = geth->port1->netdev;
	else
		other_netdev = geth->port0->netdev;

	if (other_netdev && netif_running(other_netdev))
		return -EBUSY;

Can unbinding one ethernet-port child and then bringing the surviving port
up (gmac_open() -> geth_resize_freeq(), or ethtool -G ->
gmac_set_ringparam()) read the freed netdev private area here and pass the
resulting garbage to netif_running()?

The same statement also dereferences NULL if the sibling port never reached
the geth->portN assignment, for example on a probe failure or
-EPROBE_DEFER, since the check below only guards other_netdev and not the
port pointer.

> @@ -2683,6 +2685,8 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
>  {
>  	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
>  
> +	devm_of_platform_depopulate(&pdev->dev);
> +	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
>  	geth_cleanup_freeq(geth);
>  	geth->initialized = false;
>  }

[Severity: Medium]
This is a pre-existing issue rather than something added here, but this
call site is now the only path into the forced reference drop in
geth_cleanup_freeq():

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

gmac_rx() hands free-queue pages to the stack as skb fragments:

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

If a socket still holds those skbs across unregister_netdev(), does this
loop return pages to the allocator while the queued fragments still
reference them, and does skb_release_data() then underflow the refcount?

For what it is worth, the following patch in the series, "net: ethernet:
cortina: Manage RX buffers with page_pool", drops this loop and returns
only the fragments the driver still owns, with page_pool_destroy()
deferring the final teardown, so the behaviour does not survive to the end
of the series.

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