On Tue, 11 Aug 2026 21:49:10 +0400
Ivan Malov <[email protected]> wrote:
> Three independent fixes. The first ensures that Rx queue
> type flags are derived from scratch on every queue setup,
> preventing flags from a prior configuration persisting
> when an offload is disabled.
>
> The second patch removes an erroneous static qualifier
> from a flow RSS iterator variable, which incorrectly
> shared state between multiple interfaces.
>
> The third patch corrects reading of the advertised
> autoneg capability. When the user disables it, the
> corresponding bit was re-added upon the next
> link-state query.
>
> Ivan Malov (3):
> net/sfc: set Rx queue type flags from scratch on queue setup
> net/sfc: drop wrong static qualifier from iterator variable
> common/sfc_efx/base: fix reading advertised autoneg ability
>
> drivers/common/sfc_efx/base/efx_np.c | 11 +++++------
> drivers/common/sfc_efx/base/medford4_phy.c | 6 +++++-
> drivers/net/sfc/sfc_flow_rss.c | 2 +-
> drivers/net/sfc/sfc_rx.c | 2 +-
> 4 files changed, 12 insertions(+), 9 deletions(-)
>
Detailed AI review found some issues here.
Review of [PATCH 0/3] SFC bug fixes (Ivan Malov)
Patch 1/3 - net/sfc: set Rx queue type flags from scratch on queue setup
Error: replacing "|=" with "=" discards extra type flags that callers
seed via sfc_rx_qinit_info() immediately before sfc_rx_qinit().
Two call sites do this today:
drivers/net/sfc/sfc_repr_proxy.c:556
sfc_rx_qinit_info(sa, rxq->sw_index, EFX_RXQ_FLAG_INGRESS_MPORT);
sfc_rx_qinit(sa, rxq->sw_index, ...); /* line 562 */
drivers/net/sfc/sfc_mae_counter.c:870
sfc_rx_qinit_info(sa, sa->counter_rxq.sw_index,
EFX_RXQ_FLAG_USER_MARK);
sfc_rx_qinit(sa, sa->counter_rxq.sw_index, ...); /* line 875 */
sfc_rx_qinit_info() does "rxq_info->type_flags = extra_efx_type_flags"
(sfc_rx.c:1663). The "|=" at sfc_rx.c:1186 was what preserved that
value; with "=" it is overwritten before anything reads it.
Two consequences:
- rxq_info->type_flags is passed straight to efx_rx_qcreate()
(sfc_rx.c:822 and :842), so the hardware Rx prefix no longer
carries ingress mport / user mark for those queues.
- sfc_rx.c:1240 derives SFC_RXQ_FLAG_INGRESS_MPORT from type_flags,
and sfc_ef100_rx.c:824 consumes it. Representor proxy demux
loses its mport field.
EFX_RXQ_FLAG_USER_MARK is re-added at sfc_rx.c:1203, but only when
RTE_ETH_RX_METADATA_USER_MARK was negotiated or flow tunnel is active
- neither holds for the MAE counter queue.
The underlying problem the patch describes is real: sfc_rx_configure()
only calls sfc_rx_qinit_info() for newly added queues (sfc_rx.c:1837,
inside "while (sas->ethdev_rxq_count < nb_rx_queues)"), so existing
ethdev queues keep stale flags across a reconfigure. The fix needs to
reset only the offload-derived bits. Suggested approach: record the
caller-supplied flags in a separate field, e.g.
/* sfc_rx_qinit_info() */
rxq_info->extra_type_flags = extra_efx_type_flags;
rxq_info->type_flags = extra_efx_type_flags;
/* sfc_rx_qinit() */
rxq_info->type_flags = rxq_info->extra_type_flags |
((offloads & RTE_ETH_RX_OFFLOAD_SCATTER) ?
EFX_RXQ_FLAG_SCATTER : EFX_RXQ_FLAG_NONE);
Patch 2/3 - net/sfc: drop wrong static qualifier from iterator variable
Info: the change is correct, but the commit message overstates the
impact. TAILQ_FOREACH() assigns the variable from TAILQ_FIRST()
before the first iteration, and every return path returns a value
produced inside the loop, so the static storage is never read stale.
There is no observable misbehaviour to backport a fix for. Consider
rewording as a cleanup (unnecessary global state, not thread-safe by
construction) and dropping the Cc: stable and Fixes: tags, or state
explicitly that no functional change is expected.
Patch 3/3 - common/sfc_efx/base: fix reading advertised autoneg ability
Error: the first efx_np.c hunk does not apply to main. The patch
expects the block
if (lsp->enls_an_supported != B_FALSE)
lsp->enls_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;
to sit after the LINK_STATE_OUT_ADVERTISED_ABILITIES conversion, but
upstream has it before it (efx_np.c:432), and at the position the
patch expects upstream has
if (status_flags & (1U << MC_CMD_LINK_STATUS_FLAGS_AN_ABLE))
lsp->enls_lp_cap_mask |= 1U << EFX_PHY_CAP_AN;
from 70857163b72a ("common/sfc_efx/base: fix autoneg detection with
netport MCDI"). git am and git apply -3 both fail on that hunk; the
efx_np_attach() hunk and medford4_phy.c apply cleanly. Please rebase
on main and resend.
The logic itself checks out on the rebased placement. Removing the
AN bit from enls_adv_cap_mask leaves only two consumers, and both are
covered: efx_np_attach() (efx_np.c:1005) now sets the bit itself, and
medford4_phy_get_link() (medford4_phy.c:43) restores it from
ep_adv_cap_mask. medford4_mac_poll() writes the result back into
ep_adv_cap_mask, so the bit is self-sustaining, and clearing it via
efx_phy_adv_cap_set() sticks. Setting it back is gated on
ep_phy_cap_mask (efx_phy.c:264), which attach only populates when
AN is supported, so the preserved bit cannot outlive AN support.
Info: the added local
const efx_port_t *port = &enp->en_port;
is used once and the file otherwise reaches through enp->en_port
directly (line 33) or names the local "epp" (medford4_phy_reconfigure,
medford4_mac_poll). Suggest dropping it:
preserve_an = enp->en_port.ep_adv_cap_mask &
(1U << EFX_PHY_CAP_AN);
Fixes: tags in all three patches resolve to real commits with matching
subjects.
