Re: [PATCH net-next v4 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 patch wires the AQC113 (Antigua) PTP hardware ops into hw_atl2:
TSG clock setup/adjust, PTP ring init, TX timestamp readback from
descriptor writeback, RX timestamp extraction from packet trailer, and
GPIO pulse / PPS output. It also reduces HW_ATL2_RXBUF_MAX from 192 to
172.

[Low]
On the commit message itself, a couple of descriptions look like they
do not quite match the code that landed in this patch:

   - hw_atl2.h/hw_atl2_utils.h/hw_atl2_internal.h: export PTP function
     prototypes, per-instance aq_ptp_offset arrays, RX timestamp size

Every new hw_atl2 PTP function is static in hw_atl2.c and is reached
only through hw_atl2_ops, so are any prototypes actually being exported
from the headers?  The new HW_ATL2_PTP_OFFSET_* additions in hw_atl2.h
are also individual scalar #defines, not arrays — should the wording be
adjusted?

   - hw_atl2.c: implement hw_atl2_enable_ptp() to program TSG clocks,
     PTP TC receive path and filter ART entries, and re-enable PTP
     state after hardware reset.

The hw_atl2_enable_ptp() body only resets/enables the TSG clocks,
optionally calls hw_atl2_adj_clock_freq(), and toggles
hw_atl2_tpb_tps_highest_priority_tc_enable_set().  Is the "PTP TC
receive path and filter ART entries" claim describing this patch, or
work that lives in another patch in the series?

> 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 6d9ec4072d43..e94627c36163 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c

[ ... ]

> @@ -127,6 +137,7 @@ static u32 hw_atl2_sem_act_rslvr_get(struct aq_hw_s *self)
>  static int hw_atl2_hw_reset(struct aq_hw_s *self)
>  {
>  	struct hw_atl2_priv *priv = self->priv;
> +	s8 clk_sel;
>  	int err;
>  	int i;
>  
> @@ -144,6 +155,13 @@ static int hw_atl2_hw_reset(struct aq_hw_s *self)
>  		priv->l3l4_filters[i].l4_index = -1;
>  	}
>  
> +	clk_sel = READ_ONCE(self->clk_select);
> +	if (clk_sel != -1)
> +		hw_atl2_enable_ptp(self,
> +				   clk_sel,
> +				   aq_utils_obj_test(&self->flags, AQ_HW_PTP_AVAILABLE) ?
> +				   1 : 0);
> +

[Medium]
Where is self->clk_select initialised to -1?

struct aq_hw_s is allocated through kzalloc_obj() in aq_pci_func.c, so
clk_select starts as 0, and 0 is also a valid clock index
(ATL_TSG_CLOCK_SEL_0).  On the very first aq_nic_init()->hw_reset()
the test:

	if (clk_sel != -1)
		hw_atl2_enable_ptp(self, clk_sel, ...);

evaluates true with clk_sel == 0, and AQ_HW_PTP_AVAILABLE is not set
yet at this point in the series (aq_ptp_init() in this tree only sets
it for the a1 PTP path), so hw_atl2_enable_ptp(self, 0, 0) gets
unconditionally called for every hw_atl2 chip including aqc115c and
aqc116c.

That call then does:

	hw_atl2_tsg_clock_reset(self, 0);
	hw_atl2_tsg_clock_en(self, 1, 0);
	hw_atl2_tsg_clock_en(self, 0, 0);
	hw_atl2_tpb_tps_highest_priority_tc_enable_set(self, 0);

on parts that may have no PTP setup at all.  Should the -1 sentinel
initialisation that lives in a later patch in the series be folded
into this commit so each commit is self-contained for bisection?

>  	self->aq_fw_ops->set_state(self, MPI_RESET);
>  
>  	err = aq_hw_err_from_flags(self);
> @@ -725,14 +743,32 @@ static int hw_atl2_hw_ring_rx_init(struct aq_hw_s *self,
>  				   struct aq_ring_s *aq_ring,
>  				   struct aq_ring_param_s *aq_ring_param)
>  {
> -	return hw_atl_b0_hw_ring_rx_init(self, aq_ring, aq_ring_param);
> +	int res = hw_atl_b0_hw_ring_rx_init(self, aq_ring, aq_ring_param);
> +
> +	if (!res && aq_ptp_ring(aq_ring->aq_nic, aq_ring)) {
> +		int rx_err = hw_atl2_hw_rx_ptp_ring_init(self, aq_ring);
> +
> +		if (!res)
> +			res = rx_err;
> +	}
> +
> +	return res;
>  }
>  
>  static int hw_atl2_hw_ring_tx_init(struct aq_hw_s *self,
>  				   struct aq_ring_s *aq_ring,
>  				   struct aq_ring_param_s *aq_ring_param)
>  {
> -	return hw_atl_b0_hw_ring_tx_init(self, aq_ring, aq_ring_param);
> +	int res = hw_atl_b0_hw_ring_tx_init(self, aq_ring, aq_ring_param);
> +
> +	if (!res && aq_ptp_ring(aq_ring->aq_nic, aq_ring)) {
> +		int tx_err = hw_atl2_hw_tx_ptp_ring_init(self, aq_ring);
> +
> +		if (!res)
> +			res = tx_err;
> +	}
> +
> +	return res;
>  }

[Low]
The inner if (!res) check is provably always true here — the outer
if (!res && aq_ptp_ring(...)) already established res == 0 for the
whole block:

	if (!res && aq_ptp_ring(aq_ring->aq_nic, aq_ring)) {
		int rx_err = hw_atl2_hw_rx_ptp_ring_init(self, aq_ring);

		if (!res)         /* always true */
			res = rx_err;
	}

Was the intent to also call the PTP ring init when the b0 init had
already failed (and only then preserve the first error), or is the
inner test simply redundant and meant to be a plain res = rx_err?

[ ... ]

> @@ -892,6 +928,151 @@ static struct aq_stats_s *hw_atl2_utils_get_hw_stats(struct aq_hw_s *self)
>  	return &self->curr_stats;
>  }
>  
> +static void hw_atl2_enable_ptp(struct aq_hw_s *self,
> +			       unsigned int param, int enable)
> +{
> +	WRITE_ONCE(self->clk_select, (s8)param);
> +
> +	/* enable tsg counter */
> +	hw_atl2_tsg_clock_reset(self, self->clk_select);
> +	hw_atl2_tsg_clock_en(self, !self->clk_select, enable);
> +	hw_atl2_tsg_clock_en(self, self->clk_select, enable);
> +
> +	if (enable)
> +		hw_atl2_adj_clock_freq(self, 0);
> +
> +	hw_atl2_tpb_tps_highest_priority_tc_enable_set(self, enable);
> +}

[Low]
Is mixing one WRITE_ONCE store with three plain reads of the same
field intentional?

The other accessors in this file (hw_atl2_hw_reset(), aq_get_ptp_ts(),
hw_atl2_adj_sys_clock(), hw_atl2_adj_clock_freq(),
hw_atl2_hw_get_clk_sel()) consistently use READ_ONCE(self->clk_select),
suggesting the field is treated as concurrent state.  Here the three
plain reads after WRITE_ONCE() defeat that protocol and KCSAN will
flag the inconsistent annotations.

Would using the local param value (which is what was just stored) be
clearer, e.g.:

	s8 sel = (s8)param;

	WRITE_ONCE(self->clk_select, sel);
	hw_atl2_tsg_clock_reset(self, sel);
	hw_atl2_tsg_clock_en(self, !sel, enable);
	hw_atl2_tsg_clock_en(self, sel, enable);

> +
> +static void aq_get_ptp_ts(struct aq_hw_s *self, u64 *stamp)
> +{
> +	s8 clk_sel = READ_ONCE(self->clk_select);
> +
> +	if (clk_sel < 0)
> +		return;
> +	if (stamp)
> +		*stamp = hw_atl2_tsg_clock_read(self, clk_sel);
> +}

[Medium]
Can this leave *stamp uninitialised on the early return?

The existing callers in aq_ptp.c declare an uninitialised stack
variable and pass its address straight in, expecting the callee to
populate it:

aq_ptp_gettime():
	u64 ns;

	aq_ptp_get_sync_time_from_pkt(aq_nic, NULL, &ns);
	...
	hw_get_ptp_ts() variants are called the same way

aq_ptp_settime():
	u64 now;

	aq_nic->aq_hw_ops->hw_get_ptp_ts(aq_nic->aq_hw, &now);

aq_ptp_pps_pin_configure():
	u64 start;

	aq_nic->aq_hw_ops->hw_get_ptp_ts(aq_nic->aq_hw, &start);

When clk_sel < 0 the new aq_get_ptp_ts() returns without touching
*stamp, so the caller then forwards stack garbage as a timestamp
(into ns_to_timespec64() in the gettime path, into hw_adj_sys_clock()
in settime).  Should the early return set *stamp = 0 first, or should
the prototype be changed to return an error?

> +
[ ... ]
> +static int hw_atl2_hw_rx_ptp_ring_init(struct aq_hw_s *self,
> +				       struct aq_ring_s *aq_ring)
> +{
> +	hw_atl2_rpf_rx_desc_timestamp_req_set(self,
> +					      self->clk_select == ATL_TSG_CLOCK_SEL_1 ? 2 : 1,
> +					      aq_ring->idx);
> +	return aq_hw_err_from_flags(self);
> +}

[Low]
This reads self->clk_select with a plain access while every other
reader of the same field in this file uses READ_ONCE().  Should this
also be:

	s8 clk_sel = READ_ONCE(self->clk_select);

	hw_atl2_rpf_rx_desc_timestamp_req_set(self,
					      clk_sel == ATL_TSG_CLOCK_SEL_1 ? 2 : 1,
					      aq_ring->idx);

[ ... ]

> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_internal.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_internal.h
> index 31d7cae6641a..8da3262c167d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_internal.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_internal.h
> @@ -29,7 +29,8 @@
>  #define HW_ATL2_TXBUF_MAX              128U
>  #define HW_ATL2_PTP_TXBUF_SIZE           8U
>  
> -#define HW_ATL2_RXBUF_MAX              192U
> +/* AQC113 on-chip RX packet buffer available for data TCs */
> +#define HW_ATL2_RXBUF_MAX              172U
>  #define HW_ATL2_PTP_RXBUF_SIZE          16U
>  #define HW_ATL2_RSS_REDIRECTION_MAX 64U

[Medium]
The comment scopes this change to AQC113, but HW_ATL2_RXBUF_MAX is
used unconditionally in hw_atl2_hw_qos_set():

	u32 rx_buff_size = HW_ATL2_RXBUF_MAX;

for hw_atl2_caps_aqc113, hw_atl2_caps_aqc115c and
hw_atl2_caps_aqc116c, with no per-chip selection.  Does the same
on-chip 172 KB limit apply to aqc115c and aqc116c, or does this
silently shrink the RX buffer pool for chips outside the stated
scope of the patch?

The non-PTP path on AQC113 is also affected: hw_atl2_hw_qos_set()
already subtracts HW_ATL2_PTP_RXBUF_SIZE (16) when cfg->is_ptp is
set, so the PTP TC reservation is independent of this base value.
Would it make sense to split the RXBUF_MAX change into its own
commit with a Fixes: tag (if 192 was wrong) or a justification (if
this is a tradeoff for a new TC layout)?

[ ... ]