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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250414170215.4b94c934@kmaincent-XPS-13-7390>
Date: Mon, 14 Apr 2025 17:02:15 +0200
From: Kory Maincent <kory.maincent@...tlin.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Marcin Wojtas <marcin.s.wojtas@...il.com>,
 netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH RFC net-next 1/5] net: mvpp2: add support for hardware
 timestamps

On Mon, 14 Apr 2025 15:43:22 +0100
"Russell King (Oracle)" <linux@...linux.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@...linux.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@...linux.org.uk>  
> > 
> > Reviewed-by: Kory Maincent <kory.maincent@...tlin.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