Re: [PATCH RFC net-next 1/5] net: mvpp2: add support for hardware timestamps

[Kory Maincent <kory.maincent@bootlin.com>]

On Mon, 14 Apr 2025 15:43:22 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Apr 14, 2025 at 02:51:50PM +0200, Kory Maincent wrote:
> > On Fri, 11 Apr 2025 22:26:31 +0100
> > Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >   
> > > Add support for hardware timestamps in (e.g.) the PHY by calling
> > > skb_tx_timestamp() as close as reasonably possible to the point that
> > > the hardware is instructed to send the queued packets.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>  
> > 
> > Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
> >   
> > > ---
> > >  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index
> > > 416a926a8281..e3f8aa139d1e 100644 ---
> > > a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++
> > > b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -4439,6 +4439,8 @@
> > > static netdev_tx_t mvpp2_tx(struct sk_buff *skb, struct net_device *dev)
> > > txq_pcpu->count += frags; aggr_txq->count += frags;
> > >  
> > > +		skb_tx_timestamp(skb);
> > > +
> > >  		/* Enable transmit */
> > >  		wmb();
> > >  		mvpp2_aggr_txq_pend_desc_add(port, frags);  
> > 
> > Small question for my curiosity here. Shouldn't we move the
> > skb_tx_timestamp() call after the memory barrier for a better precision or
> > is it negligible?  
> 
> Depends what the wmb() is there for, which is entirely undocumented.
> 
> mvpp2_aggr_txq_pend_desc_add() uses writel(), which is itself required
> to ensure that writes to memory before the writel() occurs are visible
> to DMA agents. So, the wmb() there shouldn't be necessary if all
> that's going on here is to ensure that the packet is visible to the
> buffer manager hardware.
> 
> On arm64, that's __io_wmb(), which becomes __dma_wmb() and ultimately
> "dmb oshst". wmb() on the other hand is a heavier barrier, "dsb st".
> This driver ends up doing both, inexplicably.

Indeed and that is the same case for all the wmb() call in that file. 
Not sure two consecutive memory barrier are useful here, indeed a bit of doc
would have been great!
Thanks!

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com