Re: [PATCH net-next v6 11/16] net: macb: change function signatures to take contexts

[Théo Lebrun]

Replying to net sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260731-macb-context-v6-0-49d5a1439d48%40bootlin.com

> commit 8d7bc94670a7294669d008889ae3d43a6f139a9f
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: change function signatures to take contexts
>
> This converts the macb ring/descriptor helpers and the alloc/free/init
> paths so they take a struct macb_context *ctx (plus a queue index where a
> struct macb_queue * was used) instead of struct macb *bp, in preparation
> for having more than one context around. Callers in the normal datapath
> now latch bp->ctx into a local variable and pass it down.
>
> A few questions below about the conversion.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 97c8374317ceb..8f8c952ffc74a 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
> > @@ -1233,14 +1238,15 @@ static int macb_halt_tx(struct macb *bp)
> >  					bp, TSR);
> >  }
> >  
> > -static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budget)
> > +static void macb_tx_unmap(const struct macb_info *info,
> > +			  struct macb_tx_skb *tx_skb, int budget)
>
> [Severity: Low]
> [Finding: fable-5-1-2]
> [Sources: fable-5, opus-5]
> The commit message presents an exhaustive enumeration:
>
>     Full list:
>
>        macb_adj_dma_desc_idx()
>        macb_tx_ring_wrap()
>        ...
>        macb_init_rings()
>
> macb_tx_unmap() also changes signature here, and all of its call sites are
> updated (macb_tx_error_task(), macb_tx_complete(), macb_tx_map() pass
> &bp->info, macb_free() passes ctx->info), but it is not in that list.
>
> It is also the one converted helper that does not follow the stated rule:
>
>     To reduce bug surface area, taint those functions to *only* take a
>     context `struct macb_context *ctx` and no `struct macb *bp`.
>
> macb_tx_unmap() takes const struct macb_info *info instead. Could the list
> be extended and the macb_info choice mentioned, so the exception is
> explicit?

macb_tx_unmap() should have been updated in [10/16] rather than [11/16].
I'm surprised the LLM didn't notice that; probably has a rather limited
view of the series, on purpose. Fixed for next revision.

> > @@ -5026,7 +5051,7 @@ static int at91ether_alloc_coherent(struct macb *bp)
> >  
> >  	rxq->ring = dma_alloc_coherent(&bp->pdev->dev,
> >  				       (AT91ETHER_MAX_RX_DESCR *
> > -					macb_dma_desc_get_size(&bp->info)),
> > +				       macb_dma_desc_get_size(&bp->info)),
> >  				       &rxq->ring_dma, GFP_KERNEL);
>
> [Severity: Low]
> [Finding: fable-5-1-2]
> [Sources: fable-5, opus-5]
> This hunk changes only leading whitespace, nothing on the line is affected
> by the context conversion, and it is not mentioned in the commit message.
> The new indentation also no longer lines up with the open parenthesis of
> (AT91ETHER_MAX_RX_DESCR *, which the original did. Could this hunk be
> dropped?

Removed.

Thanks,

-- 
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com