Re: [PATCH 0/3] net/sfc: miscellaneous bug fixes

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.