Re: [PATCH net-next v3 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 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?

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

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

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

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