On 7/18/2025 9:50 AM, Simon Horman wrote:
> On Thu, Jul 17, 2025 at 09:57:09AM -0700, Jacob Keller wrote:
>>
>> Fixes: 91427e6d9030 ("ice: Support 5 layer topology")
>> Signed-off-by: Jacob Keller <[email protected]>
>
> Thanks for the extensive explanation.
> Thanks. This took me forever to track down exactly what went wrong, enough that I had to have the customer send me the card back because we thought the firmware was unrecoverable and bricked. > I have a minor nit below, but that notwithstanding this looks good to me. > > Reviewed-by: Simon Horman <[email protected]> > > >> --- >> drivers/net/ethernet/intel/ice/ice_ddp.c | 44 >> ++++++++++++++++++++++--------- >> drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++++---- >> 2 files changed, 41 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c >> b/drivers/net/ethernet/intel/ice/ice_ddp.c >> index 59323c019544..bc525de019de 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c >> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c >> @@ -2374,7 +2374,13 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 >> buf_size, >> * The function will apply the new Tx topology from the package buffer >> * if available. >> * >> - * Return: zero when update was successful, negative values otherwise. >> + * Return: >> + * * 0 - Successfully applied topology configuration. >> + * * -EBUSY - Failed to acquire global configuration lock. >> + * * -EEXIST - Topology configuration has already been applied. >> + * * -EIO - Unable to apply topology configuration. >> + * * -ENODEV - Failed to re-initialize device after applying configuration. >> + * * Other negative error codes indicate unexpected failures. >> */ >> int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len) >> { >> @@ -2407,7 +2413,7 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void >> *buf, u32 len) >> >> if (status) { >> ice_debug(hw, ICE_DBG_INIT, "Get current topology is failed\n"); >> - return status; >> + return -EIO; >> } >> >> /* Is default topology already applied ? */ >> @@ -2494,31 +2500,45 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void >> *buf, u32 len) >> ICE_GLOBAL_CFG_LOCK_TIMEOUT); >> if (status) { >> ice_debug(hw, ICE_DBG_INIT, "Failed to acquire global lock\n"); >> - return status; >> + return -EBUSY; >> } >> >> /* Check if reset was triggered already. */ >> 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, "Failed setting Tx topology\n"); >> - return status; >> + ice_debug(hw, ICE_DBG_INIT, "Failed to set Tx topology, status >> %pe\n", >> + ERR_PTR(status)); >> + /* only report -EIO here as the caller checks the error value >> + * and reports an informational error message informing that >> + * the driver failed to program Tx topology. >> + */ >> + status = -EIO; >> } >> >> - /* New topology is updated, delay 1 second before issuing the CORER */ >> + /* 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 >> + */ >> msleep(1000); >> ice_reset(hw, ICE_RESET_CORER); >> - /* CORER will clear the global lock, so no explicit call >> - * required for release. >> - */ >> + ice_check_reset(hw); >> >> - return 0; >> +reinit_hw: > > nit: I think you can move this label above ice_check_reset(). > As the only place that jumps to this label calls ice_check_reset() > immediately before doing so. If so, renaming the label might > also be appropriate (up to you on all fronts:) > You're right thats probably slightly better. I'm not sure its worth a re-roll vs getting this fix out since its a pretty minor difference. >> + /* 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 -ENODEV; >> + } >> + >> + return status; >> } > > ...
OpenPGP_signature.asc
Description: OpenPGP digital signature
