RE: [EXTERNAL] Re: [PATCH net-next v2 9/9] net: atlantic: add PTP support for AQC113 (Antigua) (Antigua)

[Sukhdeep Soni [C]]

On 12 May 2026, Paolo Abeni wrote:

> ---
> net: atlantic: add PTP support for AQC113 (Antigua) (Antigua)
>
> This patch adds IEEE 1588 PTP support for the AQC113 (Antigua) controller
> alongside the existing AQC107 (Atlantic) PTP path. It introduces dual TSG
> clocks, descriptor-writeback TX timestamps, per-instance PTP offsets,
> L3/L4 multicast filters, and reworks IRQ vector math and RX filter
> application order.
>
> Subject: net: atlantic: add PTP support for AQC113 (Antigua) (Antigua)
>
> [Low]
> The codename "(Antigua)" is duplicated in the subject line.

Fixed in v3.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> @@ -68,20 +70,14 @@ int aq_ndev_open(struct net_device *ndev)
>  	if (err < 0)
>  		goto err_exit;
>  
> -	err = aq_reapply_rxnfc_all_rules(aq_nic);
> -	if (err < 0)
> -		goto err_exit;
> -
> -	err = aq_filters_vlans_update(aq_nic);
> -	if (err < 0)
> -		goto err_exit;
> -
>  	err = aq_nic_start(aq_nic);
>  	if (err < 0) {
>  		aq_nic_stop(aq_nic);
>  		goto err_exit;
>  	}
>  
> +	aq_utils_obj_set(&aq_nic->aq_hw->flags, AQ_HW_FLAG_STARTED);
> +
>  err_exit:
>  	if (err < 0)
>  		aq_nic_deinit(aq_nic, true);
> @@ -97,6 +93,7 @@ int aq_ndev_close(struct net_device *ndev)
>  	err = aq_nic_stop(aq_nic);
>  	aq_nic_deinit(aq_nic, true);
>  
> +	aq_utils_obj_clear(&aq_nic->aq_hw->flags, AQ_HW_FLAG_STARTED);
>  	return err;
>  }
>
> [Medium]
> AQ_HW_FLAG_STARTED is set in aq_ndev_open() and cleared in
> aq_ndev_close(), but a grep across the tree shows no reader of that
> flag.
> 
> The commit message lists "Add AQ_HW_FLAG_STARTED flag management in
> open/close" as one of the changes.

> Is there a consumer of AQ_HW_FLAG_STARTED that was intended to be part
> of this patch but was omitted, or should the flag writes be dropped?

The AQ_HW_FLAG_STARTED set/clear calls will be dropped from the code in
v3 as there is no consumer. The commit message reference to it will be removed
in v3 as well.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -172,7 +180,11 @@ static int aq_nic_update_link_status(struct aq_nic_s *self)
>  		aq_nic_update_interrupt_moderation_settings(self);
>  
>  		if (self->aq_ptp) {
> -			aq_ptp_clock_init(self);
> +			/* PTP does not work in some modes even if physical link is up */
> +			bool ptp_link_good = (self->aq_hw->aq_link_status.mbps >= 100 &&
> +					      self->aq_hw->aq_link_status.full_duplex);
> +
> +			aq_ptp_clock_init(self, ptp_link_good ? AQ_PTP_LINK_UP : AQ_PTP_NO_LINK);
>  			aq_ptp_tm_offset_set(self,
>  					     self->aq_hw->aq_link_status.mbps);
>  			aq_ptp_link_change(self);
>
> [High]
> aq_nic_update_link_status() runs from aq_linkstate_threaded_isr() and
> from aq_nic_service_task(), neither of which holds rtnl_lock. The new
> AQC113 path calls into aq_ptp_clock_init() which in turn calls
> aq_ptp_dpath_enable().
> 
> aq_ptp_dpath_enable() writes aq_ptp->udp_filter[] and programs L3/L4/L2
> filter registers. The ioctl path aq_ndev_hwtstamp_set() ->
> aq_ptp_hwtstamp_config_set() -> aq_ptp_dpath_enable() runs under
> rtnl_lock only, so the lockset intersection with the link-change path
> is empty.
>
> Can two concurrent callers of aq_ptp_dpath_enable() tear udp_filter[]
> entries and interleave hw_filter_l3l4_set / hw_filter_l2_set register
> writes on different CPUs? Before this patch the A1 flow touched these
> filters only from the ioctl path, so this race did not exist.

Both call sites of aq_ptp_dpath_enable() — the link-change path via
aq_ptp_clock_init() and the ioctl path via aq_ptp_hwtstamp_config_set()
— hold ptp_filter_lock around the call in v3. Concurrent callers are serialized
by that mutex.

> @@ -279,6 +291,9 @@ static int aq_nic_hw_prepare(struct aq_nic_s *self)
>  	int err = 0;
>  
>  	err = self->aq_hw_ops->hw_soft_reset(self->aq_hw);
> +
> +	self->aq_hw->clk_select = -1;
> +
>  	if (err)
>  		goto exit;
>
> [Medium]
> clk_select is set to -1 here and is then fed, without validation, into
> hw_atl2_tsg_clock_read() / hw_atl2_tsg_clock_add() /
> hw_atl2_tsg_clock_sub() / hw_atl2_tsg_clock_increment_set() through
> aq_get_ptp_ts(), hw_atl2_adj_sys_clock() and
> hw_atl2_adj_clock_freq().
>
> Those helpers plug the value into HW_ATL2_TSG_REG_ADR(clock_sel, ...)
> with no range check. Since clk_select is s8, -1 promoted through the
> unsigned int parameter becomes 0xffffffff and produces an out-of-range
> MMIO offset.
> 
> In aq_ptp_init() the PTP clock is exposed to userspace via
> ptp_clock_register() before hw_atl2_enable_ptp() sets clk_select to 0
> or 1. A gettime/adjtime on the newly appearing /dev/ptpN during that
> window would land in the above helpers with clk_select still equal to
> -1.
> 
> Would a guard like "if (self->clk_select < 0) return 0;" at the top of
> each of those helpers be appropriate?
>
> [Medium]
> clk_select is also written by hw_atl2_enable_ptp() (called from
> aq_ptp_init / aq_ptp_free / hw_atl2_hw_reset) and read on the TX fast
> path via self->aq_hw_ops->hw_get_clk_sel(self->aq_hw) from
> aq_nic_map_skb(), as well as by hw_atl2_hw_rx_ptp_ring_init() and
> hw_atl2_hw_reset(). There is no lock, no READ_ONCE/WRITE_ONCE, and no
> atomic wrapper.
>
> Can a TX in flight during reset/enable/disable observe a torn or stale
> clk_select and either tag a descriptor with the wrong clock selector
> or propagate -1 into the HW register macros?

Both concerns are addressed in v3. All TSG helpers (aq_get_ptp_ts,
hw_atl2_adj_sys_clock, hw_atl2_adj_clock_freq) snapshot clk_select
via READ_ONCE and return early if the value is < 0, closing the window
between ptp_clock_register and hw_atl2_enable_ptp. The writer
(hw_atl2_enable_ptp) uses WRITE_ONCE, and hw_atl2_hw_get_clk_sel
on the TX fast path also uses READ_ONCE, preventing torn or stale reads.

> @@ -450,7 +465,14 @@ int aq_nic_init(struct aq_nic_s *self)
>  	}
>  
>  	if (aq_nic_get_cfg(self)->is_ptp) {
> -		err = aq_ptp_init(self, self->irqvecs - 1);
> +		u32 ptp_isr_vec;
> +
> +		if (self->irqvecs > AQ_HW_PTP_IRQS)
> +			ptp_isr_vec = self->irqvecs - AQ_HW_PTP_IRQS;
> +		else
> +			ptp_isr_vec = 0;
> +
> +		err = aq_ptp_init(self, ptp_isr_vec);
>
> [ ... ]
>
> @@ -793,6 +823,12 @@ unsigned int aq_nic_map_skb(struct aq_nic_s *self, struct sk_buff *skb,
>  
>  	first->eop_index = dx;
>  	dx_buff->is_eop = 1U;
> +	if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
> +	    self->aq_hw_ops->enable_ptp &&
> +	    self->aq_hw_ops->hw_get_clk_sel) {
> +		dx_buff->request_ts = 1U;
> +		dx_buff->clk_sel = self->aq_hw_ops->hw_get_clk_sel(self->aq_hw);
> +	}
>  	dx_buff->skb = skb;
>  	dx_buff->xdpf = NULL;
>  	goto exit;
>
> [Critical]
> request_ts is armed here whenever SKBTX_IN_PROGRESS is set and the op
> pointers exist, not only for PTP traffic. SKBTX_IN_PROGRESS fires for
> any SO_TIMESTAMPING socket on AQC113, which makes the following
> aq_ring_tx_clean() path reachable via ordinary unprivileged traffic:

Fixed in v3. The condition now includes aq_ptp_ring(self, ring), so request_ts
is only armed when the skb is being sent on the dedicated PTP TX ring.
Ordinary SO_TIMESTAMPING traffic on non-PTP rings does not trigger the
timestamp polling path.

> drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> aq_ring_tx_clean() {
>    ...
>    if (likely(buff->is_mapped)) {
>        ...
>        dma_unmap_single(dev, buff->pa, buff->len, DMA_TO_DEVICE);
>        /* or dma_unmap_page(...) */
>    }
>
>    if (unlikely(buff->is_eop)) {
>        if (unlikely(buff->request_ts) &&
>            self->aq_nic->aq_hw_ops->hw_ring_tx_ptp_get_ts) {
>            u64 ts = ...->hw_ring_tx_ptp_get_ts(self);
>
>            if (!ts)
>                break;           /* HW has not set DD/TS_VALID yet */
>            ...
>        }
>        ...
>    }
>    buff->skb = NULL;
>    buff->xdpf = NULL;
>    buff->pa = 0U;
>    ...
>    self->sw_head = aq_ring_next_dx(self, self->sw_head);
> }
>
> hw_atl2_hw_ring_tx_ptp_get_ts() explicitly returns 0 when DD or
> TS_VALID is not yet set, so the zero-timestamp break path is
> deliberately reachable.
>
> On the next aq_ring_tx_clean() invocation, sw_head is unchanged,
> buff->is_mapped is still 1, and buff->pa still holds the original DMA
> handle. Is the second pass going to call dma_unmap_single() or
> dma_unmap_page() a second time on the already-unmapped address? A grep
> for "is_mapped = 0" in the driver returns no matches, so buff->is_mapped
> is never cleared by any other path.
>
> Additionally, the commit message states:
>
>    Poll for TX timestamp completion in aq_ring_tx_clean() with a
>    timeout mechanism (aq_ptp_tx_ts_timedout/clear)
> 
> Neither aq_ptp_tx_ts_timedout nor aq_ptp_tx_ts_clear exists anywhere
> in the tree. If HW never produces a timestamp (for example when
> request_ts is armed but AQ_HW_TXD_CTL_TS_EN is not set, or under a HW
> glitch), sw_head never advances and the TX ring wedges until the TX
> watchdog fires.
> 
> Would it be safer to check the timestamp before any DMA unmap and/or
> guard the break path so it does not leave buff->is_mapped and buff->pa
> live for a repeat unmap?

Both issues are fixed in v3. On the ts == 0 break path, buff->is_mapped
and buff->pa are cleared before the break, so the subsequent aq_ring_tx_clean()
pass sees is_mapped == 0 and skips the DMA unmap — no double-unmap is possible.
The timeout is implemented via self->ptp_ts_deadline: when it expires, request_ts
is cleared, the skb is freed, and the loop continues normally, unblocking the ring.
The commit message references to aq_ptp_tx_ts_timedout/clear are stale and will
be removed in v3.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> @@ -572,53 +770,53 @@ void aq_ptp_hwtstamp_config_get(struct aq_ptp_s *aq_ptp,
>  	*config = aq_ptp->hwtstamp_config;
>  }
>  
> -static void aq_ptp_prepare_filters(struct aq_ptp_s *aq_ptp)
> +static unsigned int aq_ptp_parse_rx_filters(enum hwtstamp_rx_filters rx_filter)
>  {
> [ ... ]
> +	return ptp_en_flags;
>  }
>
> [Low]
> aq_ptp_parse_rx_filters() is typed to take enum hwtstamp_rx_filters,
> but aq_ptp_clock_init() calls it with AQ_HW_PTP_DISABLE, which belongs
> to the driver-internal AQ_HW_PTP_* bitmask enum. This works today only
> because AQ_HW_PTP_DISABLE happens to equal HWTSTAMP_FILTER_NONE (both
> zero).
>
> Could the call site pass HWTSTAMP_FILTER_NONE explicitly, or the
> helper be reshaped to take a boolean disable argument, so future
> renumbering of either enum does not silently break link-down filter
> teardown?

Fixed in v3. The call site in aq_ptp_clock_init now passes
HWTSTAMP_FILTER_NONE explicitly for the disable path,
removing the accidental coupling to the zero value of
AQ_HW_PTP_DISABLE.

> @@ -972,11 +1179,13 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
>  	if (err)
>  		goto err_exit_ptp_tx;
>  
> -	err = aq_ring_hwts_rx_alloc(&aq_ptp->hwts_rx, aq_nic, PTP_HWST_RING_IDX,
> -				    aq_nic->aq_nic_cfg.rxds,
> -				    aq_nic->aq_nic_cfg.aq_hw_caps->rxd_size);
> -	if (err)
> -		goto err_exit_ptp_rx;
> +	if (aq_ptp->a1_ptp) {
> +		err = aq_ring_hwts_rx_alloc(&aq_ptp->hwts_rx, aq_nic, PTP_HWST_RING_IDX,
> +					    aq_nic->aq_nic_cfg.rxds,
> +					    aq_nic->aq_nic_cfg.aq_hw_caps->rxd_size);
> +		if (err)
> +			goto err_exit_ptp_rx;
> +	}
>  
>  	err = aq_ptp_skb_ring_init(&aq_ptp->skb_ring, aq_nic->aq_nic_cfg.rxds);
>  	if (err != 0) {
> @@ -984,7 +1193,7 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
>  		goto err_exit_hwts_rx;
>  	}
>  
> [ ... ]
>  err_exit_hwts_rx:
> -	aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
> +	if (aq_ptp->a1_ptp)
> +		aq_ring_free(&aq_ptp->hwts_rx);
>  err_exit_ptp_rx:
>
> [High]
> aq_ring_hwts_rx_alloc() allocates size * dx_size + AQ_CFG_RXDS_DEF via
> dma_alloc_coherent(), and aq_ring_hwts_rx_free() passes the same
> "+ AQ_CFG_RXDS_DEF" back to dma_free_coherent(). The replacement call
> aq_ring_free() here passes only self->size * self->dx_size:
>
> drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> aq_ring_free() {
>    ...
>     dma_free_coherent(aq_nic_get_dev(self->aq_nic),
>                      self->size * self->dx_size, self->dx_ring,
>                      self->dx_ring_pa);
>    ...
> }
>
> The DMA API requires dma_free_coherent() to be called with the same
> size as dma_alloc_coherent(). The non-error path in aq_ptp_ring_free()
> still calls aq_ring_hwts_rx_free().
>
> Is this asymmetric free intentional, and if not, should the error path
> use aq_ring_hwts_rx_free() as well?

Fixed in v3. The error path at err_exit_hwts_rx now uses
aq_ring_hwts_rx_free(), matching the + AQ_CFG_RXDS_DEF
allocation size passed to dma_alloc_coherent().

> @@ -1244,30 +1514,45 @@ void aq_ptp_free(struct aq_nic_s *aq_nic)
>  	if (!aq_ptp)
>  		return;
>  
> +	/* disable ptp */
> +	if (aq_ptp->a1_ptp) {
> +		mutex_lock(&aq_nic->fwreq_mutex);
> +		aq_nic->aq_fw_ops->enable_ptp(aq_nic->aq_hw, 0);
> +		mutex_unlock(&aq_nic->fwreq_mutex);
> +	}
> +
> +	if (aq_ptp->a2_ptp)
> +		aq_nic->aq_hw_ops->enable_ptp(aq_nic->aq_hw,
> +					      aq_ptp->ptp_clock_sel, 0);
> +
> +	cancel_delayed_work_sync(&aq_ptp->poll_sync);
> +
>
> [Medium]
> The prior ordering cancelled poll_sync before disabling PTP. This hunk
> disables PTP first and then cancels the delayed work.
>
> aq_ptp_poll_sync_work_cb() calls aq_ptp_check_sync1588(), which
> touches PTP hardware state (hw_ts_to_sys_clock, ptp_clock_event on
> aq_ptp->ptp_clock) and reschedules itself via schedule_delayed_work()
> when extts_pin_enabled is set.
>
> Can a pending callback run against PTP hardware that was just turned
> off, and would it be better to cancel_delayed_work_sync() first and
> only then tear the hardware down?

Fixed in v3. cancel_delayed_work_sync(&aq_ptp->poll_sync) is called first,
then PTP is disabled — matching the standard pattern of cancelling
workers before tearing down the resources they access.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -308,24 +308,30 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
> [ ... see Critical comment above on dma_unmap / break ordering ... ]
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> @@ -886,6 +912,138 @@ static struct aq_stats_s *hw_atl2_utils_get_hw_stats(struct aq_hw_s *self)
> +static int hw_atl2_gpio_pulse(struct aq_hw_s *self, u32 index, u32 clk_sel,
> +			      u64 start, u32 period, u32 hightime)
> +{
> +	u32 mode;
> +
> +	if (start == 0)
> +		mode = HW_ATL2_GPIO_PIN_SPEC_MODE_GPIO;
> +	else if (clk_sel == ATL_TSG_CLOCK_SEL_0)
> +		mode = HW_ATL2_GPIO_PIN_SPEC_MODE_TSG0_EVENT_OUTPUT;
> +	else
> +		mode = HW_ATL2_GPIO_PIN_SPEC_MODE_TSG1_EVENT_OUTPUT;
> +
> +	if (index == 1 || index == 3) { /* Hardware limitation */
> +		hw_atl2_gpio_special_mode_set(self, mode, index);
> +	}
> +
> +	hw_atl2_tsg_ptp_gpio_gen_pulse(self, clk_sel, start, period, hightime);
> +
> +	return 0;
> +}
>
> [Medium]
> hw_atl2_gpio_special_mode_set() is only called when index is 1 or 3,
> but hw_atl2_tsg_ptp_gpio_gen_pulse() is invoked unconditionally and 0
> is returned for every index.
>
> For a PTP_PIN_SETFUNC on pin 0, 2 or any other non-special index, the
> TSG pulse generator is armed but the pin mux is never reprogrammed, so
> no signal appears on the pin and the userspace request appears to
> succeed.
> 
> Should invalid indices be rejected with -EINVAL, or should the pulse
> generator only be armed when the pin can actually be muxed?

Fixed in v3. Added early return -EINVAL for indices other than 1 and 3.
hw_atl2_tsg_ptp_gpio_gen_pulse remains unconditional so the
period=0 disable path always reaches hardware.

> 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
> --- 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
> +/* Reduced from 192 to reserve space for PTP RX timestamp trailer */
> +#define HW_ATL2_RXBUF_MAX              172U
>  #define HW_ATL2_PTP_RXBUF_SIZE          16U
>
> [Medium]
>HW_ATL2_RXBUF_MAX is reduced unconditionally for every AQC113 user,
> but hw_atl2_hw_qos_set() already subtracts HW_ATL2_PTP_RXBUF_SIZE when
> cfg->is_ptp is true, so the reduction appears redundant when PTP is
> enabled and a pure loss for non-PTP deployments.
>
> The justification in the comment is per-packet (HW_ATL2_RX_TS_SIZE is
> 8 bytes, handled in hw_atl2_hw_rx_extract_ts), while this register
> programs the on-chip packet-buffer KB allocation.
>
> Could this change be gated on cfg->is_ptp, or is there a separate
> reason to take ~10% of the RX packet buffer from non-PTP users?

172 is the correct hardware maximum for AQC113 data TCs — AQC113 has a smaller on-chip RX packet buffer than A1. With PTP enabled, hw_atl2_hw_qos_set() correctly subtracts HW_ATL2_PTP_RXBUF_SIZE (16 KB) from 172, giving 156 KB for regular TCs and 16 KB for the PTP TC. Setting it to 192 causes ptp4l to fail, confirming 172 is the hardware constraint. The comment will be updated in v3 to clarify this is a hardware limit, not a PTP-related reduction.

> -- 
> This is an AI-generated review.