Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] ice: premature AFTER_TC_CHANGE and unlock before ice_dcb_noncontig_cfg() - [High] ice: inconsistent QoS and DCB state on error path in ice_pf_dcb_cfg() - [Medium] ice: unrolled state mutations when ice_pf_dcb_cfg() fails with -EBUSY --
commit 0b49c36f284346b01578d0498826a110d7c3a315 Author: Petr Oros <[email protected]> ice: avoid RTNL deadlock when notifying the RDMA auxiliary driver This commit avoids an ABBA deadlock between rtnl_lock and the RDMA auxiliary device lock by acquiring the auxiliary device lock with device_trylock() via ice_aux_dev_trylock() around RDMA event delivery. In ice_pf_dcb_cfg(), the locks are held across the DCB reconfiguration from IIDC_RDMA_EVENT_BEFORE_TC_CHANGE through IIDC_RDMA_EVENT_AFTER_TC_CHANGE, and the non-contiguous traffic class fallback is deferred until after the locks are released. > diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > index 0bc6dd3756879..9916bd408071c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > @@ -5,6 +5,9 @@ > #include "ice_dcb_nl.h" > #include "devlink/devlink.h" > > +static bool ice_dcb_tc_contig(u8 *prio_table); > +static int ice_dcb_noncontig_cfg(struct ice_pf *pf); > + > /** > * ice_dcb_get_ena_tc - return bitmap of enabled TCs > * @dcbcfg: DCB config to evaluate for enabled TCs > @@ -352,11 +355,16 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct > ice_dcbx_cfg *new_cfg, bool locked) > struct ice_aqc_port_ets_elem buf = { 0 }; > struct ice_dcbx_cfg *old_cfg, *curr_cfg; > struct device *dev = ice_pf_to_dev(pf); > + struct iidc_rdma_priv_dev_info *privd; > + struct iidc_rdma_core_dev_info *cdev; > struct iidc_rdma_event *event; > int ret = ICE_DCB_NO_HW_CHG; > struct ice_vsi *pf_vsi; > + bool noncontig = false; > + bool dcb_ena; > > curr_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg; > + dcb_ena = test_bit(ICE_FLAG_DCB_ENA, pf->flags); [ ... ] > @@ -405,16 +413,21 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct > ice_dcbx_cfg *new_cfg, bool locked) > goto free_cfg; > } > > - set_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type); > - ice_send_event_to_aux(pf, event); > - kfree(event); > - > /* avoid race conditions by holding the lock while disabling and > * re-enabling the VSI > */ > if (!locked) > rtnl_lock(); > > + if (!ice_aux_dev_trylock(pf)) { > + assign_bit(ICE_FLAG_DCB_ENA, pf->flags, dcb_ena); > + ret = -EBUSY; > + goto unlock_rtnl; > + } [Severity: Medium] When ice_aux_dev_trylock(pf) fails here and returns -EBUSY, what happens to the state mutations that already took place earlier in ice_pf_dcb_cfg() and its callers? Earlier in ice_pf_dcb_cfg(), when ice_dcb_get_num_tc(new_cfg) > 1, ice_tear_down_devlink_rate_tree(pf) has already run before ice_aux_dev_trylock(pf): if (ice_dcb_get_num_tc(new_cfg) > 1) { if (pf->hw.port_info->is_custom_tx_enabled) { ... return -EBUSY; } ice_tear_down_devlink_rate_tree(pf); set_bit(ICE_FLAG_DCB_ENA, pf->flags); } Unlike the is_custom_tx_enabled check, failing at ice_aux_dev_trylock(pf) reverts ICE_FLAG_DCB_ENA while leaving the devlink rate tree torn down. In addition, callers that configure 1 TC (where is_custom_tx_enabled could never trigger), such as ice_dcb_sw_dflt_cfg() and ice_dcb_init_cfg(), zero pi->qos_cfg.local_dcbx_cfg with memset() before calling ice_pf_dcb_cfg(): memset(&pi->qos_cfg.local_dcbx_cfg, 0, sizeof(*dcbcfg)); ... ret = ice_pf_dcb_cfg(pf, dcbcfg, locked); If ice_aux_dev_trylock(pf) returns -EBUSY before curr_cfg is populated, does local_dcbx_cfg remain completely zeroed out? Similarly, in ice_dcbnl_setapp(), a negative return from ice_pf_dcb_cfg() is converted to ICE_DCB_NO_HW_CHG without rolling back dcb_ieee_setapp(), dscp_mapped, desired_dcbx_cfg, or ice_aq_set_pfc_mode(). > + > + set_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type); > + ice_send_event_to_aux(pf, event); > + > /* disable VSIs affected by DCB changes */ > ice_dcb_ena_dis_vsi(pf, false, true); > > @@ -441,13 +454,28 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct > ice_dcbx_cfg *new_cfg, bool locked) > goto out; > } > > - ice_pf_dcb_recfg(pf, false); > + ice_pf_dcb_recfg(pf, true); > + noncontig = !ice_dcb_tc_contig(curr_cfg->etscfg.prio_table); > > out: > + cdev = pf->cdev_info; > + if (cdev) { > + privd = cdev->iidc_priv; > + ice_setup_dcb_qos_info(pf, &privd->qos_info); > + } > + clear_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type); > + set_bit(IIDC_RDMA_EVENT_AFTER_TC_CHANGE, event->type); > + ice_send_event_to_aux(pf, event); [Severity: High] Can jumping to out on error leave curr_cfg, ICE_FLAG_DCB_ENA, and privd->qos_info inconsistent with the actual VSI configuration and cause active RDMA queue pairs to transition to the error state? In ice_pf_dcb_cfg(), if ice_set_dcb_cfg() succeeds (updating firmware and PRTDCB_TUP2TC) but ice_query_port_ets() fails and jumps to out: 1. curr_cfg still contains new_cfg (unlike the ice_set_dcb_cfg() failure path, which restores old_cfg), and neither error path restores ICE_FLAG_DCB_ENA to dcb_ena. 2. Because ice_pf_dcb_recfg(pf, true) was skipped, vsi->tc_cfg.ena_tc still only has the old traffic classes enabled. 3. At out, ice_setup_dcb_qos_info(pf, &privd->qos_info) populates privd->qos_info from the new curr_cfg and PRTDCB_TUP2TC register, and ice_send_event_to_aux() delivers IIDC_RDMA_EVENT_AFTER_TC_CHANGE. When irdma handles IIDC_RDMA_EVENT_AFTER_TC_CHANGE via icrdma_iidc_event_handler() -> irdma_change_l2params() -> irdma_sc_suspend_resume_qps(), it attempts to resume QPs on the new TCs in qos_info->up2tc[] via irdma_ws_add() -> icrdma_lan_register_qset() -> ice_add_rdma_qset(). Since vsi->tc_cfg.ena_tc was never updated for those new TCs, ice_ena_vsi_rdma_qset() fails with -EINVAL, causing irdma_sc_suspend_resume_qps() to call irdma_modify_qp_to_err() on the QPs. > + ice_aux_dev_unlock(pf); > + > /* enable previously downed VSIs */ > ice_dcb_ena_dis_vsi(pf, true, true); > + if (noncontig) > + ice_dcb_noncontig_cfg(pf); [Severity: High] When a DCB configuration has non-contiguous traffic classes in curr_cfg->etscfg.prio_table, does deferring ice_dcb_noncontig_cfg(pf) until after IIDC_RDMA_EVENT_AFTER_TC_CHANGE and ice_aux_dev_unlock(pf) break RDMA QP resume? Previously, ice_pf_dcb_cfg() called ice_pf_dcb_recfg(pf, false), which saw !ice_dcb_tc_contig(dcbcfg->etscfg.prio_table) and immediately ran ice_dcb_noncontig_cfg(pf) to reset local_dcbx_cfg and hardware PRTDCB_TUP2TC back to the default TC0 configuration before populating privd->qos_info, sending IIDC_RDMA_EVENT_AFTER_TC_CHANGE, or re-enabling the VSIs. With this change: 1. ice_set_dcb_cfg() programs the non-contiguous prio_table into hardware PRTDCB_TUP2TC. 2. ice_pf_dcb_recfg(pf, true) configures the PF VSI with tc_map = ICE_DFLT_TRAFFIC_CLASS (only TC0 enabled in vsi->tc_cfg.ena_tc) and skips ice_dcb_noncontig_cfg(pf) because locked is true. 3. Before ice_dcb_noncontig_cfg(pf) runs, ice_setup_dcb_qos_info() reads PRTDCB_TUP2TC from hardware: dcbx_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg; up2tc = rd32(&pf->hw, PRTDCB_TUP2TC); qos_info->num_tc = ice_dcb_get_num_tc(dcbx_cfg); for (i = 0; i < IIDC_MAX_USER_PRIORITY; i++) qos_info->up2tc[i] = (up2tc >> (i * 3)) & 0x7; This populates qos_info->up2tc[] with the un-reset non-contiguous TCs (> 0) while vsi->tc_cfg.ena_tc only has BIT(0) set. 4. ice_send_event_to_aux() then sends IIDC_RDMA_EVENT_AFTER_TC_CHANGE. When irdma resumes QPs via irdma_sc_suspend_resume_qps(), irdma_ws_add() -> ice_add_rdma_qset() -> ice_ena_vsi_rdma_qset() fails with -EINVAL for any TC > 0, and irdma_modify_qp_to_err() transitions the QPs to the error state. 5. Finally, ice_aux_dev_unlock(pf) drops the locks and ice_dcb_ena_dis_vsi(pf, true, true) re-enables the VSIs before ice_dcb_noncontig_cfg(pf) resets the configuration, and if the second ice_aux_dev_trylock(pf) inside ice_dcb_noncontig_cfg() -> ice_dcb_sw_dflt_cfg() -> ice_pf_dcb_cfg() fails with -EBUSY, the fallback aborts with local_dcbx_cfg zeroed out. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
