Re: [PATCH iwl-net 0/6] ice: fix DCB lock ordering

Re: [PATCH iwl-net 0/6] ice: fix DCB lock ordering

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

[PATCH iwl-net 0/6] ice: fix DCB lock ordering

[Przemek Kitszel]

Fix deadlocks and lock inversions around ice DCB configuration, between
RTNL, pf->tc_mutex, the PF devlink lock and pf->vfs.table_lock.

RTNL becomes the outer lock of all DCB configuration paths. dcbnl ops
and ndo_setup_tc() already run under it; patch 2 makes the LLDP MIB
change handler and ice_dcb_rebuild() take it too, so patch 5 can drop
the now redundant pf->tc_mutex. Patch 1 goes first, so that a dcbnl op
does not wait out a reset, whose rebuild now needs RTNL.

Enabling DCB in switchdev tears down the devlink-rate tree, which needs
the PF devlink lock and pf->vfs.table_lock, both taken before RTNL
elsewhere. Patch 4 only tries them, and only when a tree is exported,
which it tells by rate_node pointers, no longer left dangling by patch 3.

Patch 6 is an unrelated fix of the ice_init_lag() error path.

Reproducers are in the commit messages, each verified to trigger the
lockdep or KASAN report before its fix and not after it.

Petr's "ice: fix RTNL vs auxiliary device lock inversion" [0] touches
the same ice_setup_tc() and ice_pf_dcb_cfg() code, so one of the series
needs a rebase. With this one applied first, his patch 2 is not needed.
[0] https://lore.kernel.org/intel-wired-lan/20261023070258.730168-1-poros@redhat.com



Przemek Kitszel (6):
  ice: don't wait out a reset in ice_dcbnl_devreset()
  ice: take rtnl before tc_mutex in DCB MIB change and rebuild
  ice: clear stale rate_node pointers in
    ice_tear_down_devlink_rate_tree()
  ice: don't block on locks in ice_tear_down_devlink_rate_tree()
  ice: replace tc_mutex with RTNL
  ice: register LAG event handler last in ice_init_lag()

 .../net/ethernet/intel/ice/devlink/devlink.h  |  2 +-
 drivers/net/ethernet/intel/ice/ice.h          |  1 -
 drivers/net/ethernet/intel/ice/ice_dcb_lib.h  |  1 +
 .../net/ethernet/intel/ice/devlink/devlink.c  | 52 +++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c  | 46 ++++++++++------
 drivers/net/ethernet/intel/ice/ice_dcb_nl.c   | 35 ++++++-------
 drivers/net/ethernet/intel/ice/ice_lag.c      | 15 +++---
 drivers/net/ethernet/intel/ice/ice_main.c     |  8 ++-
 8 files changed, 108 insertions(+), 52 deletions(-)

-- 
2.54.0

[PATCH iwl-net 1/6] ice: don't wait out a reset in ice_dcbnl_devreset()

[Przemek Kitszel]

ice_dcbnl_devreset() spins on ice_is_reset_in_progress() while holding RTNL,
taken by dcb_doit(), and pf->tc_mutex, taken by its dcbnl caller. The reset
rebuild needs RTNL in ice_ena_vsi() and pf->tc_mutex in ice_dcb_rebuild(),
both called from ice_rebuild(). So the waiter keeps the reset from ever
finishing, and thus keeps waiting.

Nothing is gained by the wait anyway: a reset in flight normally rebuilds
the DCB config and reopens the netdev on its own, which is all the
ifdown/ifup here is for. Bail out instead.

The reset bits are cleared only after ice_rebuild() returns, so a call can
still see them set after the rebuild has finished, and skip the ifdown/ifup.
That is harmless: ice_pf_dcb_cfg() has already reconfigured the VSIs, only
the stack-level notifications of the bounce are lost.

Fixes: b94b013eb626 ("ice: Implement DCBNL support")
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_dcb_nl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
index bb5cc0a2ba69..9cdb9581fdc3 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
@@ -15,8 +15,9 @@ static void ice_dcbnl_devreset(struct net_device *netdev)
 {
 	struct ice_pf *pf = ice_netdev_to_pf(netdev);
 
-	while (ice_is_reset_in_progress(pf->state))
-		usleep_range(1000, 2000);
+	/* waiting for a reset in flight would block it on the locks we hold */
+	if (ice_is_reset_in_progress(pf->state))
+		return;
 
 	dev_close(netdev);
 	netdev_state_change(netdev);
-- 
2.54.0

[PATCH iwl-net 2/6] ice: take rtnl before tc_mutex in DCB MIB change and rebuild

[Przemek Kitszel]

ice_setup_tc() and dcbnl ops are called with rtnl held and then take
pf->tc_mutex. LLDP MIB change handler and ice_dcb_rebuild() (the latter
only on its error path, via ice_pf_dcb_cfg(..., false)) do the opposite:
take pf->tc_mutex and then rtnl. The MIB change handler runs from the
service task. ice_dcb_rebuild() runs from ice_rebuild() (reset, also PCI
AER resume), and from ice_link_event() in the service task, on a link
change while DCB is enabled. So a MIB change from the link partner, a reset
or a link change, concurrent with "tc qdisc add ... mqprio" or "dcb ets
set" may deadlock.

Take rtnl first in both functions. ice_dcb_rebuild() now holds rtnl also
on its success path, which only sends a few AQ commands. None of its
callers hold rtnl.

Lockdep splat, on the first MIB change that needs a DCB reconfiguration
after "dcb ets set" was used:

        CPU0                    CPU1
        ----                    ----
   lock(&pf->tc_mutex);
                                lock(rtnl_mutex);
                                lock(&pf->tc_mutex);
   lock(rtnl_mutex);

Reproducer, with the FW LLDP agent in its default (willing) mode:
  PF=eth2
  ethtool --set-priv-flags $PF fw-lldp-agent off
  dcb ets set dev $PF tc-tsa all:ets tc-bw all:0 0:100 prio-tc all:0
  ethtool --set-priv-flags $PF fw-lldp-agent on
and then, on the link partner, send an ETS recommendation with a different
UP2TC mapping (python3 lldp-ets.py <partner-ifname>):
  import socket, struct, sys
  ifc = sys.argv[1]
  mac = open(f"/sys/class/net/{ifc}/address").read().strip()
  mac = bytes.fromhex(mac.replace(":", ""))
  tlv = lambda t, v: struct.pack("!H", t << 9 | len(v)) + v
  # not willing, prio 4 -> TC1, TC0/TC1 ETS 50/50
  ets = bytes([0, 0, 0, 0x10, 0, 50, 50] + [0] * 6 + [2, 2] + [0] * 6)
  lldp = (tlv(1, b"\x04" + mac) + tlv(2, b"\x03" + mac) +
          tlv(3, b"\x00\x78") +
          tlv(127, b"\x00\x80\xc2\x09" + ets) +  # ETS Configuration
          tlv(127, b"\x00\x80\xc2\x0a" + ets) +  # ETS Recommendation
          tlv(0, b""))
  s = socket.socket(socket.AF_PACKET, socket.SOCK_RAW)
  s.bind((ifc, 0))
  s.send(bytes.fromhex("0180c200000e") + mac + b"\x88\xcc" + lldp)

Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index 0bc6dd375687..3034f461f513 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -537,6 +537,7 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 	struct ice_dcbx_cfg *err_cfg;
 	int ret;
 
+	rtnl_lock();
 	mutex_lock(&pf->tc_mutex);
 
 	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
@@ -570,14 +571,16 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 	}
 
 	mutex_unlock(&pf->tc_mutex);
+	rtnl_unlock();
 
 	return;
 
 dcb_error:
 	dev_err(dev, "Disabling DCB until new settings occur\n");
 	err_cfg = kzalloc_obj(*err_cfg);
 	if (!err_cfg) {
 		mutex_unlock(&pf->tc_mutex);
+		rtnl_unlock();
 		return;
 	}
 
@@ -591,10 +594,11 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 	 * Suppress the Coverity warning with the following comment...
 	 */
 	/* coverity[check_return] */
-	ice_pf_dcb_cfg(pf, err_cfg, false);
+	ice_pf_dcb_cfg(pf, err_cfg, true);
 	kfree(err_cfg);
 
 	mutex_unlock(&pf->tc_mutex);
+	rtnl_unlock();
 }
 
 /**
@@ -1055,6 +1059,7 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf,
 	}
 
 	/* That a DCB change has happened is now determined */
+	rtnl_lock();
 	mutex_lock(&pf->tc_mutex);
 
 	/* store the old configuration */
@@ -1105,25 +1110,23 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf,
 		pending_handled = true;
 	}
 
-	rtnl_lock();
 	/* disable VSIs affected by DCB changes */
 	ice_dcb_ena_dis_vsi(pf, false, true);
 
 	ret = ice_query_port_ets(pi, &buf, sizeof(buf), NULL);
 	if (ret) {
 		dev_err(dev, "Query Port ETS failed\n");
-		goto unlock_rtnl;
+		goto out;
 	}
 
 	/* changes in configuration update VSI */
 	ice_pf_dcb_recfg(pf, false);
 
 	/* enable previously downed VSIs */
 	ice_dcb_ena_dis_vsi(pf, true, true);
-unlock_rtnl:
-	rtnl_unlock();
 out:
 	mutex_unlock(&pf->tc_mutex);
+	rtnl_unlock();
 
 	/* Send Execute Pending MIB Change event if it is a Pending event */
 	if (!pending_handled)
-- 
2.54.0

[PATCH iwl-net 3/6] ice: clear stale rate_node pointers in ice_tear_down_devlink_rate_tree()

[Przemek Kitszel]

ice_tear_down_devlink_rate_tree() frees the devlink rate nodes with
devl_rate_nodes_destroy(), but leaves ice_sched_node::rate_node pointing at
them. The pointers are only reset when the last representor goes away, in
ice_eswitch_detach().

If DCB is turned on and off again while representors exist, the next
ice_devlink_rate_init_tx_topology() (e.g. on adding an SF port) sees the
stale pointers as already exported nodes and passes one of them as the
parent to devl_rate_leaf_create(), which writes to freed memory.

Clear the pointers before destroying the nodes, as ice_eswitch_detach() does.

dcbnl ops can call ice_tear_down_devlink_rate_tree() while a reset is in
flight, or after a failed one, when the Tx scheduler tree is gone. Make
ice_devlink_rate_clear_tx_topology() skip the clearing then, as other Tx
scheduler helpers do when the port is not ready.

Reproducer:
  PF=eth3
  BDF=$(basename $(readlink /sys/class/net/$PF/device))
  ethtool --set-priv-flags $PF fw-lldp-agent off
  devlink dev eswitch set pci/$BDF mode switchdev
  echo 1 > /sys/class/net/$PF/device/sriov_numvfs
  dcb ets set dev $PF tc-tsa all:ets tc-bw all:0 0:50 1:50 prio-tc all:0 4:1
  dcb ets set dev $PF tc-tsa all:ets tc-bw all:0 0:100 prio-tc all:0
  devlink port add pci/$BDF flavour pcisf pfnum 0 sfnum 1

  BUG: KASAN: slab-use-after-free in devl_rate_leaf_create+0x109/0x390

Fixes: 86197ad5800b ("ice: set Tx topology every time new repr is added")
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/devlink/devlink.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c
index 8c2b63eef82b..044aa62be473 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
@@ -705,6 +705,7 @@ void ice_tear_down_devlink_rate_tree(struct ice_pf *pf)
 	}
 	mutex_unlock(&pf->vfs.table_lock);
 
+	ice_devlink_rate_clear_tx_topology(ice_get_main_vsi(pf));
 	devl_rate_nodes_destroy(devlink);
 	devl_unlock(devlink);
 }
@@ -842,6 +843,10 @@ void ice_devlink_rate_clear_tx_topology(struct ice_vsi *vsi)
 {
 	struct ice_port_info *pi = vsi->port_info;
 
+	/* the Tx scheduler tree is gone during a reset */
+	if (pi->port_state != ICE_SCHED_PORT_STATE_READY)
+		return;
+
 	mutex_lock(&pi->sched_lock);
 	ice_clear_rate_nodes(pi->root->children[0]);
 	mutex_unlock(&pi->sched_lock);
-- 
2.54.0

[PATCH iwl-net 4/6] ice: don't block on locks in ice_tear_down_devlink_rate_tree()

[Przemek Kitszel]

ice_tear_down_devlink_rate_tree() is called with RTNL held (dcbnl, mqprio),
and then blocks on the PF devlink lock and pf->vfs.table_lock, both of which
are taken the other way around elsewhere:
 - ice_start_vfs() and ice_free_vfs() hold table_lock and call
   ice_eswitch_{attach,detach}_vf(), which take the PF devlink lock and
   register/unregister representor netdevs (so RTNL).
 - ice_probe() holds the devlink lock across ice_load(), which registers
   the netdev (so RTNL).

Lockdep reports a possible ABBA deadlock:

        CPU0                    CPU1
        ----                    ----
   lock(&pf->vfs.table_lock);
                                lock(&devlink->lock_key);
                                lock(&pf->vfs.table_lock);
   lock(&devlink->lock_key);

Swapping the two locks would only fix this particular pair, as the RTNL
inversion would remain. Instead, only try both locks and fail with -EAGAIN
when either is contended, unlike the -EBUSY of an enabled custom Tx
scheduler.

Return early, without taking any lock, when no tree is exported: always
when switchdev is not running, and also after DCB got enabled, as the tree
is not exported again then. The latter is checked by rate_node pointers of
the Tx scheduler nodes, under the scheduler lock, unless the scheduler is
not ready (during a reset). This way DCB and mqprio configuration fails on
contention only when there is a tree to tear down. It also covers
ice_pf_dcb_cfg() with locked == false, called from probe or devlink reload,
where the devlink lock is already held and RTNL is not.

Propagate the error to the callers that can report it: dcbnl ops and
mqprio. For that, stop ice_dcbnl_setapp() from folding all errors into
ICE_DCB_NO_HW_CHG. ice_dcbnl_setapp() updates the app list, and may switch
the FW to DSCP mode, before it calls ice_pf_dcb_cfg(). To fail before any
change in the usual case, move the custom Tx scheduler check and the
teardown into a helper, and call it from ice_dcbnl_setapp() first.
ice_dcbnl_delapp() does not add TCs, so it has no tree to tear down.

ice_dcbnl_cee_set_all() returns a u8 status, not an errno, so it can't
report -EAGAIN, but the desired config is kept and a retry applies it.
DCB init, called from ethtool priv flags, already ignores ice_pf_dcb_cfg()
failures.

Found during development, reproducer:
  PF=ens801f7np7
  BDF=$(basename $(readlink /sys/class/net/$PF/device))
  ethtool --set-priv-flags $PF fw-lldp-agent off
  devlink dev eswitch set pci/$BDF mode switchdev
  echo 2 > /sys/class/net/$PF/device/sriov_numvfs
  dcb ets set dev $PF tc-tsa all:ets tc-bw all:0 0:50 1:50 prio-tc all:0 4:1

Fixes: fff292b47ac1 ("ice: add VF representors one by one")
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 .../net/ethernet/intel/ice/devlink/devlink.h  |  2 +-
 drivers/net/ethernet/intel/ice/ice_dcb_lib.h  |  1 +
 .../net/ethernet/intel/ice/devlink/devlink.c  | 47 +++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c  | 27 +++++++++--
 drivers/net/ethernet/intel/ice/ice_dcb_nl.c   | 15 +++++-
 drivers/net/ethernet/intel/ice/ice_main.c     |  4 +-
 6 files changed, 83 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.h b/drivers/net/ethernet/intel/ice/devlink/devlink.h
index 1af3b0763fbb..4139171ca57c 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink.h
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.h
@@ -20,7 +20,7 @@ void ice_devlink_init_regions(struct ice_pf *pf);
 void ice_devlink_destroy_regions(struct ice_pf *pf);
 
 int ice_devlink_rate_init_tx_topology(struct devlink *devlink, struct ice_vsi *vsi);
-void ice_tear_down_devlink_rate_tree(struct ice_pf *pf);
+int ice_tear_down_devlink_rate_tree(struct ice_pf *pf);
 void ice_devlink_rate_clear_tx_topology(struct ice_vsi *vsi);
 
 #endif /* _ICE_DEVLINK_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.h b/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
index da9ba814b4e8..dd00cb459212 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.h
@@ -20,6 +20,7 @@ u8 ice_dcb_get_num_tc(struct ice_dcbx_cfg *dcbcfg);
 void ice_vsi_set_dcb_tc_cfg(struct ice_vsi *vsi);
 bool ice_is_pfc_causing_hung_q(struct ice_pf *pf, unsigned int txqueue);
 u8 ice_dcb_get_tc(struct ice_vsi *vsi, int queue_index);
+int ice_dcb_prep_multi_tc(struct ice_pf *pf);
 int
 ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked);
 int ice_dcb_bwchk(struct ice_pf *pf, struct ice_dcbx_cfg *dcbcfg);
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c
index 044aa62be473..c94bc75e448e 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
@@ -683,31 +683,68 @@ static int ice_devlink_tx_sched_layers_validate(struct devlink *devlink, u32 id,
 	return 0;
 }
 
+static bool ice_devlink_rate_tree_is_exported(struct ice_port_info *pi)
+{
+	struct ice_sched_node *tc_node;
+	bool exported = false;
+
+	if (pi->port_state != ICE_SCHED_PORT_STATE_READY)
+		return false;
+
+	mutex_lock(&pi->sched_lock);
+	tc_node = pi->root->children[0];
+	for (int i = 0; i < tc_node->num_children; i++) {
+		if (tc_node->children[i]->rate_node) {
+			exported = true;
+			break;
+		}
+	}
+	mutex_unlock(&pi->sched_lock);
+
+	return exported;
+}
+
 /**
  * ice_tear_down_devlink_rate_tree - removes devlink-rate exported tree
  * @pf: pf struct
  *
  * This function tears down tree exported during VF's creation.
+ *
+ * Return: 0 on success or when no tree is exported, -EAGAIN if a lock could
+ * not be taken.
  */
-void ice_tear_down_devlink_rate_tree(struct ice_pf *pf)
+int ice_tear_down_devlink_rate_tree(struct ice_pf *pf)
 {
 	struct devlink *devlink;
 	struct ice_vf *vf;
 	unsigned int bkt;
 
+	/* The tree is exported only when switchdev is running */
+	if (!ice_is_switchdev_running(pf) ||
+	    !ice_devlink_rate_tree_is_exported(pf->hw.port_info))
+		return 0;
+
+	ASSERT_RTNL();
+
 	devlink = priv_to_devlink(pf);
 
-	devl_lock(devlink);
-	mutex_lock(&pf->vfs.table_lock);
+	/* Both locks are taken before RTNL elsewhere, so only try them here */
+	if (!mutex_trylock(&pf->vfs.table_lock))
+		return -EAGAIN;
+	if (!devl_trylock(devlink)) {
+		mutex_unlock(&pf->vfs.table_lock);
+		return -EAGAIN;
+	}
 	ice_for_each_vf(pf, bkt, vf) {
 		if (vf->devlink_port.devlink_rate)
 			devl_rate_leaf_destroy(&vf->devlink_port);
 	}
-	mutex_unlock(&pf->vfs.table_lock);
-
 	ice_devlink_rate_clear_tx_topology(ice_get_main_vsi(pf));
 	devl_rate_nodes_destroy(devlink);
 	devl_unlock(devlink);
+	mutex_unlock(&pf->vfs.table_lock);
+
+	return 0;
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index 3034f461f513..59fafd296f1c 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -341,6 +341,23 @@ int ice_dcb_bwchk(struct ice_pf *pf, struct ice_dcbx_cfg *dcbcfg)
 	return 0;
 }
 
+/**
+ * ice_dcb_prep_multi_tc - prepare the Tx scheduler for more than one TC
+ * @pf: pointer to the PF struct
+ *
+ * Return: 0 on success, -EBUSY if the Tx scheduler is customized via
+ * devlink-rate, -EAGAIN if its devlink-rate export could not be torn down.
+ */
+int ice_dcb_prep_multi_tc(struct ice_pf *pf)
+{
+	if (pf->hw.port_info->is_custom_tx_enabled) {
+		dev_err(ice_pf_to_dev(pf), "Custom Tx scheduler feature enabled, can't configure DCB\n");
+		return -EBUSY;
+	}
+
+	return ice_tear_down_devlink_rate_tree(pf);
+}
+
 /**
  * ice_pf_dcb_cfg - Apply new DCB configuration
  * @pf: pointer to the PF struct
@@ -364,12 +381,12 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
 
 	/* Enable DCB tagging only when more than one TC */
 	if (ice_dcb_get_num_tc(new_cfg) > 1) {
+		int err;
+
 		dev_dbg(dev, "DCB tagging enabled (num TC > 1)\n");
-		if (pf->hw.port_info->is_custom_tx_enabled) {
-			dev_err(dev, "Custom Tx scheduler feature enabled, can't configure DCB\n");
-			return -EBUSY;
-		}
-		ice_tear_down_devlink_rate_tree(pf);
+		err = ice_dcb_prep_multi_tc(pf);
+		if (err)
+			return err;
 
 		set_bit(ICE_FLAG_DCB_ENA, pf->flags);
 	} else {
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
index 9cdb9581fdc3..2b9f5e6c1bbe 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
@@ -736,6 +736,7 @@ static int ice_dcbnl_setapp(struct net_device *netdev, struct dcb_app *app)
 	struct ice_pf *pf = ice_netdev_to_pf(netdev);
 	struct ice_dcb_app_priority_table new_app;
 	struct ice_dcbx_cfg *old_cfg, *new_cfg;
+	int tc_to_prep;
 	u8 max_tc;
 	int ret;
 
@@ -779,6 +780,18 @@ static int ice_dcbnl_setapp(struct net_device *netdev, struct dcb_app *app)
 	new_cfg = &pf->hw.port_info->qos_cfg.desired_dcbx_cfg;
 	old_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg;
 
+	/* fail before any change; switching to DSCP mode enables max_tc TCs */
+	if (old_cfg->pfc_mode == ICE_QOS_MODE_VLAN)
+		tc_to_prep = max_tc;
+	else
+		tc_to_prep = ice_dcb_get_num_tc(new_cfg);
+
+	if (tc_to_prep > 1) {
+		ret = ice_dcb_prep_multi_tc(pf);
+		if (ret)
+			goto setapp_out;
+	}
+
 	ret = dcb_ieee_setapp(netdev, app);
 	if (ret)
 		goto setapp_out;
@@ -850,7 +863,7 @@ static int ice_dcbnl_setapp(struct net_device *netdev, struct dcb_app *app)
 	/* return of zero indicates new cfg applied */
 	if (ret == ICE_DCB_HW_CHG_RST)
 		ice_dcbnl_devreset(netdev);
-	else
+	else if (ret > 0)
 		ret = ICE_DCB_NO_HW_CHG;
 
 setapp_out:
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 454fba07d4ec..30de690ecf7c 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -9180,7 +9180,9 @@ static int ice_setup_tc_mqprio_qdisc(struct net_device *netdev, void *type_data)
 			dev_err(dev, "Custom Tx scheduler feature enabled, can't configure ADQ\n");
 			return -EBUSY;
 		}
-		ice_tear_down_devlink_rate_tree(pf);
+		ret = ice_tear_down_devlink_rate_tree(pf);
+		if (ret)
+			return ret;
 
 		ret = ice_validate_mqprio_qopt(vsi, mqprio_qopt);
 		if (ret) {
-- 
2.54.0

[PATCH iwl-net 5/6] ice: replace tc_mutex with RTNL

[Przemek Kitszel]

ice_setup_tc() takes pf->adev_mutex and the RDMA aux device lock, and then
pf->tc_mutex, for mqprio. dcbnl ops take pf->tc_mutex, and then
pf->adev_mutex in ice_send_event_to_aux(), when ice_pf_dcb_cfg() notifies
the RDMA driver about a TC change. Lockdep reports a possible deadlock:

        CPU0                    CPU1
        ----                    ----
   lock(&pf->adev_mutex);
                                lock(&pf->tc_mutex);
                                lock(&pf->adev_mutex);
   lock(&pf->tc_mutex);

All users of pf->tc_mutex (dcbnl ops, ndo_setup_tc() for mqprio,
ice_dcb_rebuild() and the LLDP MIB change handler) already run under
RTNL, the latter two since they take it first, earlier in this series.
So the inversion can't deadlock, but lockdep still reports it, and the
mutex is redundant. Drop it, instead of reordering it against
pf->adev_mutex.

Reproducer, with RDMA enabled on the PF, but irdma not loaded:
  PF=eth3
  ethtool --set-priv-flags $PF fw-lldp-agent off
  dcb ets set dev $PF tc-tsa all:ets tc-bw all:0 0:50 1:50 prio-tc all:0 4:1
  dcb ets set dev $PF tc-tsa all:ets tc-bw all:0 0:100 prio-tc all:0
  tc qdisc add dev $PF root mqprio num_tc 2 map 0 0 0 0 1 1 1 1 \
          queues 4@0 4@4 hw 1 mode channel

Fixes: 4b31fd4d77ff ("ice: Fix RDMA VSI removal during queue rebuild")
Reported-by: Petr Oros <poros@redhat.com>
Closes: https://lore.kernel.org/all/20261023070258.730168-3-poros@redhat.com/
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h         |  1 -
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c |  6 ------
 drivers/net/ethernet/intel/ice/ice_dcb_nl.c  | 15 ---------------
 drivers/net/ethernet/intel/ice/ice_main.c    |  4 ----
 4 files changed, 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 432da05c48c0..12907be77454 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -592,7 +592,6 @@ struct ice_pf {
 	struct work_struct serv_task;
 	struct mutex avail_q_mutex;	/* protects access to avail_[rx|tx]qs */
 	struct mutex sw_mutex;		/* lock for protecting VSI alloc flow */
-	struct mutex tc_mutex;		/* lock to protect TC changes */
 	struct mutex adev_mutex;	/* lock to protect aux device access */
 	struct mutex lag_mutex;		/* protect ice_lag struct in PF */
 	u32 msg_enable;
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index 59fafd296f1c..5355519726df 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -555,7 +555,6 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 	int ret;
 
 	rtnl_lock();
-	mutex_lock(&pf->tc_mutex);
 
 	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
 	if (ret) {
@@ -587,16 +586,14 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 		goto dcb_error;
 	}
 
-	mutex_unlock(&pf->tc_mutex);
 	rtnl_unlock();
 
 	return;
 
 dcb_error:
 	dev_err(dev, "Disabling DCB until new settings occur\n");
 	err_cfg = kzalloc_obj(*err_cfg);
 	if (!err_cfg) {
-		mutex_unlock(&pf->tc_mutex);
 		rtnl_unlock();
 		return;
 	}
@@ -614,7 +611,6 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 	ice_pf_dcb_cfg(pf, err_cfg, true);
 	kfree(err_cfg);
 
-	mutex_unlock(&pf->tc_mutex);
 	rtnl_unlock();
 }
 
@@ -1077,7 +1073,6 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf,
 
 	/* That a DCB change has happened is now determined */
 	rtnl_lock();
-	mutex_lock(&pf->tc_mutex);
 
 	/* store the old configuration */
 	tmp_dcbx_cfg = pi->qos_cfg.local_dcbx_cfg;
@@ -1142,7 +1137,6 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf,
 	/* enable previously downed VSIs */
 	ice_dcb_ena_dis_vsi(pf, true, true);
 out:
-	mutex_unlock(&pf->tc_mutex);
 	rtnl_unlock();
 
 	/* Send Execute Pending MIB Change event if it is a Pending event */
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
index 2b9f5e6c1bbe..bc2af975b1e3 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_nl.c
@@ -78,8 +78,6 @@ static int ice_dcbnl_setets(struct net_device *netdev, struct ieee_ets *ets)
 
 	new_cfg = &pf->hw.port_info->qos_cfg.desired_dcbx_cfg;
 
-	mutex_lock(&pf->tc_mutex);
-
 	new_cfg->etscfg.willing = ets->willing;
 	new_cfg->etscfg.cbs = ets->cbs;
 	ice_for_each_traffic_class(i) {
@@ -117,7 +115,6 @@ static int ice_dcbnl_setets(struct net_device *netdev, struct ieee_ets *ets)
 		err = ICE_DCB_HW_CHG_RST;
 
 ets_out:
-	mutex_unlock(&pf->tc_mutex);
 	return err;
 }
 
@@ -277,8 +274,6 @@ static int ice_dcbnl_setpfc(struct net_device *netdev, struct ieee_pfc *pfc)
 		return -EINVAL;
 	}
 
-	mutex_lock(&pf->tc_mutex);
-
 	new_cfg = &pf->hw.port_info->qos_cfg.desired_dcbx_cfg;
 
 	if (pfc->pfc_cap)
@@ -293,7 +288,6 @@ static int ice_dcbnl_setpfc(struct net_device *netdev, struct ieee_pfc *pfc)
 		ice_dcbnl_devreset(netdev);
 	if (err == ICE_DCB_NO_HW_CHG)
 		err = ICE_DCB_HW_CHG_RST;
-	mutex_unlock(&pf->tc_mutex);
 	return err;
 }
 
@@ -774,9 +768,6 @@ static int ice_dcbnl_setapp(struct net_device *netdev, struct dcb_app *app)
 		return -EINVAL;
 	}
 
-	/* grab TC mutex */
-	mutex_lock(&pf->tc_mutex);
-
 	new_cfg = &pf->hw.port_info->qos_cfg.desired_dcbx_cfg;
 	old_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg;
 
@@ -867,7 +858,6 @@ static int ice_dcbnl_setapp(struct net_device *netdev, struct dcb_app *app)
 		ret = ICE_DCB_NO_HW_CHG;
 
 setapp_out:
-	mutex_unlock(&pf->tc_mutex);
 	return ret;
 }
 
@@ -895,7 +885,6 @@ static int ice_dcbnl_delapp(struct net_device *netdev, struct dcb_app *app)
 		return -EINVAL;
 	}
 
-	mutex_lock(&pf->tc_mutex);
 	old_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg;
 
 	ret = dcb_ieee_delapp(netdev, app);
@@ -978,7 +967,6 @@ static int ice_dcbnl_delapp(struct net_device *netdev, struct dcb_app *app)
 		ret = ICE_DCB_HW_CHG;
 
 delapp_out:
-	mutex_unlock(&pf->tc_mutex);
 	return ret;
 }
 
@@ -1003,11 +991,8 @@ static u8 ice_dcbnl_cee_set_all(struct net_device *netdev)
 
 	new_cfg = &pf->hw.port_info->qos_cfg.desired_dcbx_cfg;
 
-	mutex_lock(&pf->tc_mutex);
-
 	err = ice_pf_dcb_cfg(pf, new_cfg, true);
 
-	mutex_unlock(&pf->tc_mutex);
 	return (err != ICE_DCB_HW_CHG_RST) ? ICE_DCB_NO_HW_CHG : err;
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 30de690ecf7c..6b6e5ae533ae 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3876,7 +3876,6 @@ void ice_deinit_pf(struct ice_pf *pf)
 	mutex_destroy(&pf->lag_mutex);
 	mutex_destroy(&pf->adev_mutex);
 	mutex_destroy(&pf->sw_mutex);
-	mutex_destroy(&pf->tc_mutex);
 	mutex_destroy(&pf->avail_q_mutex);
 	mutex_destroy(&pf->vfs.table_lock);
 
@@ -3975,7 +3974,6 @@ int ice_init_pf(struct ice_pf *pf)
 	int err = -ENOMEM;
 
 	mutex_init(&pf->sw_mutex);
-	mutex_init(&pf->tc_mutex);
 	mutex_init(&pf->adev_mutex);
 	mutex_init(&pf->lag_mutex);
 
@@ -9461,9 +9459,7 @@ ice_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 		}
 
 		/* setup traffic classifier for receive side */
-		mutex_lock(&pf->tc_mutex);
 		err = ice_setup_tc_mqprio_qdisc(netdev, type_data);
-		mutex_unlock(&pf->tc_mutex);
 
 adev_unlock:
 		if (locked) {
-- 
2.54.0

[PATCH iwl-net 6/6] ice: register LAG event handler last in ice_init_lag()

[Przemek Kitszel]

ice_init_lag() registers the netdevice notifier before it creates the LAG
recipes, but when creating any of them fails, the error path frees the lag
struct without unregistering the notifier. A failure of ice_init_lag() is
not fatal to probe, and pf->lag is set to NULL, so ice_deinit_lag() won't
unregister it later either. The next netdev event then makes
ice_lag_event_handler() use the freed lag struct.

Register the notifier last, after the recipes are created and associated
with the profiles, so the error paths have no handler, nor queued work for
it, that could use the lag struct or its recipes. Free the recipes if the
registration itself fails.

Fixes: ec5a6c5f79ed ("ice: process events created by lag netdev event handler")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-e0559bca-fcfd-4435-96f2-15f7e3d74184
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lag.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
index 08a17ded0ad5..c4b8af01e2af 100644
--- a/drivers/net/ethernet/intel/ice/ice_lag.c
+++ b/drivers/net/ethernet/intel/ice/ice_lag.c
@@ -2601,12 +2601,6 @@ int ice_init_lag(struct ice_pf *pf)
 	lag->notif_block.notifier_call = NULL;
 	memset(lag->sec_vf, 0, sizeof(lag->sec_vf));
 
-	err = ice_register_lag_handler(lag);
-	if (err) {
-		dev_warn(dev, "INIT LAG: Failed to register event handler\n");
-		goto lag_error;
-	}
-
 	err = ice_create_lag_recipe(&pf->hw, &lag->pf_recipe,
 				    ice_dflt_vsi_rcp, 1);
 	if (err)
@@ -2638,11 +2632,20 @@ int ice_init_lag(struct ice_pf *pf)
 		}
 	}
 
+	err = ice_register_lag_handler(lag);
+	if (err) {
+		dev_warn(dev, "INIT LAG: Failed to register event handler\n");
+		goto free_act_act_res;
+	}
+
 	ice_display_lag_info(lag);
 
 	dev_dbg(dev, "INIT LAG complete\n");
 	return 0;
 
+free_act_act_res:
+	ice_free_hw_res(&pf->hw, ICE_AQC_RES_TYPE_RECIPE, 1,
+			&lag->act_act_recipe);
 free_lport_res:
 	ice_free_hw_res(&pf->hw, ICE_AQC_RES_TYPE_RECIPE, 1,
 			&lag->lport_recipe);
-- 
2.54.0