netdev - Re: [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <95ddc646-d348-45e3-b1f8-b0f114163b11@intel.com>
Date: Fri, 18 Jul 2025 12:56:29 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Simon Horman <horms@...nel.org>
CC: Anthony Nguyen <anthony.l.nguyen@...el.com>, Intel Wired LAN
	<intel-wired-lan@...ts.osuosl.org>, Aleksandr Loktionov
	<aleksandr.loktionov@...el.com>, <vgrinber@...hat.com>,
	<netdev@...r.kernel.org>
Subject: Re: [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 <jacob.e.keller@...el.com>
> 
> 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 <horms@...nel.org>
> 
> 
>> ---
>>  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;
>>  }
> 
> ...



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)