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

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

[Ivan Malov]

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(-)

-- 
2.47.3

[PATCH 1/3] net/sfc: set Rx queue type flags from scratch on queue setup

[Ivan Malov]

Derive queue type flags from scratch on every queue setup
so that flags left over from a previous configuration do
not persist when an offload is disabled.

Fixes: b8cf5ba549f2 ("net/sfc: support initialising different Rx queue types")
Cc: stable@dpdk.org

Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
Reviewed-by: Andy Moreton <andy.moreton@amd.com>
---
 drivers/net/sfc/sfc_rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
index d8b961ff7e..e049beced5 100644
--- a/drivers/net/sfc/sfc_rx.c
+++ b/drivers/net/sfc/sfc_rx.c
@@ -1183,7 +1183,7 @@ sfc_rx_qinit(struct sfc_adapter *sa, sfc_sw_index_t sw_index,
 	else
 		rxq_info->type = EFX_RXQ_TYPE_DEFAULT;
 
-	rxq_info->type_flags |=
+	rxq_info->type_flags =
 		(offloads & RTE_ETH_RX_OFFLOAD_SCATTER) ?
 		EFX_RXQ_FLAG_SCATTER : EFX_RXQ_FLAG_NONE;
 
-- 
2.47.3

[PATCH 2/3] net/sfc: drop wrong static qualifier from iterator variable

[Ivan Malov]

Obviously, this should be per interface state.
Static wrongly turns it into global state.

Fixes: 6da67e706dc9 ("net/sfc: rework flow action RSS support")
Cc: stable@dpdk.org

Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
Reviewed-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
Reviewed-by: Andy Moreton <andy.moreton@amd.com>
---
 drivers/net/sfc/sfc_flow_rss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/sfc/sfc_flow_rss.c b/drivers/net/sfc/sfc_flow_rss.c
index 8e2749833b..f19d6f00c5 100644
--- a/drivers/net/sfc/sfc_flow_rss.c
+++ b/drivers/net/sfc/sfc_flow_rss.c
@@ -202,7 +202,7 @@ sfc_flow_rss_ctx_reuse(struct sfc_adapter *sa,
 		       uint16_t sw_qid_min, const uint16_t *sw_qids)
 {
 	struct sfc_flow_rss *flow_rss = &sa->flow_rss;
-	static struct sfc_flow_rss_ctx *ctx;
+	struct sfc_flow_rss_ctx *ctx;
 
 	SFC_ASSERT(sfc_adapter_is_locked(sa));
 
-- 
2.47.3

[PATCH 3/3] common/sfc_efx/base: fix reading advertised autoneg ability

[Ivan Malov]

The issue is that when the user disables auto-negotiation by removing
the capability bit from the 'advertised mask' (set method) and then
reads the resulting capabilities, which involves querying MCDI, the
bit reappears in the mask irrespective of the user's intent.

Fix this by remembering the user's intent before any link-state queries.

Fixes: 2a5cf77e6de8 ("common/sfc_efx/base: provide PHY link get method on Medford4")
Fixes: 06f569de6c06 ("common/sfc_efx/base: decode netport link state on probe path")
Cc: stable@dpdk.org

Suggested-by: Andy Moreton <andy.moreton@amd.com>
Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
Reviewed-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
Reviewed-by: Andy Moreton <andy.moreton@amd.com>
---
 drivers/common/sfc_efx/base/efx_np.c       | 11 +++++------
 drivers/common/sfc_efx/base/medford4_phy.c |  6 +++++-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx_np.c b/drivers/common/sfc_efx/base/efx_np.c
index 4ba3c7d260..a8396535bf 100644
--- a/drivers/common/sfc_efx/base/efx_np.c
+++ b/drivers/common/sfc_efx/base/efx_np.c
@@ -436,9 +436,6 @@ efx_np_link_state(
 	    MCDI_OUT2(req, const uint8_t, LINK_STATE_OUT_ADVERTISED_ABILITIES),
 	    &lsp->enls_adv_cap_mask);
 
-	if (lsp->enls_an_supported != B_FALSE)
-		lsp->enls_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;
-
 	efx_np_cap_hw_data_to_sw_mask(
 	    MCDI_OUT2(req, const uint8_t,
 		    LINK_STATE_OUT_LINK_PARTNER_ABILITIES),
@@ -1020,11 +1017,13 @@ efx_np_attach(
 	if (rc != 0)
 		goto fail3;
 
-	if (ls.enls_an_supported != B_FALSE)
-		epp->ep_phy_cap_mask |= 1U << EFX_PHY_CAP_AN;
-
 	epp->ep_adv_cap_mask = ls.enls_adv_cap_mask;
 
+	if (ls.enls_an_supported != B_FALSE) {
+		epp->ep_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;
+		epp->ep_phy_cap_mask |= 1U << EFX_PHY_CAP_AN;
+	}
+
 #if EFSYS_OPT_LOOPBACK
 	efx_np_assign_loopback_props(enp);
 #endif /* EFSYS_OPT_LOOPBACK */
diff --git a/drivers/common/sfc_efx/base/medford4_phy.c b/drivers/common/sfc_efx/base/medford4_phy.c
index 7b456c9b8a..4d2a954d4f 100644
--- a/drivers/common/sfc_efx/base/medford4_phy.c
+++ b/drivers/common/sfc_efx/base/medford4_phy.c
@@ -32,15 +32,19 @@ medford4_phy_get_link(
 	__out		ef10_link_state_t *elsp)
 {
 	efx_np_handle_t nph = enp->en_port.ep_np_handle;
+	const efx_port_t *port = &enp->en_port;
 	efx_np_link_state_t ls;
 	efx_np_mac_state_t ms;
+	uint32_t preserve_an;
 	efx_rc_t rc;
 
+	preserve_an = port->ep_adv_cap_mask & (1U << EFX_PHY_CAP_AN);
+
 	rc = efx_np_link_state(enp, nph, &ls);
 	if (rc != 0)
 		goto fail1;
 
-	elsp->epls.epls_adv_cap_mask = ls.enls_adv_cap_mask;
+	elsp->epls.epls_adv_cap_mask = ls.enls_adv_cap_mask | preserve_an;
 	elsp->epls.epls_lp_cap_mask = ls.enls_lp_cap_mask;
 	elsp->epls.epls_lane_count = ls.enls_lane_count;
 	elsp->els_loopback = ls.enls_loopback;
-- 
2.47.3

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

[Stephen Hemminger]

On Tue, 11 Aug 2026 21:49:10 +0400
Ivan Malov <ivan.malov@arknetworks.am> 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.

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

[Ivan Malov]

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.


v2:

- note for the future AI reviews: apply this on top of
  the 'common/sfc_efx/base: fix code analysis issues' series

- addressed https://mails.dpdk.org/archives/dev/2026-August/343072.html
  -- fixed the queue type flags so that it accounts for the extra flags
  -- the rest of the notes do not point at factual defects


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                   |  8 ++++----
 drivers/net/sfc/sfc_rx.h                   |  1 +
 5 files changed, 16 insertions(+), 12 deletions(-)

-- 
2.47.3

[PATCH v2 1/3] net/sfc: set Rx queue type flags from scratch on queue setup

[Ivan Malov]

Derive queue type flags from scratch on every queue setup
so that flags left over from a previous configuration do
not persist when an offload is disabled.

Fixes: b8cf5ba549f2 ("net/sfc: support initialising different Rx queue types")
Cc: stable@dpdk.org

Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
Reviewed-by: Andy Moreton <andy.moreton@amd.com>
---
 drivers/net/sfc/sfc_rx.c | 8 ++++----
 drivers/net/sfc/sfc_rx.h | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
index d8b961ff7e..1d4101b9e9 100644
--- a/drivers/net/sfc/sfc_rx.c
+++ b/drivers/net/sfc/sfc_rx.c
@@ -1183,9 +1183,9 @@ sfc_rx_qinit(struct sfc_adapter *sa, sfc_sw_index_t sw_index,
 	else
 		rxq_info->type = EFX_RXQ_TYPE_DEFAULT;
 
-	rxq_info->type_flags |=
-		(offloads & RTE_ETH_RX_OFFLOAD_SCATTER) ?
-		EFX_RXQ_FLAG_SCATTER : EFX_RXQ_FLAG_NONE;
+	rxq_info->type_flags = rxq_info->extra_type_flags |
+		((offloads & RTE_ETH_RX_OFFLOAD_SCATTER) ?
+		EFX_RXQ_FLAG_SCATTER : EFX_RXQ_FLAG_NONE);
 
 	if ((encp->enc_tunnel_encapsulations_supported != 0) &&
 	    (sfc_dp_rx_offload_capa(sa->priv.dp_rx) &
@@ -1660,7 +1660,7 @@ sfc_rx_qinit_info(struct sfc_adapter *sa, sfc_sw_index_t sw_index,
 	SFC_ASSERT(rte_is_power_of_2(max_entries));
 
 	rxq_info->max_entries = max_entries;
-	rxq_info->type_flags = extra_efx_type_flags;
+	rxq_info->extra_type_flags = extra_efx_type_flags;
 
 	return 0;
 }
diff --git a/drivers/net/sfc/sfc_rx.h b/drivers/net/sfc/sfc_rx.h
index 4ab513915e..bd189e6a56 100644
--- a/drivers/net/sfc/sfc_rx.h
+++ b/drivers/net/sfc/sfc_rx.h
@@ -110,6 +110,7 @@ struct sfc_rxq_info {
 	unsigned int		entries;
 	efx_rxq_type_t		type;
 	unsigned int		type_flags;
+	unsigned int		extra_type_flags;
 	struct sfc_dp_rxq	*dp;
 	boolean_t		deferred_start;
 	boolean_t		deferred_started;
-- 
2.47.3

[PATCH v2 2/3] net/sfc: drop wrong static qualifier from iterator variable

[Ivan Malov]

Obviously, this should be per interface state.
Static wrongly turns it into global state.

Fixes: 6da67e706dc9 ("net/sfc: rework flow action RSS support")
Cc: stable@dpdk.org

Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
Reviewed-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
Reviewed-by: Andy Moreton <andy.moreton@amd.com>
---
 drivers/net/sfc/sfc_flow_rss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/sfc/sfc_flow_rss.c b/drivers/net/sfc/sfc_flow_rss.c
index 8e2749833b..f19d6f00c5 100644
--- a/drivers/net/sfc/sfc_flow_rss.c
+++ b/drivers/net/sfc/sfc_flow_rss.c
@@ -202,7 +202,7 @@ sfc_flow_rss_ctx_reuse(struct sfc_adapter *sa,
 		       uint16_t sw_qid_min, const uint16_t *sw_qids)
 {
 	struct sfc_flow_rss *flow_rss = &sa->flow_rss;
-	static struct sfc_flow_rss_ctx *ctx;
+	struct sfc_flow_rss_ctx *ctx;
 
 	SFC_ASSERT(sfc_adapter_is_locked(sa));
 
-- 
2.47.3

[PATCH v2 3/3] common/sfc_efx/base: fix reading advertised autoneg ability

[Ivan Malov]

The issue is that when the user disables auto-negotiation by removing
the capability bit from the 'advertised mask' (set method) and then
reads the resulting capabilities, which involves querying MCDI, the
bit reappears in the mask irrespective of the user's intent.

Fix this by remembering the user's intent before any link-state queries.

Fixes: 2a5cf77e6de8 ("common/sfc_efx/base: provide PHY link get method on Medford4")
Fixes: 06f569de6c06 ("common/sfc_efx/base: decode netport link state on probe path")
Cc: stable@dpdk.org

Suggested-by: Andy Moreton <andy.moreton@amd.com>
Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
Reviewed-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
Reviewed-by: Andy Moreton <andy.moreton@amd.com>
---
 drivers/common/sfc_efx/base/efx_np.c       | 11 +++++------
 drivers/common/sfc_efx/base/medford4_phy.c |  6 +++++-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx_np.c b/drivers/common/sfc_efx/base/efx_np.c
index 293f587892..f35d6aaee1 100644
--- a/drivers/common/sfc_efx/base/efx_np.c
+++ b/drivers/common/sfc_efx/base/efx_np.c
@@ -436,9 +436,6 @@ efx_np_link_state(
 	    MCDI_OUT2(req, const uint8_t, LINK_STATE_OUT_ADVERTISED_ABILITIES),
 	    &lsp->enls_adv_cap_mask);
 
-	if (lsp->enls_an_supported != B_FALSE)
-		lsp->enls_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;
-
 	efx_np_cap_hw_data_to_sw_mask(
 	    MCDI_OUT2(req, const uint8_t,
 		    LINK_STATE_OUT_LINK_PARTNER_ABILITIES),
@@ -1022,11 +1019,13 @@ efx_np_attach(
 	if (rc != 0)
 		goto fail3;
 
-	if (ls.enls_an_supported != B_FALSE)
-		epp->ep_phy_cap_mask |= 1U << EFX_PHY_CAP_AN;
-
 	epp->ep_adv_cap_mask = ls.enls_adv_cap_mask;
 
+	if (ls.enls_an_supported != B_FALSE) {
+		epp->ep_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;
+		epp->ep_phy_cap_mask |= 1U << EFX_PHY_CAP_AN;
+	}
+
 #if EFSYS_OPT_LOOPBACK
 	efx_np_assign_loopback_props(enp);
 #endif /* EFSYS_OPT_LOOPBACK */
diff --git a/drivers/common/sfc_efx/base/medford4_phy.c b/drivers/common/sfc_efx/base/medford4_phy.c
index 7b456c9b8a..4d2a954d4f 100644
--- a/drivers/common/sfc_efx/base/medford4_phy.c
+++ b/drivers/common/sfc_efx/base/medford4_phy.c
@@ -32,15 +32,19 @@ medford4_phy_get_link(
 	__out		ef10_link_state_t *elsp)
 {
 	efx_np_handle_t nph = enp->en_port.ep_np_handle;
+	const efx_port_t *port = &enp->en_port;
 	efx_np_link_state_t ls;
 	efx_np_mac_state_t ms;
+	uint32_t preserve_an;
 	efx_rc_t rc;
 
+	preserve_an = port->ep_adv_cap_mask & (1U << EFX_PHY_CAP_AN);
+
 	rc = efx_np_link_state(enp, nph, &ls);
 	if (rc != 0)
 		goto fail1;
 
-	elsp->epls.epls_adv_cap_mask = ls.enls_adv_cap_mask;
+	elsp->epls.epls_adv_cap_mask = ls.enls_adv_cap_mask | preserve_an;
 	elsp->epls.epls_lp_cap_mask = ls.enls_lp_cap_mask;
 	elsp->epls.epls_lane_count = ls.enls_lane_count;
 	elsp->els_loopback = ls.enls_loopback;
-- 
2.47.3

Re: [PATCH v2 3/3] common/sfc_efx/base: fix reading advertised autoneg ability

[Stephen Hemminger]

On Wed, 12 Aug 2026 21:10:17 +0400
Ivan Malov <ivan.malov@arknetworks.am> wrote:

> The issue is that when the user disables auto-negotiation by removing
> the capability bit from the 'advertised mask' (set method) and then
> reads the resulting capabilities, which involves querying MCDI, the
> bit reappears in the mask irrespective of the user's intent.
> 
> Fix this by remembering the user's intent before any link-state queries.
> 
> Fixes: 2a5cf77e6de8 ("common/sfc_efx/base: provide PHY link get method on Medford4")
> Fixes: 06f569de6c06 ("common/sfc_efx/base: decode netport link state on probe path")
> Cc: stable@dpdk.org
> 
> Suggested-by: Andy Moreton <andy.moreton@amd.com>
> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
> Reviewed-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
> Reviewed-by: Andy Moreton <andy.moreton@amd.com>

This patch depends on the previous common series and therefore can
not be easily backported to stable.

There is a way to mark patch dependencies using Depends-on:
please use that instead of instructions to AI.

AI says that in its usual over the top wordy way...

Patch 3/3 - common/sfc_efx/base: fix reading advertised autoneg ability

  Warning: this patch depends on the pending common/sfc series for its
  diff context only, and that dependency creates a backport conflict.

  The efx_np.c hunk removes

      if (lsp->enls_an_supported != B_FALSE)
              lsp->enls_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;

  from a position after the LINK_STATE_OUT_ADVERTISED_ABILITIES
  conversion.  On main and on every stable branch the block is still
  before that conversion, where 06f569de6c06 originally put it.  Since
  efx_np_cap_mask_hw_to_sw() ORs into *sw_cap_maskp (efx_np.c:197)
  rather than assigning, the block's position has no effect on the
  resulting mask - the pending series is only moving text.

  Both Fixes: commits first appear in v25.07, so this needs to reach
  25.11 LTS and 25.07, neither of which has the move.  Stable will hit
  a conflict on a hunk whose resolution is not obvious from the diff.

  Suggest sending this fix ahead of the series that moves the block,
  or as a standalone patch against main.  It then applies unchanged
  everywhere, and the other series absorbs a zero-cost rebase.  If the
  current ordering has to stand, please post an explicit backport to
  stable@dpdk.org rather than leaving the resolution to the stable
  maintainer.

  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:

      preserve_an = enp->en_port.ep_adv_cap_mask &
                    (1U << EFX_PHY_CAP_AN);

Re: [PATCH v2 3/3] common/sfc_efx/base: fix reading advertised autoneg ability

[Ivan Malov]

Dear Stephen,

If I may, I should like to address the following:

- Backport conflict and 'Depends-on:':
   The stable team operates independently of the upstream review process; should the patch not apply cleanly, the stable maintainers will contact me directly (a well-established practice) and I am entirely willing to provide all the assistance at the appropriate juncture. In other words, the 'Cc: stable@dpdk.org' signals that a backport is desirable, not that it must apply without manual resolution. Upstream acceptance is not contingent on the mechanics of a stable backport, which falls outside the scope of this review.

   As to 'Depends-on:': the dependency here is purely contextual. It is one of hunk context, not of function; the fix is semantically correct regardless of whether the block precedes or follows the conversion call.

- Local variable ('port'): I am entirely mindful of the desire to keep code laconic, yet, high-quality future-proof code benefits from such local declarations as they make the actual usage sites easier on the eyes and can be re-used by later additions to the logic.

On these premises, I respectfully suggest that the series be put forward for reconsideration and integration.

Thank you.

On Wed, 12 Aug 2026, Stephen Hemminger wrote:

> On Wed, 12 Aug 2026 21:10:17 +0400
> Ivan Malov <ivan.malov@arknetworks.am> wrote:
>
>> The issue is that when the user disables auto-negotiation by removing
>> the capability bit from the 'advertised mask' (set method) and then
>> reads the resulting capabilities, which involves querying MCDI, the
>> bit reappears in the mask irrespective of the user's intent.
>>
>> Fix this by remembering the user's intent before any link-state queries.
>>
>> Fixes: 2a5cf77e6de8 ("common/sfc_efx/base: provide PHY link get method on Medford4")
>> Fixes: 06f569de6c06 ("common/sfc_efx/base: decode netport link state on probe path")
>> Cc: stable@dpdk.org
>>
>> Suggested-by: Andy Moreton <andy.moreton@amd.com>
>> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
>> Reviewed-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
>> Reviewed-by: Andy Moreton <andy.moreton@amd.com>
>
> This patch depends on the previous common series and therefore can
> not be easily backported to stable.
>
> There is a way to mark patch dependencies using Depends-on:
> please use that instead of instructions to AI.
>
> AI says that in its usual over the top wordy way...
>
> Patch 3/3 - common/sfc_efx/base: fix reading advertised autoneg ability
>
>  Warning: this patch depends on the pending common/sfc series for its
>  diff context only, and that dependency creates a backport conflict.
>
>  The efx_np.c hunk removes
>
>      if (lsp->enls_an_supported != B_FALSE)
>              lsp->enls_adv_cap_mask |= 1U << EFX_PHY_CAP_AN;
>
>  from a position after the LINK_STATE_OUT_ADVERTISED_ABILITIES
>  conversion.  On main and on every stable branch the block is still
>  before that conversion, where 06f569de6c06 originally put it.  Since
>  efx_np_cap_mask_hw_to_sw() ORs into *sw_cap_maskp (efx_np.c:197)
>  rather than assigning, the block's position has no effect on the
>  resulting mask - the pending series is only moving text.
>
>  Both Fixes: commits first appear in v25.07, so this needs to reach
>  25.11 LTS and 25.07, neither of which has the move.  Stable will hit
>  a conflict on a hunk whose resolution is not obvious from the diff.
>
>  Suggest sending this fix ahead of the series that moves the block,
>  or as a standalone patch against main.  It then applies unchanged
>  everywhere, and the other series absorbs a zero-cost rebase.  If the
>  current ordering has to stand, please post an explicit backport to
>  stable@dpdk.org rather than leaving the resolution to the stable
>  maintainer.
>
>  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:
>
>      preserve_an = enp->en_port.ep_adv_cap_mask &
>                    (1U << EFX_PHY_CAP_AN);
>