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

[Sukhdeep Soni [C]]

On 22nd May, 2026 Jakub Kicinski wrote:

> net: atlantic: add AQC113 PTP support in aq_ptp and driver core
>
> This commit adds PTP support for the AQC113 hardware. It updates PTP state
> initialization, refactors hardware filter configurations to support L3/L4 PTP
> multicast rules, and registers new per-instance offset tables.
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index ef94478100716..f1ce11c6a3d1e 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>
> [ ... ]
>
> @@ -793,6 +822,17 @@ 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 &&
> +	    aq_ptp_ring(self, ring)) {
> +		clk_sel = (s32)self->aq_hw_ops->hw_get_clk_sel(self->aq_hw);
> +		if (clk_sel < 0)
> +			goto exit;
>
> [Severity: High]
> If hw_get_clk_sel() returns a negative value, this skips setting dx_buff->skb
> and dx_buff->xdpf to NULL because of the early exit. Since the descriptors
> are reused, dx_buff->skb might contain a stale pointer.
>
> Could this cause a use-after-free when aq_ring_tx_clean() tries to free the
> stale skb? Also, does this leak the current skb since it was DMA mapped but
> not stored?

Valid observation ? this was a real bug and will be fixed in v4. dx_buff->skb = skb
and dx_buff->xdpf = NULL are now set unconditionally before the PTP timestamp
block, so the goto exit on a negative clk_sel is safe: the skb is stored in the descriptor
and aq_ring_tx_clean() will free it correctly. No stale pointer, no leak.

> +		dx_buff->request_ts = 1U;
> +		dx_buff->clk_sel = (u32)clk_sel;
> +		ring->ptp_ts_deadline = jiffies + HZ;
> +	}
>  	dx_buff->skb = skb;
>  	dx_buff->xdpf = NULL;
>  	goto exit;
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> index 7486a28d7ff8f..31373c20782cc 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
>
> [ ... ]
>
> +static int aq_ptp_dpath_enable(struct aq_ptp_s *aq_ptp,
> +			       int enable_flags, u16 rx_queue)
> +{
>
> [ ... ]
>
> +	if (enable_flags) {
> +		if (enable_flags & (AQ_HW_PTP_L4_ENABLE)) {
> +			if (aq_ptp->a1_ptp) {
>
> [ ... ]
>
> +		if (enable_flags & AQ_HW_PTP_L2_ENABLE) {
> +			aq_ptp->eth_type_filter.ethertype = ETH_P_1588;
> +			aq_ptp->eth_type_filter.queue = rx_queue;
> +		}
> +
> +		if (hw_ops->hw_filter_l3l4_set) {
>
> [ ... ]
>
> +		if (!err && (enable_flags & AQ_HW_PTP_L2_ENABLE) &&
> +		    hw_ops->hw_filter_l2_set) {
>
> [Severity: Medium]
> If userspace initially requests both L2 and L4 filters, and later changes the
> configuration to L2-only, the enable_flags will only have AQ_HW_PTP_L2_ENABLE
> set.
>
> This executes the if (enable_flags) branch and updates the L2 filters, but
> skips clearing the L4 hardware filters.
>
> Will this leave stale L4 filters active in the hardware?

Valid concern ? this was a real bug and is already fixed. At the start of the
if (enable_flags) block, all existing L4 filters are now explicitly cleared in hardware
before applying the new configuration:
 
  /* 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]);
  }
 
This ensures an L2+L4 → L2-only transition always clears the stale L4 hardware
filters before setting the new ones.

> +			err = hw_ops->hw_filter_l2_set(aq_nic->aq_hw,
> +					&aq_ptp->eth_type_filter);
>
> [ ... ]
>
>  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);
>
> [Severity: Medium]
> Is it possible for the hardware and software states to become out of sync here?
>
> The new ptp_filter_lock protects the hardware configuration, but the software
> state (aq_ptp->hwtstamp_config.rx_filter) doesn't seem to be updated under this
> lock.
>
> If a link status change occurs concurrently:
>
> aq_ptp_clock_init() {
>    ...
>    aq_ptp_dpath_enable(aq_ptp, ptp_en_flags, aq_ptp->ptp_rx.idx);
> }
>
> Could aq_ptp_clock_init() read the old software state and restore stale filters
> right after the ioctl configures the new ones?

Valid concern ? aq_ptp->hwtstamp_config is now updated
inside ptp_filter_lock, only on success:
 
  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);
 
The link-bounce path (aq_ptp_clock_init()) also holds ptp_filter_lock around
its aq_ptp_dpath_enable() call, so it cannot race with the ioctl path ? it will
either see the old config before the lock or the fully updated config after it.

> +		err = aq_ptp_dpath_enable(aq_ptp,
> +					  ptp_en_flags,
> +					  aq_ptp->ptp_rx.idx);
> +		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 {
>
> [Severity: High]
> Does this break TX hardware timestamping if RX filtering is disabled?
>
> If a user configures pure TX timestamping (tx_type = HWTSTAMP_TX_ON) but
> rx_filter = HWTSTAMP_FILTER_NONE, ptp_en_flags evaluates to AQ_HW_PTP_DISABLE.
> This clears AQ_NIC_PTP_DPATH_UP, which will prevent the datapath from routing
> packets to the PTP TX ring.
> 
> The old code correctly preserved this flag by checking
> config->tx_type == HWTSTAMP_TX_ON.

The AQC113 hardware does not support isolated TX-only timestamping ? the
PTP datapath requires RX filters to be active for TX timestamping to function
correctly. Setting AQ_NIC_PTP_DPATH_UP based solely on ptp_en_flags (RX filter)
is intentional and matches the vendor reference driver behavior. A
tx_type=HWTSTAMP_TX_ON with rx_filter=HWTSTAMP_FILTER_NONE configuration
is not a valid use case for this hardware.

> -		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);
> -	}