| 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
| ||
|
Message-ID: <20251107191051.GB4126953@ragnatech.se>
Date: Fri, 7 Nov 2025 20:10:51 +0100
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Simon Horman <horms@...nel.org>
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,v2 7/7] net: ravb: Use common defines for time
stamping control
Hi Simon,
Thanks for your test of new tools!
On 2025-11-07 17:50:02 +0000, Simon Horman wrote:
> On Tue, Nov 04, 2025 at 11:24:20PM +0100, Niklas Söderlund wrote:
> > Instead of translating to/from driver specific flags for packet time
> > stamp control use the common flags directly. This simplifies the driver
> > as the translating code can be removed while at the same time making it
> > clear the flags are not flags written to hardware registers.
> >
> > The change from a device specific bit-field track variable to the common
> > enum datatypes forces us to touch the ravb_rx_rcar_hwstamp() in a non
> > trivial way. To make this cleaner and easier to understand expand the
> > nested conditions.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
>
> ...
>
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 5477bb5c69ae..1680e94b9242 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -950,13 +950,14 @@ static void ravb_rx_rcar_hwstamp(struct ravb_private *priv, int q,
> > struct ravb_ex_rx_desc *desc,
> > struct sk_buff *skb)
> > {
> > - u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
> > struct skb_shared_hwtstamps *shhwtstamps;
> > struct timespec64 ts;
> > + bool get_ts;
> >
> > - get_ts &= (q == RAVB_NC) ?
> > - RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
> > - ~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
> > + 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)
> > return;
>
> Hi Niklas,
>
> It is Friday evening and I'm exercising a new tool, so please forgive me if
> this analysis is wrong. But it seems that there are cases where there old
> bit-based logic and the new integer equality based logic don't match.
>
> 1. If q == RAVB_NC then previously timestamping would occur
> for HWTSTAMP_FILTER_ALL, because:
>
> (RAVB_TXTSTAMP_ENABLED | RAVB_RXTSTAMP_TYPE_ALL) &
> RAVB_RXTSTAMP_TYPE & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT =
> (0x10 | 0x6) & 0x06 & 0x02 = 0x2, which is non-zero.
>
> But with the new logic timestamping does not occur because:
>
> HWTSTAMP_FILTER_ALL == HWTSTAMP_FILTER_PTP_V2_L2_EVENT is false
>
> 2. If q != RAVB_NC then previously timestamping would not occur
> for HWTSTAMP_FILTER_NONE because:
>
> 0 & RAVB_RXTSTAMP_TYPE & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT = 0
>
> But with the new logic timestamping does occur because:
>
> HWTSTAMP_FILTER_NONE != HWTSTAMP_FILTER_PTP_V2_L2_EVENT is true
I must have written this code a late Friday night.. you are absolutely
correct! Those two error cases exists. I wrote a test program to really
verify it.
NG q: RAVB_BE old: 0=0, new=HWTSTAMP_FILTER_NONE=1
OK q: RAVB_NC old: 0=0, new=HWTSTAMP_FILTER_NONE=0
OK q: RAVB_BE old: RAVB_RXTSTAMP_TYPE_V2_L2_EVENT=0, new=HWTSTAMP_FILTER_PTP_V2_L2_EVENT=0
OK q: RAVB_NC old: RAVB_RXTSTAMP_TYPE_V2_L2_EVENT=1, new=HWTSTAMP_FILTER_PTP_V2_L2_EVENT=1
OK q: RAVB_BE old: RAVB_RXTSTAMP_TYPE_ALL=1, new=HWTSTAMP_FILTER_ALL=1
NG q: RAVB_NC old: RAVB_RXTSTAMP_TYPE_ALL=1, new=HWTSTAMP_FILTER_ALL=0
The correct conversion to int comparison are,
if (q == RAVB_NC)
get_ts = tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE;
else
get_ts = tstamp_rx_ctrl == HWTSTAMP_FILTER_ALL;
And yes, I did use my test to verify it :-)
OK q: RAVB_BE old: 0=0, new=HWTSTAMP_FILTER_NONE=0
OK q: RAVB_NC old: 0=0, new=HWTSTAMP_FILTER_NONE=0
OK q: RAVB_BE old: RAVB_RXTSTAMP_TYPE_V2_L2_EVENT=0, new=HWTSTAMP_FILTER_PTP_V2_L2_EVENT=0
OK q: RAVB_NC old: RAVB_RXTSTAMP_TYPE_V2_L2_EVENT=1, new=HWTSTAMP_FILTER_PTP_V2_L2_EVENT=1
OK q: RAVB_BE old: RAVB_RXTSTAMP_TYPE_ALL=1, new=HWTSTAMP_FILTER_ALL=1
OK q: RAVB_NC old: RAVB_RXTSTAMP_TYPE_ALL=1, new=HWTSTAMP_FILTER_ALL=1
I will submit a correction promptly.
>
> I came across this by chance because this patch is currently
> the most recent patch in net-next that touches C code. And I was
> exercising Claude Code with https://github.com/masoncl/review-prompts
> It reported the above and after significantly
> more thinking I've come to agree with it.
I'm happy you did! Thanks for spotting this.
>
> But it is Friday evening, so YMMV.
Obviously, but mostly on for the non-AI entities it seems :-)
>
> For reference, I've provided the text generated by Claude Code at the end of
> this email.
Seems like a very useful tool!
>
> ...
>
> > @@ -2446,15 +2437,13 @@ static int ravb_hwtstamp_set(struct net_device *ndev,
> > struct netlink_ext_ack *extack)
> > {
> > struct ravb_private *priv = netdev_priv(ndev);
> > - u32 tstamp_rx_ctrl = RAVB_RXTSTAMP_ENABLED;
> > - u32 tstamp_tx_ctrl;
> > + enum hwtstamp_rx_filters tstamp_rx_ctrl;
> > + enum hwtstamp_tx_types tstamp_tx_ctrl;
> >
> > switch (config->tx_type) {
> > case HWTSTAMP_TX_OFF:
> > - tstamp_tx_ctrl = 0;
> > - break;
> > case HWTSTAMP_TX_ON:
> > - tstamp_tx_ctrl = RAVB_TXTSTAMP_ENABLED;
> > + tstamp_tx_ctrl = config->tx_type;
> > break;
> > default:
> > return -ERANGE;
> > @@ -2462,14 +2451,12 @@ static int ravb_hwtstamp_set(struct net_device *ndev,
> >
> > switch (config->rx_filter) {
> > case HWTSTAMP_FILTER_NONE:
> > - tstamp_rx_ctrl = 0;
> > - break;
> > case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > - tstamp_rx_ctrl |= RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
> > + tstamp_rx_ctrl = config->rx_filter;
> > break;
> > default:
> > config->rx_filter = HWTSTAMP_FILTER_ALL;
> > - tstamp_rx_ctrl |= RAVB_RXTSTAMP_TYPE_ALL;
> > + tstamp_rx_ctrl = HWTSTAMP_FILTER_ALL;
> > }
> >
> > priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
> > --
> > 2.51.1
> >
> >
>
> Text generated by Claude Code:
>
> Is this logic translation correct? Looking at the old bit-field
> operations and new enum comparisons, the behavior seems to change in
> two cases:
>
> Old behavior (from ravb_hwtstamp_set):
> - FILTER_NONE stored as 0
> - FILTER_PTP_V2_L2_EVENT stored as 0x12 (ENABLED | TYPE_V2_L2)
> - FILTER_ALL stored as 0x16 (ENABLED | TYPE_ALL)
>
> Old ravb_rx_rcar_hwstamp for RAVB_NC queue:
> get_ts = (0x12 & 0x06) & 0x02 = 0x02 & 0x02 = 0x02 (timestamp)
> get_ts = (0x16 & 0x06) & 0x02 = 0x06 & 0x02 = 0x02 (timestamp)
>
> So the old code timestamps RAVB_NC queue packets for both
> FILTER_PTP_V2_L2_EVENT and FILTER_ALL.
>
> New ravb_rx_rcar_hwstamp for RAVB_NC queue with FILTER_ALL:
> get_ts = (HWTSTAMP_FILTER_ALL == HWTSTAMP_FILTER_PTP_V2_L2_EVENT)
> get_ts = (1 == 9) = false (no timestamp)
>
> Similarly for non-RAVB_NC queue, old behavior with FILTER_NONE:
> get_ts = (0 & 0x06) & ~0x02 = 0 & ~0x02 = 0 (no timestamp)
>
> New behavior for non-RAVB_NC queue with FILTER_NONE:
> get_ts = (HWTSTAMP_FILTER_NONE != HWTSTAMP_FILTER_PTP_V2_L2_EVENT)
> get_ts = (0 != 9) = true (timestamp)
>
> Can the refactoring introduce these two behavior changes?
>
> 1. RAVB_NC queue with FILTER_ALL: old code timestamps, new code doesn't
> 2. Non-RAVB_NC queue with FILTER_NONE: old code doesn't timestamp, new
> code does
--
Kind Regards,
Niklas Söderlund
Powered by blists - more mailing lists