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

[Sukhdeep Soni [C]]

On 12 May 2026, Paolo Abeni wrote:

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

The accept-all-multicast enable has been removed from v3. The remaining
 changes — PTP TC buffer reservation, PCIe extended-tag-gated TX
 read-request limits, and the RSS refactor — are kept in one commit.
 The PTP TC reservation and TX path improvements are functionally
 related: the TX read-request limit increase benefits PTP TC throughput
 and is most naturally introduced alongside the TC setup. The RSS
 simplification is a one-liner mechanical refactor touching the same
 file; splitting it into a separate commit adds overhead without
 meaningful review benefit. If the preference is strict separation we
 can split the RSS change in v4.

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

Correct — the commit message is misleading. The code only reads
hw_atl2_phi_ext_tag_get() to check whether extended PCIe tag mode
is already enabled by firmware/BIOS, and if so raises the TX data and
descriptor read-request limits. The driver never calls hw_atl2_phi_ext_tag_set()
to enable it. Will reword in v3 to: "Increase TX data and descriptor
read-request limits when firmware has already enabled extended PCIe tag mode."

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

The hw_atl_rpfl2_accept_all_mc_packets_set(self, 1) call has been
 removed from the code in v3 and the commit message will be updated
 to remove that bullet point accordingly.

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