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

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

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

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

[ ... ]

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

[ ... ]

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

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

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

[ ... ]

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

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

[ ... ]

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

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

[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)

[ ... ]

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