Re: [PATCH net-next v2 11/11] net: ethernet: cortina: Use guard helpers for locking

[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 patch goes against netdev's documented policy on cleanup.h
  and on standalone clean-ups.
- [Low] gmac_rx() now mixes goto-based cleanup with a cleanup.h helper,
  which subsystem/cleanup.md and include/linux/cleanup.h (lines 148-153)…

--

net: ethernet: cortina: Use guard helpers for locking

This commit converts the spinlock lock and unlock pairs in the Gemini
ethernet driver to guard() and scoped_guard() from linux/cleanup.h. The
goto based unlock path in geth_freeq_claim() is replaced with early
returns. No functional change is intended.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 910de9925949..4fdd7478601f 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -790,10 +778,11 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
>  
>  	index = geth_freeq_mapping_index(geth, mapping);
>  
> -	spin_lock_irqsave(&geth->freeq_lock, flags);
> +	guard(spinlock_irqsave)(&geth->freeq_lock);
> +
>  	gpage = xa_load(&geth->freeq_mappings, index);
>  	if (!gpage || !gpage->page || !gpage->fragments)
> -		goto err_unlock;
> +		return NULL;

[Severity: Low]
Does this conversion fit the netdev guidance on cleanup.h?
Documentation/process/maintainer-netdev.rst says:

  Use of guard() is discouraged within any function longer than 20 lines,
  scoped_guard() is considered more readable. Using normal lock/unlock is
  still (weakly) preferred.

geth_freeq_claim() is about 46 lines long. Its function-scope guard()
covers xa_load(), xa_erase(), dma_unmap_single_attrs() and put_page().
There are also two early return NULL exits, which now release freeq_lock
implicitly.

gmac_update_hw_stats(), gmac_enable_irq() and the gemini_port_irq()
hardirq handler are also longer than 20 lines and now use a
function-scope guard().

The same document also says:

  Netdev discourages patches which perform simple clean-ups, which are not
  in the context of other work.

The commit message only says "Use cleanup guard helpers to scope the
driver's spinlocks automatically". The patch has no functional change, and
it touches the RX and IRQ paths in gmac_rx(), gmac_irq() and
gemini_port_irq(). That may make stable backports harder.

Would it be better to keep the explicit spin_lock_irqsave() and
spin_unlock_irqrestore() calls in the longer functions? Or use
scoped_guard() there, or fold this into other work on the driver?

[ ... ]

> @@ -1622,17 +1593,16 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,

[ ... ]

> -	spin_lock_irqsave(&geth->irq_lock, flags);
> -	rw.bits32 = readl(ptr_reg);
> -	/* Reset interrupt as all packages until here are taken into account */
> -	writel(DEFAULT_Q0_INT_BIT << netdev->dev_id,
> -	       geth->base + GLOBAL_INTERRUPT_STATUS_1_REG);
> -	spin_unlock_irqrestore(&geth->irq_lock, flags);
> +	scoped_guard(spinlock_irqsave, &geth->irq_lock) {
> +		rw.bits32 = readl(ptr_reg);
> +		/* Reset interrupt as all packages until here are taken into account */
> +		writel(DEFAULT_Q0_INT_BIT << netdev->dev_id,
> +		       geth->base + GLOBAL_INTERRUPT_STATUS_1_REG);
> +	}
>  
>  	r = rw.bits.rptr;
>  	w = rw.bits.wptr;

[Severity: Low]
Does this mix goto based cleanup with a cleanup.h helper in gmac_rx()?

Before this patch gmac_rx() used only gotos. Its RX loop already relies on
goto based cleanup through the err_length, err_drop and next_desc labels:

err_drop:
		if (skb) {
			napi_free_frags(&port->napi);
			skb = NULL;
			frag_nr = 0;
		}

		if (page)
			put_page(page);

include/linux/cleanup.h says:

  the expectation is that usage of "goto" and cleanup helpers is never
  mixed in the same function. I.e. for a given routine, convert all
  resources that need a "goto" cleanup to scope-based cleanup, or
  convert none of them.

Nothing goes wrong today. The scoped_guard() block closes before the while
loop, and none of the gotos cross its scope.

If the locked section were later moved into the loop, or changed to
guard(), those gotos would jump across a cleanup scope.

Could gmac_rx() keep the explicit spin_lock_irqsave() and
spin_unlock_irqrestore() pair? Another option is to move the locked rwptr
read and interrupt ack into a small helper.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org