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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
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?

> +		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?

> +
> +	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.

> +	u32 reserved;
> +};