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

[‍김우석[학생](전자정보대학 전자공학과)]

Hi Théo,

Thanks for explaining the approach.

I found both issues while validating hardware timestamp configuration
on the Raspberry Pi 5's Ethernet interface for my research. I reproduced
them on the board and tested both fixes there, as described in the
cover letter. These were hardware-verified failures, rather than
cleanup based only on source inspection.

I've already applied the fixes locally on my Pi, so there is no
immediate urgency on my side. That said, the structure you suggested
looks like a cleaner way to handle the configuration path overall, and
I think it makes sense to take this opportunity to rework it in that
direction rather than keep the changes limited to the current fixes.

Rather than posting a v2 limited to the current two fixes, I'll prepare
a new series that refactors gem_set_hwtst() along those lines and
incorporates both fixes. I'll link back to this series for context and
validate the reworked path on the Pi 5.

Thanks,
Wooseok Kim

On Mon, 21 Sep 2026 15:11:13 +0200, "Théo Lebrun"
<theo.lebrun@xxxxxxxxxxx> wrote:
> 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