Re: [PATCH net-next 1/4] net: macb: Preserve timestamp settings on rejected requests

[Théo Lebrun]

Hello Théo,

On Tue Sep 22, 2026 at 9:01 PM CEST, Théo Lebrun wrote:
> Hello Kim,
>
> On Tue Sep 22, 2026 at 11:10 AM CEST, Kim Wooseok via B4 Relay wrote:
>> From: Kim Wooseok <5mghybrid@xxxxxxxxx>
>>
>> gem_set_hwtst() can reject a request after changing the TX one-step
>> setting, because it programs the TX mode before checking the RX
>> filter. The call returns -ERANGE, but the hardware may no longer match
>> the cached configuration.
>>
>> Validate both settings first and keep the adjusted RX filter local
>> until validation succeeds. Then apply the register settings and update
>> the configuration. A rejected request now leaves the hardware, the
>> caller's settings and the cached configuration unchanged.
>>
>> Protect the NCR read-modify-write with bp->lock, keeping the descriptor
>> writes and cache update in the same section. With the register writes
>> now in the setter, remove gem_ptp_set_one_step_sync() and
>> gem_ptp_set_ts_mode().
>>
>> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
>> Assisted-by: GPT-6 Astra
>> Signed-off-by: Kim Wooseok <5mghybrid@xxxxxxxxx>
>> ---
> [...]
>>  int gem_set_hwtst(struct net_device *netdev,
>>  		  struct kernel_hwtstamp_config *tstamp_config,
>>  		  struct netlink_ext_ack *extack)
>>  {
>> +	u32 ncr_mask = 0;
>>  	enum macb_bd_control tx_bd_control = TSTAMP_DISABLED;
>>  	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
>> +	int rx_filter = tstamp_config->rx_filter;
>>  	struct macb *bp = netdev_priv(netdev);
>> +	unsigned long flags;
>> +	u32 ncr_bits = 0;
>>  	u32 regval;
>
> Same remark as Nicolai (no surprise there). With that
>
> Reviewed-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>

Sorry for the two stage review, I had forgotten that aspect on first
review. I would prefer simpler code:

> @@ -424,18 +406,17 @@ int gem_set_hwtst(struct net_device *netdev,
>     case HWTSTAMP_TX_OFF:
>        break;
>     case HWTSTAMP_TX_ONESTEP_SYNC:
> -      gem_ptp_set_one_step_sync(bp, 1);
> -      tx_bd_control = TSTAMP_ALL_FRAMES;
> -      break;
> +      ncr_bits |= MACB_BIT(OSSMODE);
> +      fallthrough;
>     case HWTSTAMP_TX_ON:
> -      gem_ptp_set_one_step_sync(bp, 0);
> +      ncr_mask |= MACB_BIT(OSSMODE);
>        tx_bd_control = TSTAMP_ALL_FRAMES;
>        break;
>     default:
>        return -ERANGE;
>     }

Let's avoid the fallthrough. Simple & easy to read:

	case HWTSTAMP_TX_ONESTEP_SYNC:
		ncr_bits |= MACB_BIT(OSSMODE);
		ncr_mask |= MACB_BIT(OSSMODE);
		tx_bd_control = TSTAMP_ALL_FRAMES;
		break;
	case HWTSTAMP_TX_ON:
		ncr_mask |= MACB_BIT(OSSMODE);
		tx_bd_control = TSTAMP_ALL_FRAMES;
		break;

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com