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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250916130848.GD1045278@ragnatech.se>
Date: Tue, 16 Sep 2025 15:08:48 +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 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.

If I keep the chunk as-is it gives the impression there is some other 
condition other then priv->tstamp_rx_ctrl that could set get_ts and make 
a valid use-case of reading the timestamp from the descriptor.

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

static void ravb_rx_rcar_hwstamp(...)
{
    bool get_ts = false;

    ...

    if (get_ts) {
        ....
    }

}

static int ravb_rx_rcar(..)
{
    ...

    if (priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE)
        ravb_rx_rcar_hwstamp(...);

    ...
}

Would that work ?

> 
> 	Andrew

-- 
Kind Regards,
Niklas Söderlund