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

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:

quoted

From: Kim Wooseok <redacted>

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

[...]

quoted

 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@bootlin.com>

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

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