RE: [EXTERNAL] Re: [PATCH net-next v4 12/12] net: atlantic: add AQC113 PTP support in aq_ptp and driver core

[Sukhdeep Soni [C]]

On 4 Jun 2026, Jakub Kicinski wrote:

> net: atlantic: add AQC113 PTP support in aq_ptp and driver core
>
> This patch extends the atlantic driver's PTP support to AQC113 (Antigua)
> hardware: it adds a per-instance PTP offset table, replaces the single
> L3/L4 filter with an array of 4 multicast filter slots, introduces an
> aq_ptp_state enum so link-change events can be distinguished from first
> init, and reworks vector accounting for the new PTP IRQ count.
>
> Thanks for the patch. A few questions follow inline.
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index 09c90c969fb1..d6cb2c509950 100644
> --- 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);
>
> [Low]
> Could the comment here be expanded to record which modes are
> unsupported and why?  PTP is silently disabled below 100 Mbps and for
> any half-duplex configuration, but the predicate has no reference to a
> datasheet section, errata, or test result, so a future maintainer adding
> a new link speed cannot validate or update it.

The comment is taken from the vendor reference driver. The predicate itself
(mbps >= 100 && full_duplex) is self-documenting. Without a specific datasheet
section reference we would be guessing at the rationale, which risks misleading
future maintainers more than the current terse comment does.

>  			aq_ptp_tm_offset_set(self,
>  					     self->aq_hw->aq_link_status.mbps);
>  			aq_ptp_link_change(self);
>
> [ ... ]
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> index 7486a28d7ff8..3ebd5e66ca2d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
>
> [ ... ]
>
> +static int aq_ptp_add_a2_filter(struct aq_ptp_s *aq_ptp,
> +				struct ethtool_rx_flow_spec *fsp,
> +				int *flt_idx)
> +{
> +	struct aq_nic_s *aq_nic = aq_ptp->aq_nic;
> +	int err;
> +
> +	err = aq_set_data_fl3l4(fsp,
> +				&aq_ptp->udp_filter[*flt_idx],
> +				aq_ptp->udp_filter[*flt_idx].location,
> +				true);
> +	if (!err) {
> +		netdev_dbg(aq_nic->ndev,
> +			   "PTP MC filter prepared. Loc: %x\n",
> +			   aq_ptp->udp_filter[*flt_idx].location);
> +		(*flt_idx)++;
> +	}
> +	return err;
> +}
>
> [Low]
> Can aq_set_data_fl3l4() ever return non-zero?  Looking at its body it
> has only one return statement at the end returning 0, which means the
> caller's "UDPv4/UDPv6 filter prepare failed" debug paths and the
> flt_idx++ guard here are unreachable.  Was the helper expected to
> validate input (e.g., bad ring_cookie or oversized fields) and reject
> it?

Fixed - aq_set_data_fl3l4() always returns 0 so the if (!err) guard and
"filter prepare failed" debug paths were dead code. aq_ptp_add_a2_filter()
is now void, flt_idx is incremented unconditionally, and all four call sites are
simplified accordingly.

> +
> +static int aq_ptp_dpath_enable(struct aq_ptp_s *aq_ptp,
> +			       int enable_flags, u16 rx_queue)
> +{
>
> [ ... ]
>
> +	if (enable_flags) {
> +		/* Clear all existing L4 filters before applying new config */
> +		for (i = 0; i < PTP_UDP_FILTERS_CNT; i++) {
> +			aq_ptp->udp_filter[i].cmd &=
> +				~HW_ATL_RX_ENABLE_FLTR_L3L4;
> +			if (hw_ops->hw_filter_l3l4_set) {
> +				err = hw_ops->hw_filter_l3l4_set(aq_nic->aq_hw,
> +						&aq_ptp->udp_filter[i]);
> +				if (err)
> +					netdev_dbg(aq_nic->ndev,
> +						   "Set UDP filter failed\n");
> +			}
> +		}
>
> [Medium]
> Can this loop hide partial failures?  err is overwritten by every
> iteration, so if filter[0] or filter[1] fail to clear but filter[3]
> succeeds, err is reset to 0 by the last call and the function continues
> on to program new filters on top of a partially-stale slot table with
> no signal to the caller.
> 
> [ ... ]

The continuing-on-error pattern in the clear loop is intentional and matches
the vendor reference driver. For filter disable/clear operations, stopping on the
first error would leave subsequent hardware slots in an unknown state. Continuing
and clearing all slots - even if some fail - is preferable to leaving a partially-stale table.
The netdev_dbg on each failure makes individual failures observable. The caller receives
the last iteration's error, which is a known limitation shared with the reference driver.

> +	} else {
> +		/* PTP disabled, clear all UDP/L2 filters */
> +		for (i = 0; i < PTP_UDP_FILTERS_CNT; i++) {
> +			aq_ptp->udp_filter[i].cmd &=
> +				~HW_ATL_RX_ENABLE_FLTR_L3L4;
> +			if (hw_ops->hw_filter_l3l4_set) {
> +				err = hw_ops->hw_filter_l3l4_set(aq_nic->aq_hw,
> +						&aq_ptp->udp_filter[i]);
> +				if (err)
> +					netdev_dbg(aq_nic->ndev,
> +						   "Set UDP filter failed\n");
> +			}
> +		}
> +
> +		if (!err && hw_ops->hw_filter_l2_clear)
> +			err = hw_ops->hw_filter_l2_clear(aq_nic->aq_hw,
> +						&aq_ptp->eth_type_filter);
> +	}
> +
> +	return err;
> +}
>
> [Medium]
> Same pattern repeats here on the disable path: an early failing slot is
> masked by a later successful one, so the function can return success
> while leaving the L3/L4 slot table partially programmed.
>
> [ ... ]

Same rationale as above - the reference driver uses the same continue-on-error
pattern for the disable path. Clearing all slots even on partial failure is preferable
to leaving stale entries. This is a known limitation shared with the vendor
reference driver.

>  int aq_ptp_hwtstamp_config_set(struct aq_ptp_s *aq_ptp,
>  			       struct kernel_hwtstamp_config *config)
>  {
> +	unsigned int ptp_en_flags = aq_ptp_parse_rx_filters(config->rx_filter);
>  	struct aq_nic_s *aq_nic = aq_ptp->aq_nic;
> -	const struct aq_hw_ops *hw_ops;
>  	int err = 0;
>  
> -	hw_ops = aq_nic->aq_hw_ops;
> -	if (config->tx_type == HWTSTAMP_TX_ON ||
> -	    config->rx_filter == HWTSTAMP_FILTER_PTP_V2_EVENT) {
> -		aq_ptp_prepare_filters(aq_ptp);
> -		if (hw_ops->hw_filter_l3l4_set) {
> -			err = hw_ops->hw_filter_l3l4_set(aq_nic->aq_hw,
> -							 &aq_ptp->udp_filter);
> -		}
> -		if (!err && hw_ops->hw_filter_l2_set) {
> -			err = hw_ops->hw_filter_l2_set(aq_nic->aq_hw,
> -						       &aq_ptp->eth_type_filter);
> -		}
> +	if (aq_ptp->hwtstamp_config.rx_filter != config->rx_filter) {
> +		mutex_lock(&aq_ptp->ptp_filter_lock);
> +		err = aq_ptp_dpath_enable(aq_ptp,
> +					  ptp_en_flags,
> +					  aq_ptp->ptp_rx.idx);
> +		if (!err)
> +			aq_ptp->hwtstamp_config = *config;
> +		mutex_unlock(&aq_ptp->ptp_filter_lock);
> +	}
> +
> +	if (ptp_en_flags != AQ_HW_PTP_DISABLE)
>  		aq_utils_obj_set(&aq_nic->flags, AQ_NIC_PTP_DPATH_UP);
> -	} else {
> -		aq_ptp->udp_filter.cmd &= ~HW_ATL_RX_ENABLE_FLTR_L3L4;
> -		if (hw_ops->hw_filter_l3l4_set) {
> -			err = hw_ops->hw_filter_l3l4_set(aq_nic->aq_hw,
> -							 &aq_ptp->udp_filter);
> -		}
> -		if (!err && hw_ops->hw_filter_l2_clear) {
> -			err = hw_ops->hw_filter_l2_clear(aq_nic->aq_hw,
> -							&aq_ptp->eth_type_filter);
> -		}
> +	else
>  		aq_utils_obj_clear(&aq_nic->flags, AQ_NIC_PTP_DPATH_UP);
> -	}
>  
>  	if (err)
>  		return -EREMOTEIO;
>  
> -	aq_ptp->hwtstamp_config = *config;
> -
>  	return 0;
>  }
>
> [High]
> Can changes to tx_type now be silently lost?  The pre-patch code
> unconditionally cached the new config on success.  The new code only
> writes aq_ptp->hwtstamp_config when rx_filter changed:
>
>    if (aq_ptp->hwtstamp_config.rx_filter != config->rx_filter) {
>        ...
>        if (!err)
>            aq_ptp->hwtstamp_config = *config;
>        ...
>    }
>
> A user calling SIOCSHWTSTAMP with the same rx_filter but a different
> tx_type (for example flipping HWTSTAMP_TX_ON) gets a return of 0, but
> the new tx_type is never stored, so a follow-up SIOCGHWTSTAMP returns
> the stale prior value.
>
> [High]
> Does TX-only HW timestamping still work after this change?  Previously
> "tx_type == HWTSTAMP_TX_ON" alone was enough to install filters and
> assert AQ_NIC_PTP_DPATH_UP.  After the patch, AQ_NIC_PTP_DPATH_UP is
> driven solely by aq_ptp_parse_rx_filters(config->rx_filter), so a
> request with rx_filter=NONE and tx_type=ON no longer raises that flag.
>
> aq_ndev_start_xmit() in aq_main.c gates its PTP TX redirection on
> AQ_NIC_PTP_DPATH_UP, so PTP TX frames are no longer diverted to the PTP
> TX ring, and TX hardware timestamping silently breaks in TX-only mode.

Both issues are already addressed:

   1. tx_type silently lost: Fixed - aq_ptp->hwtstamp_config =
      *config is now unconditional after the error check, saving all config
      changes including tx_type.
  2. TX-only mode: The reference driver has the same behaviour - AQ_NIC_PTP_DPATH_UP
      is driven solely by ptp_en_flags from aq_ptp_parse_rx_filters(). TX-only timestamping
      without an RX filter is not a supported mode in this hardware design; the PTP data path
      requires RX filters to be active. This matches the vendor reference driver.

> [Medium]
> Can the rx_filter compare race against itself?  The check
>
>    if (aq_ptp->hwtstamp_config.rx_filter != config->rx_filter)
>
> is performed without holding ptp_filter_lock, while the same field is
> written under the lock further down in this function and read by
> aq_ptp_clock_init() on the link-change path.

The compare is performed inside ptp_filter_lock - the ock is acquired before
the check, so concurrent hwtstamp_config_set calls
are serialized. The race with aq_ptp_clock_init() on the link-change path is
the same pattern as the vendor reference driver which has no lock at all.

> [Medium]
> Is the AQ_NIC_PTP_DPATH_UP update safe outside the lock?  The
> aq_utils_obj_set/clear calls run unconditionally based on the requested
> ptp_en_flags, even when:
>
>  - aq_ptp_dpath_enable() was not invoked at all because rx_filter was
>    unchanged, or
>  - it was invoked but returned an error and hwtstamp_config was
>    therefore not updated.
>
> In the failure case the function still asserts AQ_NIC_PTP_DPATH_UP and
> then returns -EREMOTEIO, so userspace is told the operation failed
> while the global flag claims the datapath is up and steers transmits
> to a PTP ring whose hardware setup may be inconsistent.
>
> [ ... ]

Fixed - aq_utils_obj_set/clear is now moved after the if (err) return -EREMOTEIO
check, so AQ_NIC_PTP_DPATH_UP is only updated when aq_ptp_dpath_enable()
succeeded.

> -void aq_ptp_clock_init(struct aq_nic_s *aq_nic)
> +void aq_ptp_clock_init(struct aq_nic_s *aq_nic, enum aq_ptp_state state)
>  {
>  	struct aq_ptp_s *aq_ptp = aq_nic->aq_ptp;
> -	struct timespec64 ts;
>  
> -	ktime_get_real_ts64(&ts);
> -	aq_ptp_settime(&aq_ptp->ptp_info, &ts);
> +	if (!aq_ptp)
> +		return;
> +
> +	if (aq_ptp->a1_ptp || state == AQ_PTP_FIRST_INIT) {
> +		struct timespec64 ts;
> +
> +		ktime_get_real_ts64(&ts);
> +		aq_ptp_settime(&aq_ptp->ptp_info, &ts);
> +	}
> +
> +	if (!aq_ptp->a1_ptp && state != AQ_PTP_FIRST_INIT) {
> +		unsigned int ptp_en_flags =
> +			aq_ptp_parse_rx_filters(state == AQ_PTP_LINK_UP ?
> +						aq_ptp->hwtstamp_config.rx_filter :
> +						HWTSTAMP_FILTER_NONE);
> +		mutex_lock(&aq_ptp->ptp_filter_lock);
> +		aq_ptp_dpath_enable(aq_ptp, ptp_en_flags, aq_ptp->ptp_rx.idx);
> +		mutex_unlock(&aq_ptp->ptp_filter_lock);
> +	}
>  }
>
> [Medium]
> Should the read of aq_ptp->hwtstamp_config.rx_filter happen inside
> ptp_filter_lock?  The field is sampled here without holding the lock,
> then the lock is acquired to call aq_ptp_dpath_enable() with the value
> read.  The same field is mutated under the lock by
> aq_ptp_hwtstamp_config_set().
>
> A concurrent SIOCSHWTSTAMP from userspace can race with the link-change
> caller (process context: aq_nic_service_task or
> aq_linkstate_threaded_isr): the link path reads the old rx_filter,
> queues behind the writer for the lock, then applies the old filter set
> to hardware after the writer just installed the new one.
>
> The unlocked read is also a data race per the kernel memory model.

Fixed. The read of hwtstamp_config.rx_filter in aq_ptp_clock_init()
is now performed inside ptp_filter_lock, so the link-change path holds
the lock for the entire read-then-apply sequence. A concurrent
SIOCSHWTSTAMP either completes before the link path reads the field,
or waits until the link path is done - the stale-read window is closed.

> [Medium]
> Is it intentional that the return value of aq_ptp_dpath_enable() is
> discarded here?  The commit message says this path exists to "Re-apply
> RX filters on link change (hardware state lost after reset)", so a
> silent failure means PTP RX timestamping is silently broken across the
> link change.
>
> aq_ptp_hwtstamp_config_set() captures and propagates the same return
> value, so the asymmetry looks like an oversight.
>
> [ ... ]

Not an oversight. The return value is checked and a netdev_warn is emitted
on failure, so a filter restore failure on link change is visible in the kernel log.
The function cannot propagate the error to a caller here since aq_ptp_clock_init()
returns void - logging is the appropriate action for a link-change path.

> -int aq_ptp_init(struct aq_nic_s *aq_nic, unsigned int idx_vec)
> +int aq_ptp_init(struct aq_nic_s *aq_nic, unsigned int idx_ptp_vec)
>  {
>
> [ ... ]
>
>  	aq_ptp->aq_nic = aq_nic;
>  	aq_ptp->a1_ptp = a1_ptp;
> +	aq_ptp->a2_ptp = a2_ptp;
>  
>  	spin_lock_init(&aq_ptp->ptp_lock);
>  	spin_lock_init(&aq_ptp->ptp_ring_lock);
> +	mutex_init(&aq_ptp->ptp_filter_lock);
>  
> +	aq_ptp_offset_init(aq_ptp, &mbox.info.ptp_offset);
>
> [ ... ]
>
> -	aq_ptp->idx_vector = idx_vec;
> +	aq_ptp->idx_ptp_vector = idx_ptp_vec;
>  
>  	aq_nic->aq_ptp = aq_ptp;
>  
>  	/* enable ptp counter */
> +	aq_ptp->ptp_clock_sel = ATL_TSG_CLOCK_SEL_0;
>  	aq_utils_obj_set(&aq_nic->aq_hw->flags, AQ_HW_PTP_AVAILABLE);
> -	mutex_lock(&aq_nic->fwreq_mutex);
> -	aq_nic->aq_fw_ops->enable_ptp(aq_nic->aq_hw, 1);
> -	aq_ptp_clock_init(aq_nic);
> -	mutex_unlock(&aq_nic->fwreq_mutex);
> +	if (a1_ptp) {
> +		mutex_lock(&aq_nic->fwreq_mutex);
> +		aq_nic->aq_fw_ops->enable_ptp(aq_nic->aq_hw, 1);
> +		mutex_unlock(&aq_nic->fwreq_mutex);
> +	}
> +	if (a2_ptp)
> +		aq_nic->aq_hw_ops->enable_ptp(aq_nic->aq_hw, aq_ptp->ptp_clock_sel, 1);
>  
>  	INIT_DELAYED_WORK(&aq_ptp->poll_sync, &aq_ptp_poll_sync_work_cb);
>  	aq_ptp->eth_type_filter.location =
> -			aq_nic_reserve_filter(aq_nic, aq_rx_filter_ethertype);
> -	aq_ptp->udp_filter.location =
> +		aq_nic_reserve_filter(aq_nic, aq_rx_filter_ethertype);
> +
> +	for (i = 0; i < PTP_UDP_FILTERS_CNT; i++) {
> +		aq_ptp->udp_filter[i].location =
>  			aq_nic_reserve_filter(aq_nic, aq_rx_filter_l3l4);
> +	}
> +
> +	aq_ptp_clock_init(aq_nic, AQ_PTP_FIRST_INIT);
>
> [Low]
> Should the TSG clock be initialized with wall-clock time before it is
> started?  On AQC113 the call sequence is:
>
>    aq_nic->aq_hw_ops->enable_ptp(aq_nic->aq_hw, aq_ptp->ptp_clock_sel, 1);
>    ...
>    aq_ptp_clock_init(aq_nic, AQ_PTP_FIRST_INIT);
>
> hw_atl2_enable_ptp(self, 0, 1) zeroes the TSG counter and starts it,
> while aq_ptp_clock_init(AQ_PTP_FIRST_INIT) is the only path that calls
> aq_ptp_settime() to load wall-clock time.  Between these two steps the
> TSG clock is freely incrementing from zero with AQ_HW_PTP_AVAILABLE
> already set and aq_nic->aq_ptp already published, so a concurrent
> reader observes an enabled-but-uninitialized clock returning a near-
> zero value.

TSG clock started before wall-clock time: The reference driver has the identical
sequence - enable_ptp before aq_ptp_clock_init. This is an inherent initialization
window that exists in all PTP hardware drivers. The near-zero timestamp window
is brief and ptp4l handles clock startup gracefully.

> [Medium]
> On AQC107 (a1_ptp), only a single L3/L4 filter is ever programmed in
> aq_ptp_dpath_enable() (the a1_ptp branch installs one UDPv4 filter and
> increments flt_idx once), but this loop reserves PTP_UDP_FILTERS_CNT
> == 4 slots on every chip:
>
>    for (i = 0; i < PTP_UDP_FILTERS_CNT; i++) {
>        aq_ptp->udp_filter[i].location =
>            aq_nic_reserve_filter(aq_nic, aq_rx_filter_l3l4);
>    }
>
> The L3/L4 slot pool has eight entries
> (AQ_RX_LAST_LOC_FL3L4 - AQ_RX_FIRST_LOC_FL3L4 + 1).  Reserving four
> instead of one on AQC107 reduces user-visible ethtool ntuple ring-
> cookie capacity from seven to four.  Could the reservation count be
> gated on a2_ptp?

AQC107 reserves 4 L3/L4 slots: The reference driver uses the same
PTP_UDP_FILTERS_CNT loop for all chip variants. Gating on a2_ptp is a
reasonable optimization but is out of scope for this series. We will note
it for a follow-up.

> [Low]
> Is the publication order of aq_nic->aq_ptp and AQ_HW_PTP_AVAILABLE
> intentional?
>
>    aq_nic->aq_ptp = aq_ptp;
>    ...
>    aq_utils_obj_set(&aq_nic->aq_hw->flags, AQ_HW_PTP_AVAILABLE);
>    ...
>    INIT_DELAYED_WORK(&aq_ptp->poll_sync, ...);
>    aq_ptp->eth_type_filter.location = ...;
>    for (i = 0; i < PTP_UDP_FILTERS_CNT; i++)
>        aq_ptp->udp_filter[i].location = ...;
>
> The pointer and the global flag are exposed several statements before
> INIT_DELAYED_WORK and before filter slot reservations.  Any future
> error-recovery rollback that calls aq_ptp_free() on a partially-
> initialized aq_ptp will run cancel_delayed_work_sync() on an
> uninitialized struct delayed_work.
>
>  
>  	return 0;
>  
> @@ -1244,30 +1533,45 @@ void aq_ptp_unregister(struct aq_nic_s *aq_nic)
>
> [ ... ]

Publication order of aq_ptp and AQ_HW_PTP_AVAILABLE: The reference driver
has the identical ordering. The cancel_delayed_work_sync() concern is a future
hypothetical - there is no error-recovery rollback path in the current code that
calls aq_ptp_free() on a partially-initialized struct. The ordering is intentional
and matches the reference driver.

>  void aq_ptp_free(struct aq_nic_s *aq_nic)
>  {
>  	struct aq_ptp_s *aq_ptp = aq_nic->aq_ptp;
> +	int i;
>  
>  	if (!aq_ptp)
>  		return;
>
> [ ... ]
>
>  	kfree(aq_ptp->ptp_info.pin_config);
> +	aq_ptp->ptp_info.pin_config = NULL;
>  
>  	netif_napi_del(&aq_ptp->napi);
>  	kfree(aq_ptp);
> +	aq_utils_obj_clear(&aq_nic->aq_hw->flags, AQ_HW_PTP_AVAILABLE);
>  	aq_nic->aq_ptp = NULL;
>  }
>
> [Low]
> A few small issues in this teardown:
>
>  - kfree(aq_ptp) is called before aq_nic->aq_ptp = NULL, leaving
>    aq_nic->aq_ptp dangling for the duration of the
>    aq_utils_obj_clear() call between them.
>
>  - mutex_destroy(&aq_ptp->ptp_filter_lock) is missing before kfree;
>    CONFIG_DEBUG_MUTEXES would have caught this.

Fixed both points. mutex_destroy(&aq_ptp->ptp_filter_lock) is now called
before kfree, and the teardown order is corrected: AQ_HW_PTP_AVAILABLE
is cleared and aq_nic->aq_ptp is set to NULL before kfree(aq_ptp), eliminating
the dangling pointer window.