Re: [PATCH net-next v6 07/16] net: macb: introduce macb_context struct for buffer management

[Théo Lebrun]

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

> commit 4cc5f75700ce9248e0d2a9728229a16b10ce9942
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: introduce macb_context struct for buffer management
>
> This moves all per-queue ring/buffer state and the ring size parameters out
> of struct macb / struct macb_queue into a new struct macb_context that is
> allocated in macb_open() / at91ether_open() and freed in the close paths.
> The ring sizes requested by userspace are now remembered in
> bp->configured_{rx,tx}_ring_size, and all data path code reaches its
> buffers through bp->ctx.
>
> A few questions below.
>
> > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> > index 9857df5b57f09..452b2c8f8641f 100644
> > --- a/drivers/net/ethernet/cadence/macb.h
> > +++ b/drivers/net/ethernet/cadence/macb.h
> > @@ -1272,21 +1272,10 @@ struct macb_queue {
> >  
> >  	/* Lock to protect tx_head and tx_tail */
> >  	spinlock_t		tx_ptr_lock;
> > -	unsigned int		tx_head, tx_tail;
> > -	struct macb_dma_desc	*tx_ring;
> > -	struct macb_tx_skb	*tx_skb;
> > -	dma_addr_t		tx_ring_dma;
> >  	struct work_struct	tx_error_task;
> >  	bool			txubr_pending;
> >  	struct napi_struct	napi_tx;
> >  
> > -	dma_addr_t		rx_ring_dma;
> > -	dma_addr_t		rx_buffers_dma;
> > -	unsigned int		rx_tail;
> > -	unsigned int		rx_prepared_head;
> > -	struct macb_dma_desc	*rx_ring;
> > -	struct sk_buff		**rx_skbuff;
> > -	void			*rx_buffers;
> >  	struct napi_struct	napi_rx;
> >  	struct queue_stats stats;
> >  };
>
> [Severity: Low]
> [Finding: main-2-2]
> [Sources: opus-5]
> This isn't a bug, but the comment above tx_ptr_lock in struct macb_queue
> still says:
>
> 	/* Lock to protect tx_head and tx_tail */
>
> while tx_head and tx_tail are removed from that struct by this hunk. The
> protected fields are now head and tail in struct macb_txq, living in the
> separately allocated bp->ctx->txq[q]. Should the comment be updated to name
> where the protected state now lives, given the lock and the data it protects
> are in two objects with different lifetimes?

Agreed. Changing to the following for clarification.

   /* Lock to protect ctx->txq[q].head and ctx->txq[q].tail */

If we grep for tx_head we still find:

 - tx_head stack variable in macb_tx_map(). Leaving alone.

 - A printk() call in macb_start_xmit(), changing from

      netdev_dbg(netdev, "tx_head = %u, tx_tail = %u\n",
                 txq->head, txq->tail);

   to

      netdev_dbg(netdev, "txq head = %u, txq tail = %u\n",
                 txq->head, txq->tail);

> > @@ -3674,8 +3761,8 @@ static void macb_get_ringparam(struct net_device *netdev,
> >  	ring->rx_max_pending = MAX_RX_RING_SIZE;
> >  	ring->tx_max_pending = MAX_TX_RING_SIZE;
> >  
> > -	ring->rx_pending = bp->rx_ring_size;
> > -	ring->tx_pending = bp->tx_ring_size;
> > +	ring->rx_pending = bp->configured_rx_ring_size;
> > +	ring->tx_pending = bp->configured_tx_ring_size;
> >  }
>
> [Severity: Low]
> [Finding: main-2-0]
> [Sources: opus-5]
> Does this report ring sizes that at91ether devices never use?
>
> emac_config sets MACB_CAPS_MACB_IS_EMAC and .init = at91ether_init, which
> installs netdev->ethtool_ops = &macb_ethtool_ops, so .get_ringparam =
> macb_get_ringparam and .set_ringparam = macb_set_ringparam are reachable on
> those devices. macb_probe() now unconditionally sets
> configured_rx_ring_size / configured_tx_ring_size to
> DEFAULT_RX_RING_SIZE / DEFAULT_TX_RING_SIZE, so ethtool -g reports 512/512.
>
> But at91ether_open() only does kzalloc_obj() for bp->ctx and never copies
> bp->configured_* into ctx->rx_ring_size / ctx->tx_ring_size (they stay 0),
> and the at91ether path hard-codes its ring size in
> at91ether_alloc_coherent() / at91ether_start() / at91ether_rx():
>
> 	rxq->ring = dma_alloc_coherent(&bp->pdev->dev,
> 				       (AT91ETHER_MAX_RX_DESCR *
> 					macb_dma_desc_get_size(bp)),
> 				       &rxq->ring_dma, GFP_KERNEL);
>
> with AT91ETHER_MAX_RX_DESCR == 9. Before this patch the callback returned
> bp->rx_ring_size, which was only set by macb_init_dflt() and so reported 0
> for at91ether. Now it reports 512, and ethtool -G on a down at91ether
> interface stores the new value and returns success while nothing in the
> at91ether path consults it. Should macb_ethtool_ops leave these callbacks
> out for MACB_CAPS_MACB_IS_EMAC, or should at91ether_open() set
> ctx->rx_ring_size / ctx->tx_ring_size to what it actually allocates?

So to clarify those paragraphs, on EMAC:
 - RX:
    - Ring buffer size is 9.
    - Before we reported rx_ring_size=0 to userspace.
    - Now we report rx_ring_size=512 to userspace.
 - TX:
    - No ring buffer so 1.
    - Before we reported rx_ring_size=0 to userspace.
    - Now we report rx_ring_size=512 to userspace.
 - The message mentions that `ethtool -G` now changes the value without
   error but nothing uses it in the EMAC case. That was already true
   previously, it didn't change.

To my eyes we need two changes:
#1. Report proper value to userspace, by setting
    bp->configured_{rx,tx}_ring_size at probe to hardcoded values if EMAC.
#2. Refuse set_ringparam op if EMAC. I looked at all other
    macb_ethtool_ops wondering if we'd be better off with a different
    vtable, but no. Those are the only broken ones on EMAC.

Part #1 can be squashed into this commit (which introduces
bp->configured_*) but part #2 deserves its separate patch.

I want to acknowledge I'm already at 16 out of 15 max patches in net
land. I see nothing I can split up easily, and most of the code is well
reviewed already. I'll still append new patch as the doc seems to
indicate it is a soft rule:

> Avoid sending series longer than 15 patches.

I don't see many other options. Even if I sent part #2 standalone I'd
still be at 16 patches. Please tell me if I'm wrong in this!

Thanks,

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