Message-ID: <aQ4xSv9629XF-Bt3@horms.kernel.org>
Date: Fri, 7 Nov 2025 17:50:02 +0000
From: Simon Horman <horms@...nel.org>
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,v2 7/7] net: ravb: Use common defines for time
 stamping control

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 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.

But it is Friday evening, so YMMV.

For reference, I've provided the text generated by Claude Code at the end of
this email.

...

> @@ -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

Powered by blists - more mailing lists