Re: [PATCH net RESEND 1/2] net: macb: Preserve one-step mode on rejected timestamp requests

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Woo-seok Kim,

On Sun Sep 20, 2026 at 11:04 AM CEST, Woo-seok Kim wrote:
> Following up on my September 15 reply. I noticed that the series is
> still marked "Changes Requested" in Patchwork.
>
> This series fixes rejected timestamp requests changing the TX mode and
> the two PTPv1 RX filters disabling receive timestamping. As explained in
> my reply, the additional issues predate this series and are not made
> worse by it. I propose keeping those fixes separate so that this series
> remains focused on the two reported bugs.
>
> Could you please reconsider the Changes Requested status in light of
> that reply and continue reviewing the series as posted?

To me it all depends on the intent behind your series. If you have faced
this bug in practice and your patches are intended to fix your usecase
and prevent others from facing it, then we can take your patches as-is.

If they are edge-cases cleanup patches and you never encountered the
issue (maybe because you don't have hardware), then either
 - the patch won't be accepted because it's overall churn or
 - you fix the full sequence fully and we consider it a noticeable
   improvement and take that series.

About "the proper way(TM)", I expect something like:

int gem_set_hwtst(struct net_device *netdev,
		  struct kernel_hwtstamp_config *tstamp_config,
		  struct netlink_ext_ack *extack)
{
	struct macb *bp = netdev_priv(netdev);
	u32 regval;

	if (!macb_dma_ptp(bp))
		return -EOPNOTSUPP;

	// Step (1): tstamp_config->tx_type validation and precomputing
	//           of TXBDCTRL and NCR values/masks.

	// Step (2): same for tstamp_config->rx_filter.

	// Step (3): read-modify-write NCR, writel TXBDCTRL & RXBDCTRL.

	bp->tstamp_config = *tstamp_config;

	return 0;
}

Improvements:
 - we remove writel from the validation code
 - we write to NCR once and not twice
 - we don't have a tiny gem_ptp_set_ts_mode() function that returns an
   int for no reason
 - also NCR RMW probably deserves some atomicity through locking

Thanks,

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