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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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.

It would help if people would document what purpose a barrier exists
for.

Without knowing what the wmb() is there for, I wouldn't like to move
it the other side.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!