netdev - Re: [net-next 6/6] net: ravb: Use common defines for time stamping control

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: <81a21668-4886-40ad-9dc2-f6081396a94d@lunn.ch>
Date: Tue, 16 Sep 2025 16:49:33 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Paul Barker <paul@...rker.dev>, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Magnus Damm <magnus.damm@...il.com>,
	Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next 6/6] net: ravb: Use common defines for time stamping
 control

On Tue, Sep 16, 2025 at 03:08:48PM +0200, Niklas Söderlund wrote:
> On 2025-09-16 14:38:58 +0200, Andrew Lunn wrote:
> > > @@ -1010,18 +1009,27 @@ static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
> > >  				break;
> > >  			}
> > >  			skb_mark_for_recycle(skb);
> > > -			get_ts &= (q == RAVB_NC) ?
> > > -					RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
> > > -					~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
> > > -			if (get_ts) {
> > > -				struct skb_shared_hwtstamps *shhwtstamps;
> > > -
> > > -				shhwtstamps = skb_hwtstamps(skb);
> > > -				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> > > -				ts.tv_sec = ((u64) le16_to_cpu(desc->ts_sh) <<
> > > -					     32) | le32_to_cpu(desc->ts_sl);
> > > -				ts.tv_nsec = le32_to_cpu(desc->ts_n);
> > > -				shhwtstamps->hwtstamp = timespec64_to_ktime(ts);
> > > +
> > > +			if (priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE) {
> > > +				bool get_ts = false;
> > > +
> > > +				if (q == RAVB_NC)
> > > +					get_ts = priv->tstamp_rx_ctrl ==
> > > +						HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> > > +				else
> > > +					get_ts = priv->tstamp_rx_ctrl !=
> > > +						HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> > > +
> > > +				if (get_ts) {
> > > +					struct skb_shared_hwtstamps *shhwtstamps;
> > > +
> > > +					shhwtstamps = skb_hwtstamps(skb);
> > > +					memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> > > +					ts.tv_sec = ((u64)le16_to_cpu(desc->ts_sh) << 32)
> > > +						| le32_to_cpu(desc->ts_sl);
> > > +					ts.tv_nsec = le32_to_cpu(desc->ts_n);
> > > +					shhwtstamps->hwtstamp = timespec64_to_ktime(ts);
> > > +				}
> > 
> > This hunk is bigger than it needs to be because this block has been
> > indented further. Maybe keep get_ts as function scope, initialised to
> > false, so you don't need to touch this block?
> 
> Thanks for the suggestion. I could do that. What I like about this is 
> that it's immediately clear that all this depends on 
> priv->tstamp_rx_ctrl.

True.

As a reviewer, i was asking myself, has the code actually setting the
timestamp in the skbuf changed? Do i need to look at it in detail?
There should not be any need for it to change....

> I could break it out to a separate function if you prefer to reduce the 
> indentation level,

It is not really indentation level, but the fact it is in the hunk,
meaning should i look at it?

So maybe add a patch which moves the copying of the timestamp from the
descriptor into the skbuf into a helper. This patch then just calls
the helper, making this hunk much smaller and more obvious?

It does look correct as it is, but its just more effort to review.
Small, simple, obviously correct patches are what we want.

	Andrew