RE: [EXTERNAL] Re: [PATCH net-next v3 10/12] net: atlantic: add AQC113 PTP hardware ops in hw_atl2

[Sukhdeep Soni [C]]

On May 22, 2026 Jakub Kicinski wrote:

> net: atlantic: add AQC113 PTP hardware ops in hw_atl2
>
> This commit adds the hardware-layer PTP implementation for AQC113 (Antigua).
> It includes TSG clock programming, RX/TX timestamping mechanisms, and PPS
> output generation via the TSG pulse generator.
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> index 39d84576ca519..d6a97a1ec5c65 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
[ ... ]
> @@ -888,6 +924,151 @@ static struct aq_stats_s *hw_atl2_utils_get_hw_stats(struct aq_hw_s *self)
[ ... ]
> +static u64 hw_atl2_hw_ring_tx_ptp_get_ts(struct aq_ring_s *ring)
> +{
> +	struct hw_atl2_txts_s *txts;
> +	u32 ctrl;
> +
> +	txts = (struct hw_atl2_txts_s *)&ring->dx_ring[ring->sw_head *
> +						HW_ATL2_TXD_SIZE];
> +	/* DD + TS_VALID */
> +	ctrl = READ_ONCE(txts->ctrl);
> +	if ((ctrl & HW_ATL2_TXTS_DD) && (ctrl & HW_ATL2_TXTS_TS_VALID)) {
>
> [Severity: High]
> Will this fail to recognize valid TX timestamps on big-endian architectures?
>
> The struct hw_atl2_txts_s is a hardware DMA structure, and the ctrl field
> is defined as a native u32. When reading it via READ_ONCE(txts->ctrl) on a
> big-endian system, the little-endian word is loaded into the CPU register
> natively without byte-swapping, so ctrl & HW_ATL2_TXTS_DD will test the
> wrong bit.
>
> Should this field be defined as __le32 and accessed with le32_to_cpu()
> instead?

Correct - this is addressed in v4: ctrl is declared as __le32 in
hw_atl2_txts_s and accessed with le32_to_cpu(READ_ONCE(txts->ctrl))
in the code, so bit positions are correct on big-endian architectures.

> +		dma_rmb();
> +		return le64_to_cpu(txts->ts);
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int hw_atl2_adj_sys_clock(struct aq_hw_s *self, s64 delta)
> +{
> +	s8 clk_sel = READ_ONCE(self->clk_select);
> +
> +	if (clk_sel < 0)
> +		return -ENODEV;
> +	if (delta >= 0)
> +		hw_atl2_tsg_clock_add(self, clk_sel, (u64)delta);
> +	else
> +		hw_atl2_tsg_clock_sub(self, clk_sel, (u64)(-delta));
> +
> +	return 0;
> +}
> +
> +static int hw_atl2_adj_clock_freq(struct aq_hw_s *self, s32 ppb)
> +{
> +	u32 freq = AQ2_HW_PTP_COUNTER_HZ;
> +	u64 divisor = 0, base_ns;
> +	u32 nsi_frac = 0, nsi;
> +	u32 nsi_rem;
> +	s8 clk_sel;
> +
> +	base_ns = div_u64((u64)((s64)ppb + NSEC_PER_SEC) * NSEC_PER_SEC, freq);
> +	nsi = (u32)div_u64_rem(base_ns, NSEC_PER_SEC, &nsi_rem);
> +	if (nsi_rem != 0) {
> +		divisor = div_u64(mul_u32_u32(NSEC_PER_SEC, NSEC_PER_SEC),
> +				  nsi_rem);
> +		nsi_frac = (u32)div64_u64(AQ_FRAC_PER_NS * NSEC_PER_SEC,
> +					  divisor);
> +	}
> +
> +	clk_sel = READ_ONCE(self->clk_select);
> +	if (clk_sel < 0)
> +		return -ENODEV;
> +	hw_atl2_tsg_clock_increment_set(self, clk_sel, nsi, nsi_frac);
>
> [Severity: High]
> Could these two clock adjustment functions race when writing to the hardware
> command registers?
>
> The core PTP operations execute adjfine holding
> mutex_lock(&aq_nic->fwreq_mutex) and execute adjtime holding
> spin_lock_irqsave(&aq_ptp->ptp_lock). Since hw_atl2_adj_sys_clock()
> and hw_atl2_adj_clock_freq() are protected by different locks, can
> concurrent calls from userspace interleave writes to the same
> CLOCK_MODIF_CTRL hardware register and corrupt the PTP hardware clock state?

The locking asymmetry between adjtime (ptp_lock) and adjfine (fwreq_mutex)
is pre-existing in aq_ptp.c and applies to both A1 and A2 hardware paths. Unifying
them under a single lock would require refactoring the entire aq_ptp.c locking model,
which is outside the scope of this series. This can be addressed as a follow-up cleanup.

> +
> +	return 0;
> +}
> [ ... ]
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils.h
> index c84955bc14ae4..ee7f444f6d07d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils.h
> @@ -8,6 +8,16 @@
>  
>  #include "aq_hw.h"
>  
> +/* Hardware tx launch time descriptor */
> +struct hw_atl2_txts_s {
> +	__le64 ts;
> +	u32 ctrl;
>
> [Severity: High]
> Does this ctrl field need to be __le32? Since this is a hardware DMA
> structure, declaring it as a native u32 causes endianness issues when read
> on big-endian systems, as mentioned in the hw_atl2_hw_ring_tx_ptp_get_ts()
> function above.

ctrl will be declared as __le32 in the code, consistent with
the __le64 ts field above it. This will be addressed together with the
le32_to_cpu(READ_ONCE(txts->ctrl)) fix in hw_atl2_hw_ring_tx_ptp_get_ts().

> +	u32 reserved;
> +};