Re: [PATCH net-next 2/9] net: atlantic: move active_ipv4/ipv6 bitmap updates after HW write updates after HW write

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: atlantic: move active_ipv4/ipv6 bitmap updates after HW write updates after HW write

This change moves the active_ipv4/active_ipv6 bitmap updates out of
aq_set_data_fl3l4() and into aq_add_del_fl3l4() after the HW write, and
removes the bitmap manipulation from aq_nic_reserve_filter() and
aq_nic_release_filter().

> net: atlantic: move active_ipv4/ipv6 bitmap updates after HW write updates after HW write

The subject line repeats "updates after HW write" twice.  Was this
intended, or should it read "net: atlantic: move active_ipv4/ipv6 bitmap
updates after HW write"?

A few other subjects in this series look like they have the same
copy-paste pattern (for example "masking and IPv6 handling masking and
IPv6 handling", "driver internals driver internals", "TX path setup TX
path setup", "(Antigua) (Antigua)") and may want the same cleanup.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index ef9447810071..3cec853e9fad 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -1522,8 +1522,6 @@ u8 aq_nic_reserve_filter(struct aq_nic_s *self, enum aq_rx_filter_type type)
>  	case aq_rx_filter_l3l4:
>  		fltr_cnt = AQ_RX_LAST_LOC_FL3L4 - AQ_RX_FIRST_LOC_FL3L4;
>  		n_bit = fltr_cnt - self->aq_hw_rx_fltrs.fl3l4.reserved_count;
> -
> -		self->aq_hw_rx_fltrs.fl3l4.active_ipv4 |= BIT(n_bit);
>  		self->aq_hw_rx_fltrs.fl3l4.reserved_count++;
>  		location = n_bit;
>  		break;

Could dropping the active_ipv4 update here regress PTP's IPv4/IPv6
collision protection?

The PTP path calls aq_nic_reserve_filter(aq_rx_filter_l3l4) from
aq_ptp_init() to grab a single L3L4 slot, and then programs its IPv4 UDP
filter by calling hw_ops->hw_filter_l3l4_set() directly from
aq_ptp_hwtstamp_config_set() / aq_ptp_prepare_filters() — it never goes
through aq_add_del_fl3l4() where the new bitmap update now lives.

Before this change, aq_nic_reserve_filter() set:

    self->aq_hw_rx_fltrs.fl3l4.active_ipv4 |= BIT(n_bit);

which caused aq_check_approve_fl3l4() to reject any later IPv6 L3L4 rule,
protecting PTP's reserved slot.

After this change, active_ipv4 stays zero while PTP is actively filtering
IPv4 in hardware.  Can a user now add an IPv6 L3L4 rule at
AQ_RX_FIRST_LOC_FL3L4 + 4 (still within AQ_RX_LAST_LOC_FL3L4) and have
the IPv6 filter program HW slots 4..7, silently overwriting PTP's IPv4
filter at HW slot 7?

I looked through the rest of the series including the AQC113 PTP patches
(aq_ptp_dpath_enable / aq_ptp_add_a2_filter) and they also call
hw_filter_l3l4_set() directly rather than going through
aq_add_del_fl3l4(), so the reserved PTP slot does not get re-reflected
into active_ipv4 by a later patch.

> @@ -1543,7 +1541,6 @@ void aq_nic_release_filter(struct aq_nic_s *self, enum aq_rx_filter_type type,
>  		break;
>  	case aq_rx_filter_l3l4:
>  		self->aq_hw_rx_fltrs.fl3l4.reserved_count--;
> -		self->aq_hw_rx_fltrs.fl3l4.active_ipv4 &= ~BIT(location);
>  		break;

Would it be worth keeping these updates (or adding equivalent ones on the
PTP reserve/release path) so that aq_check_approve_fl3l4() continues to
see PTP's reserved slot as active_ipv4?
-- 
This is an AI-generated review.