Re: [Intel-wired-lan] [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails
> -Original Message- > From: Intel-wired-lan On Behalf Of Jacob > Keller > Sent: 17 July 2025 22:27 > To: Nguyen, Anthony L ; Intel Wired LAN > ; Loktionov, Aleksandr > > Cc: Keller, Jacob E ; Grinberg, Vitaly > ; [email protected] > Subject: [Intel-wired-lan] [PATCH iwl-net 2/2] ice: don't leave device > non-functional if Tx scheduler config fails > > 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 CORER 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 CORER 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 CORER. > > 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 by CORER 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. > > The only currently known way that ice_get_set_tx_topo() can fail is with > certain older DDP packages which contain invalid topology configuration, on > firmware versions which strictly validate this data. The most recent releases > of the DDP have resolved the invalid data. However, it is still poor practice > to essentially brick the device, and prevent access to the device even > through safe mode or recovery mode. It is also plausible that this command > could fail for some other reason in the future. > > 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 > file loaded from /lib/firmware. > > Instead, we need to trigger CORER 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 CORER 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 CORER. Thus, it > makes sense for this to be the responsibility of ice_cfg_tx_topo() rather > than its caller, ice_init_tx_topology(). This avoids needless > re-initialization in cases where we don't attempt to update the Tx scheduler > topology, such as if it has already been programmed. > > There is one catch: failure to re-initialize the HW struct should stop > ice_probe(). If this function fails, we won't have a valid HW structure and > cannot ensure the device is functioning properly. To handle this, ensure Ice_cfg_tx_topo() returns a limited set of error codes. Set aside one specifically, -ENODEV, to indicate that the ice_init_tx_topology() should fail and stop probe. > > > Other error codes indicate failure to apply the Tx scheduler topology. This > is treated as a non-fatal error, with an inf
Re: [Intel-wired-lan] [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails
On Fri, Jul 18, 2025 at 12:56:29PM -0700, Jacob Keller wrote:
>
>
> 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
> >
> > 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.
Ouch!
...
> >>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.
Yes, agreed. I'm happy to let this lie if you prefer.
...
Re: [Intel-wired-lan] [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails
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
>
> 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
>
>
>> ---
>> 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
Re: [Intel-wired-lan] [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails
On Thu, Jul 17, 2025 at 09:57:09AM -0700, Jacob Keller wrote: > 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 CORER 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 CORER 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 CORER. > > 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 by CORER 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. > > The only currently known way that ice_get_set_tx_topo() can fail is with > certain older DDP packages which contain invalid topology configuration, on > firmware versions which strictly validate this data. The most recent > releases of the DDP have resolved the invalid data. However, it is still > poor practice to essentially brick the device, and prevent access to the > device even through safe mode or recovery mode. It is also plausible that > this command could fail for some other reason in the future. > > 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 file loaded from /lib/firmware. > > Instead, we need to trigger CORER 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 CORER 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 CORER. Thus, > it makes sense for this to be the responsibility of ice_cfg_tx_topo() > rather than its caller, ice_init_tx_topology(). This avoids needless > re-initialization in cases where we don't attempt to update the Tx > scheduler topology, such as if it has already been programmed. > > There is one catch: failure to re-initialize the HW struct should stop > ice_probe(). If this function fails, we won't have a valid HW structure and > cannot ensure the device is functioning properly. To handle this, ensure > ice_cfg_tx_topo() returns a limited set of error codes. Set aside one > specifically, -ENODEV, to indicate that the ice_init_tx_topology() should > fail and stop probe. > > Other error codes indicate failure to apply the Tx scheduler topology. This > is treated as a non-fatal error, with an informational message informing > the system administrator that the updated Tx topology did not apply. This > allows the device to load and function with the default Tx scheduler > topology, rather than failing to load entirely. > > Note that this use of CORER will not result in loops with future PFs > attempting to also lo
