Re: [PATCH net-next 7/9] net: atlantic: add AQC113 PTP traffic class and TX path setup TX path setup

[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: add AQC113 PTP traffic class and TX path setup TX path setup

Reserves PTP TX/RX buffer space, configures the PTP traffic class
with no flow control and highest priority, enables multicast frame
tagging for ART-based PTP steering, and bumps TX read-request limits
when the PCIe extended-tag bit is already set.

> net: atlantic: add AQC113 PTP traffic class and TX path setup TX path setup

The subject contains the phrase "TX path setup" twice. Is that
intentional?

The patch also bundles several unrelated changes into a single commit:
PTP TC buffer reservation, an unconditional accept-all-multicast
enable, PCIe-extended-tag-gated TX read-request limit bumps, and a
non-functional RSS refactor. Would it be reasonable to split these
into separate commits so each can be reviewed, tested, and bisected
independently?

> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> index e58bfff38670..c71e8d1adfc9 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2.c

[ ... ]

> @@ -415,9 +441,20 @@ static int hw_atl2_hw_init_tx_path(struct aq_hw_s *self)
>  
>  	hw_atl2_tpb_tx_buf_clk_gate_en_set(self, 0U);
>  
> +	if (hw_atl2_phi_ext_tag_get(self)) {
> +		hw_atl2_tdm_tx_data_read_req_limit_set(self, 0x7F);
> +		hw_atl2_tdm_tx_desc_read_req_limit_set(self, 0x0F);
> +	}
> +
>  	return aq_hw_err_from_flags(self);
>  }

The commit message says:

    Enable extended PCIe tag mode (32-255) when hardware supports it,
    with increased TX data and descriptor read request limits for
    improved throughput.

Does the code actually enable extended PCIe tag mode? The call here is
hw_atl2_phi_ext_tag_get(), a read accessor via aq_hw_read_reg_bit().
The corresponding setter hw_atl2_phi_ext_tag_set() exists in
hw_atl2_llh.c but does not appear to be invoked in this patch or
elsewhere. The code path only raises the TX read-request limits when
the bit is already set by firmware/BIOS. Should the commit message be
reworded to reflect that behavior?

>  
> +/* Initialise new rx filters
> + * L2 promisc OFF
> + * VLAN promisc OFF
> + *
> + * User priority to TC
> + */
>  static void hw_atl2_hw_init_new_rx_filters(struct aq_hw_s *self)
>  {
>  	u8 *prio_tc_map = self->aq_nic_cfg->prio_tc_map;
> @@ -429,6 +466,9 @@ static void hw_atl2_hw_init_new_rx_filters(struct aq_hw_s *self)
>  	u8 index;
>  	int i;
>  
> +	/* tag MC frames always */
> +	hw_atl_rpfl2_accept_all_mc_packets_set(self, 1);
> +

Is the "tag MC frames always" intent actually durable?

The same hardware bit is rewritten unconditionally by the packet
filter path. In hw_atl_b0_hw_packet_filter_set() (shared by atl2
through hw_atl_b0.c):

    hw_atl_rpfl2_accept_all_mc_packets_set(self,
                          IS_FILTER_ENABLED(IFF_ALLMULTI) &&
                          IS_FILTER_ENABLED(IFF_MULTICAST));

ndo_set_rx_mode runs as part of normal interface bring-up and on every
multicast-list change, so the init-time write to 1 would be cleared to
0 for any interface not in allmulticast mode. Does this mean the ART
cannot match PTP multicast traffic once rx_mode has been programmed,
which is the commit's stated purpose of this write?

If the intent is to keep the bit set whenever PTP is enabled, should
the packet-filter path OR in cfg->is_ptp, rather than (or in addition
to) writing the bit once at init?

The write is also not gated by cfg->is_ptp. While it is briefly in
effect, does enabling accept_all_mc change multicast filtering
behavior for non-PTP users in a way that needs justification in the
commit log?

[ ... ]
-- 
This is an AI-generated review.