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

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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