On 9/5/2025 8:32 AM, Bruce Richardson wrote: > On Tue, Sep 02, 2025 at 06:26:59PM +0100, Anatoly Burakov wrote: >> From: Jacob Keller >> >> The ice_cfg_tx_topo function attempts to apply Tx scheduler topology >> configuration based on NVM parameters, selecting either a 5 or 9 layer >> topology. >> >> As part of this flow, the driver acquires the "Global Configuration >> Lock", which is a hardware resource associated with programming the DDP >> package to the device. This "lock" is implemented by firmware as a way to >> guarantee that only one PF can program the DDP for a device. Unlike a >> traditional lock, once a PF has acquired this lock, no other PF will be >> able to acquire it again (including that PF) until a core reset of the >> device. Future requests to acquire the lock report that global >> configuration has already completed. >> >> The following flow is used to program the Tx topology: >> >> * Read the DDP package for scheduler configuration data >> * Acquire the global configuration lock >> * Program Tx scheduler topology according to DDP package data >> * Trigger a core reset which clears the global configuration lock >> >> This is followed by the flow for programming the DDP package: >> >> * Acquire the global configuration lock (again) >> * Download the DDP package to the device >> * Release the global configuration lock. >> >> However, if configuration of the Tx topology fails, (i.e. >> ice_get_set_tx_topo() returns an error code), the driver exits >> ice_cfg_tx_topo() immediately, and fails to trigger core reset. >> >> While the global configuration lock is held, the firmware rejects most >> AdminQ commands, as it is waiting for the DDP package download (or Tx >> scheduler topology programming) to occur. >> >> The current driver flows assume that the global configuration lock has >> been reset after programming the Tx topology. Thus, the same PF attempts >> to acquire the global lock again, and fails. This results in the driver >> reporting "an unknown error occurred when loading the DDP package". It >> then attempts to enter safe mode, but ultimately fails to finish >> ice_probe() since nearly all AdminQ command report error codes, and the >> driver stops loading the device at some point during its initialization. >> >> We cannot simply release the global lock after a failed call to >> ice_get_set_tx_topo(). Releasing the lock indicates to firmware that >> global configuration (downloading of the DDP) has completed. Future >> attempts by this or other PFs to load the DDP will fail with a report >> that the DDP package has already been downloaded. Then, PFs will enter >> safe mode as they realize that the package on the device does not meet >> the minimum version requirement to load. The reported error messages are >> confusing, as they indicate the version of the default "safe mode" >> package in the NVM, rather than the version of the DDP package loaded >> from the filesystem. >> >> Instead, we need to trigger core reset to clear global configuration. >> This is the lowest level of hardware reset which clears the global >> configuration lock and related state. It also clears any already >> downloaded DDP. Crucially, it does *not* clear the Tx scheduler topology >> configuration. >> >> Refactor ice_cfg_tx_topo() to always trigger a core reset after acquiring >> the global lock, regardless of success or failure of the topology >> configuration. >> >> We need to re-initialize the HW structure when we trigger the core reset. >> Previously, this was the responsibility of the core driver to cleanup >> after the core reset. Instead, make it the responsibility of this >> function. This avoids needless re-initialization for the cases where no >> reset occurred. >> >> Signed-off-by: Jacob Keller >> Signed-off-by: Anatoly Burakov >> --- >> drivers/net/intel/ice/base/ice_ddp.c | 34 ++++++++++++++++++---------- >> 1 file changed, 22 insertions(+), 12 deletions(-) >> > > Acked-by: Bruce Richardson > > See one comment inline below. > > >> diff --git a/drivers/net/intel/ice/base/ice_ddp.c b/drivers/net/intel/ice/base/ice_ddp.c >> index 850c722a3f..68e75be4d2 100644 >> --- a/drivers/net/intel/ice/base/ice_ddp.c >> +++ b/drivers/net/intel/ice/base/ice_ddp.c >> @@ -2370,7 +2370,7 @@ int ice_cfg_tx_topo(struct ice_hw *hw, u8 *buf, u32 len) >> struct ice_buf_hdr *section; >> struct ice_pkg_hdr *pkg_hdr; >> enum ice_ddp_state state; >> - u16 i, size = 0, offset; >> + u16 size = 0, offset; >> u32 reg = 0; >> int status; >> u8 flags; >> @@ -2457,25 +2457,35 @@ int ice_cfg_tx_topo(struct ice_hw *hw, u8 *buf, u32 len) >> /* check reset was triggered already or not */ >> reg = rd32(hw, GLGEN_RSTAT); >> if (reg & GLGEN_RSTAT_DEVSTATE_M) { >> - /* Reset is in progress, re-init the hw again */ >> ice_debug(hw, ICE_DBG_INIT, "Reset is in progress. layer topology might be applied already\n"); >> ice_check_reset(hw); >> - return 0; >> + /* Reset is in progress, re-init the hw again */ >> + goto reinit_hw; >> } >> >> /* set new topology */ >> status = ice_get_set_tx_topo(hw, new_topo, size, NULL, NULL, true); >> if (status) { >> - ice_debug(hw, ICE_DBG_INIT, "Set tx topology is failed\n"); >> - return status; >> + ice_debug(hw, ICE_DBG_INIT, "Failed setting Tx topology, status %d\n", >> + status); >> + status = ICE_ERR_CFG; >> } >> >> - /* new topology is updated, delay 1 second before issuing the CORRER */ >> - for (i = 0; i < 10; i++) >> - ice_msec_delay(100, true); >> + /* Even if Tx topology config failed, we need to CORE reset here to >> + * clear the global configuration lock. Delay 1 second to allow >> + * hardware to settle then issue a CORER >> + */ >> + ice_msec_delay(1000, true); >> ice_reset(hw, ICE_RESET_CORER); >> - /* CORER will clear the global lock, so no explicit call >> - * required for release >> - */ >> - return 0; >> + ice_check_reset(hw); >> + >> +reinit_hw: >> + /* Since we triggered a CORER, re-initialize hardware */ >> + ice_deinit_hw(hw); >> + if (ice_init_hw(hw)) { >> + ice_debug(hw, ICE_DBG_INIT, "Failed to re-init hardware after setting Tx topology\n"); >> + return ICE_ERR_RESET_FAILED; >> + } >> + >> + return status; >> } > > There is a similer deinit + init combo in ice_init_pkg in the same file, > which is the only use of this function (in DPDK anyway). Therefore, I > believe that that code can be removed now that the reinit is handled by the > cfg_tx_topo() function. I am not sure we can do that. We program Tx topology by taking the global configuration lock, then programming the topology, and then performing a CORE reset. We can't simply release the global config lock, because then DDP won't be able to download at all. We can't avoid this re-init, because after a CORE reset, the drivers AQ setup will be hosed and thus we need a re-init. ice_init_pkg should be called after Tx topology is executed. If another reset occurs there, I think you may still need to do that re-init too. There could possibly be a complete refactor that removed the need for a double reset, but I am not certain.