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] [day] [month] [year] [list]

Message-ID: <20250916205311.GC1812504@ragnatech.se>
Date: Tue, 16 Sep 2025 22:53:11 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Andrew Lunn <andrew@...n.ch>
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 2025-09-16 16:49:33 +0200, Andrew Lunn wrote:
> 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.

I'm with you on creating small and easy to review patches. I will do as 
you suggest and move this out to a function in a separate patch. Thanks 
for the feedback.

> 
> 	Andrew

-- 
Kind Regards,
Niklas Söderlund