[PATCH net-next] ice: Fix incorrect timeout in ice_release_res()

[PATCH net-next] ice: Fix incorrect timeout in ice_release_res()

[Ding Hui]

The commit 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
ice_sq_done timeout") converted ICE_CTL_Q_SQ_CMD_TIMEOUT from jiffies
to microseconds.

But the ice_release_res() function was missed, and its logic still
treats ICE_CTL_Q_SQ_CMD_TIMEOUT as a jiffies value.

So correct the issue by usecs_to_jiffies().

Fixes: 5f6df173f92e ("ice: implement and use rd32_poll_timeout for ice_sq_done timeout")
Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
 drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 6fb0c1e8ae7c..5005c299deb1 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1885,7 +1885,7 @@ void ice_release_res(struct ice_hw *hw, enum ice_aq_res_ids res)
 	/* there are some rare cases when trying to release the resource
 	 * results in an admin queue timeout, so handle them correctly
 	 */
-	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
+	timeout = jiffies + 10 * usecs_to_jiffies(ICE_CTL_Q_SQ_CMD_TIMEOUT);
 	do {
 		status = ice_aq_release_res(hw, res, 0, NULL);
 		if (status != -EIO)
-- 
2.17.1

Re: [PATCH net-next] ice: Fix incorrect timeout in ice_release_res()

[Simon Horman]

On Fri, Dec 05, 2025 at 04:16:08PM +0800, Ding Hui wrote:
> The commit 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
> ice_sq_done timeout") converted ICE_CTL_Q_SQ_CMD_TIMEOUT from jiffies
> to microseconds.
> 
> But the ice_release_res() function was missed, and its logic still
> treats ICE_CTL_Q_SQ_CMD_TIMEOUT as a jiffies value.
> 
> So correct the issue by usecs_to_jiffies().
> 
> Fixes: 5f6df173f92e ("ice: implement and use rd32_poll_timeout for ice_sq_done timeout")
> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>

Thanks,

I agree with the analysis above and that the problem was introduced
by the cited commit.

As a fix for code present in net this should probably be targeted
at net (or iwl-net?) rather than net-next. But perhaps there is
no need to repost just to address that.

> ---
>  drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 6fb0c1e8ae7c..5005c299deb1 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1885,7 +1885,7 @@ void ice_release_res(struct ice_hw *hw, enum ice_aq_res_ids res)
>  	/* there are some rare cases when trying to release the resource
>  	 * results in an admin queue timeout, so handle them correctly
>  	 */
> -	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
> +	timeout = jiffies + 10 * usecs_to_jiffies(ICE_CTL_Q_SQ_CMD_TIMEOUT);
>  	do {
>  		status = ice_aq_release_res(hw, res, 0, NULL);
>  		if (status != -EIO)

I agree this minimal change is appropriate as a bug fix.

But I think that it would be good to provide a follow-up
that reworks this code a bit to to use read_poll_timeout().
As per the aim of the cited commit.

This should be targeted at net-next (or iwl-next?).
Once this bug fix propagates to in net-next.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next] ice: Fix incorrect timeout in ice_release_res()

[Ding Hui]

On 2025/12/6 0:16, Simon Horman wrote:
> On Fri, Dec 05, 2025 at 04:16:08PM +0800, Ding Hui wrote:
>> The commit 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
>> ice_sq_done timeout") converted ICE_CTL_Q_SQ_CMD_TIMEOUT from jiffies
>> to microseconds.
>>
>> But the ice_release_res() function was missed, and its logic still
>> treats ICE_CTL_Q_SQ_CMD_TIMEOUT as a jiffies value.
>>
>> So correct the issue by usecs_to_jiffies().
>>
>> Fixes: 5f6df173f92e ("ice: implement and use rd32_poll_timeout for ice_sq_done timeout")
>> Signed-off-by: Ding Hui<dinghui@sangfor.com.cn>
> Thanks,
> 
> I agree with the analysis above and that the problem was introduced
> by the cited commit.
> 
> As a fix for code present in net this should probably be targeted
> at net (or iwl-net?) rather than net-next. But perhaps there is
> no need to repost just to address that.
> 

Sorry, I mixed up the purposes of the net and net-next branches, thank
you for pointing that out, it should be net.
I'll keep that in mind in the future.

>> ---
>>   drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
>> index 6fb0c1e8ae7c..5005c299deb1 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>> @@ -1885,7 +1885,7 @@ void ice_release_res(struct ice_hw *hw, enum ice_aq_res_ids res)
>>   	/* there are some rare cases when trying to release the resource
>>   	 * results in an admin queue timeout, so handle them correctly
>>   	 */
>> -	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
>> +	timeout = jiffies + 10 * usecs_to_jiffies(ICE_CTL_Q_SQ_CMD_TIMEOUT);
>>   	do {
>>   		status = ice_aq_release_res(hw, res, 0, NULL);
>>   		if (status != -EIO)
> I agree this minimal change is appropriate as a bug fix.
> 
> But I think that it would be good to provide a follow-up
> that reworks this code a bit to to use read_poll_timeout().
> As per the aim of the cited commit.
> 

Actually, the ice_aq_release_res() called by ice_release_res() already
implements the underlying logic via read_poll_timeout() by that commit,
and here is primarily responsible for retrying the important release
command.

> This should be targeted at net-next (or iwl-next?).
> Once this bug fix propagates to in net-next.
> 
> Reviewed-by: Simon Horman<horms@kernel.org>
> 
> 

-- 
Thanks,
- Ding Hui

RE: [Intel-wired-lan] [PATCH net-next] ice: Fix incorrect timeout in ice_release_res()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Ding Hui
> Sent: Friday, December 5, 2025 9:16 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Keller, Jacob E <jacob.e.keller@intel.com>; intel-
> wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ding, Hui
> <dinghui@sangfor.com.cn>
> Subject: [Intel-wired-lan] [PATCH net-next] ice: Fix incorrect timeout
> in ice_release_res()
> 
> The commit 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
> ice_sq_done timeout") converted ICE_CTL_Q_SQ_CMD_TIMEOUT from jiffies
> to microseconds.
> 
> But the ice_release_res() function was missed, and its logic still
> treats ICE_CTL_Q_SQ_CMD_TIMEOUT as a jiffies value.
> 
> So correct the issue by usecs_to_jiffies().
> 

Please add a brief "how verified" paragraph (platform + steps).
This is a unit-conversion fix in a timeout path; a short test description helps reviewers and stable backports validate the change.

And you can add my:
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>


> Fixes: 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
> ice_sq_done timeout")
> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
> ---
>  drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
> b/drivers/net/ethernet/intel/ice/ice_common.c
> index 6fb0c1e8ae7c..5005c299deb1 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1885,7 +1885,7 @@ void ice_release_res(struct ice_hw *hw, enum
> ice_aq_res_ids res)
>  	/* there are some rare cases when trying to release the
> resource
>  	 * results in an admin queue timeout, so handle them correctly
>  	 */
> -	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
> +	timeout = jiffies + 10 *
> usecs_to_jiffies(ICE_CTL_Q_SQ_CMD_TIMEOUT);
>  	do {
>  		status = ice_aq_release_res(hw, res, 0, NULL);
>  		if (status != -EIO)
> --
> 2.17.1

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix incorrect timeout in ice_release_res()

[Ding Hui]

On 2025/12/6 5:09, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
>> Of Ding Hui
>> Sent: Friday, December 5, 2025 9:16 AM
>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
>> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
>> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>> pabeni@redhat.com; Keller, Jacob E <jacob.e.keller@intel.com>; intel-
>> wired-lan@lists.osuosl.org
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ding, Hui
>> <dinghui@sangfor.com.cn>
>> Subject: [Intel-wired-lan] [PATCH net-next] ice: Fix incorrect timeout
>> in ice_release_res()
>>
>> The commit 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
>> ice_sq_done timeout") converted ICE_CTL_Q_SQ_CMD_TIMEOUT from jiffies
>> to microseconds.
>>
>> But the ice_release_res() function was missed, and its logic still
>> treats ICE_CTL_Q_SQ_CMD_TIMEOUT as a jiffies value.
>>
>> So correct the issue by usecs_to_jiffies().
>>
> 
> Please add a brief "how verified" paragraph (platform + steps).
> This is a unit-conversion fix in a timeout path; a short test description helps reviewers and stable backports validate the change.
> 
Sorry for not being able to provide the verification information, as
I haven't actually encountered this issue.

The ice_release_res() is almost always invoked during downloading DDP
when modprobe ice.

IMO, it seems like that only when the NIC hardware or firmware enters
a bad state causing single command to fail or timeout (1 second), and
then here do the retry logic (10 senconds).

So it's hard to validate on healthy NIC, maybe inject faults in low level
function, such as ice_sq_send_cmd().

> And you can add my:
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 
> 
>> Fixes: 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
>> ice_sq_done timeout")
>> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
>> b/drivers/net/ethernet/intel/ice/ice_common.c
>> index 6fb0c1e8ae7c..5005c299deb1 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>> @@ -1885,7 +1885,7 @@ void ice_release_res(struct ice_hw *hw, enum
>> ice_aq_res_ids res)
>>   	/* there are some rare cases when trying to release the
>> resource
>>   	 * results in an admin queue timeout, so handle them correctly
>>   	 */
>> -	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
>> +	timeout = jiffies + 10 *
>> usecs_to_jiffies(ICE_CTL_Q_SQ_CMD_TIMEOUT);
>>   	do {
>>   		status = ice_aq_release_res(hw, res, 0, NULL);
>>   		if (status != -EIO)
>> --
>> 2.17.1
> 
> 
> 

-- 
Thanks,
- Ding Hui

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix incorrect timeout in ice_release_res()

[Simon Horman]

On Sat, Dec 06, 2025 at 10:42:36AM +0800, Ding Hui wrote:
> On 2025/12/6 5:09, Loktionov, Aleksandr wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> > > Of Ding Hui
> > > Sent: Friday, December 5, 2025 9:16 AM
> > > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> > > Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > > pabeni@redhat.com; Keller, Jacob E <jacob.e.keller@intel.com>; intel-
> > > wired-lan@lists.osuosl.org
> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ding, Hui
> > > <dinghui@sangfor.com.cn>
> > > Subject: [Intel-wired-lan] [PATCH net-next] ice: Fix incorrect timeout
> > > in ice_release_res()
> > > 
> > > The commit 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
> > > ice_sq_done timeout") converted ICE_CTL_Q_SQ_CMD_TIMEOUT from jiffies
> > > to microseconds.
> > > 
> > > But the ice_release_res() function was missed, and its logic still
> > > treats ICE_CTL_Q_SQ_CMD_TIMEOUT as a jiffies value.
> > > 
> > > So correct the issue by usecs_to_jiffies().
> > > 
> > 
> > Please add a brief "how verified" paragraph (platform + steps).
> > This is a unit-conversion fix in a timeout path; a short test description helps reviewers and stable backports validate the change.
> > 
> Sorry for not being able to provide the verification information, as
> I haven't actually encountered this issue.
> 
> The ice_release_res() is almost always invoked during downloading DDP
> when modprobe ice.
> 
> IMO, it seems like that only when the NIC hardware or firmware enters
> a bad state causing single command to fail or timeout (1 second), and
> then here do the retry logic (10 senconds).
> 
> So it's hard to validate on healthy NIC, maybe inject faults in low level
> function, such as ice_sq_send_cmd().

In that case I would suggest adding something like this:

Found by inspection (or static analysis, or a specific tool if publicly
available, ...).
Compile tested only.

> 
> > And you can add my:
> > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > 
> > 
> > > Fixes: 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
> > > ice_sq_done timeout")
> > > Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
> > > ---
> > >   drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
> > > b/drivers/net/ethernet/intel/ice/ice_common.c
> > > index 6fb0c1e8ae7c..5005c299deb1 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_common.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> > > @@ -1885,7 +1885,7 @@ void ice_release_res(struct ice_hw *hw, enum
> > > ice_aq_res_ids res)
> > >   	/* there are some rare cases when trying to release the
> > > resource
> > >   	 * results in an admin queue timeout, so handle them correctly
> > >   	 */
> > > -	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
> > > +	timeout = jiffies + 10 *
> > > usecs_to_jiffies(ICE_CTL_Q_SQ_CMD_TIMEOUT);
> > >   	do {
> > >   		status = ice_aq_release_res(hw, res, 0, NULL);
> > >   		if (status != -EIO)
> > > --
> > > 2.17.1
> > 
> > 
> > 
> 
> -- 
> Thanks,
> - Ding Hui
> 
> 

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix incorrect timeout in ice_release_res()

[Ding Hui]

On 2025/12/6 17:46, Simon Horman wrote:
> On Sat, Dec 06, 2025 at 10:42:36AM +0800, Ding Hui wrote:
>> On 2025/12/6 5:09, Loktionov, Aleksandr wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
>>>> Of Ding Hui
>>>> Sent: Friday, December 5, 2025 9:16 AM
>>>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
>>>> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
>>>> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>>>> pabeni@redhat.com; Keller, Jacob E <jacob.e.keller@intel.com>; intel-
>>>> wired-lan@lists.osuosl.org
>>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ding, Hui
>>>> <dinghui@sangfor.com.cn>
>>>> Subject: [Intel-wired-lan] [PATCH net-next] ice: Fix incorrect timeout
>>>> in ice_release_res()
>>>>
>>>> The commit 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
>>>> ice_sq_done timeout") converted ICE_CTL_Q_SQ_CMD_TIMEOUT from jiffies
>>>> to microseconds.
>>>>
>>>> But the ice_release_res() function was missed, and its logic still
>>>> treats ICE_CTL_Q_SQ_CMD_TIMEOUT as a jiffies value.
>>>>
>>>> So correct the issue by usecs_to_jiffies().
>>>>
>>>
>>> Please add a brief "how verified" paragraph (platform + steps).
>>> This is a unit-conversion fix in a timeout path; a short test description helps reviewers and stable backports validate the change.
>>>
>> Sorry for not being able to provide the verification information, as
>> I haven't actually encountered this issue.
>>
>> The ice_release_res() is almost always invoked during downloading DDP
>> when modprobe ice.
>>
>> IMO, it seems like that only when the NIC hardware or firmware enters
>> a bad state causing single command to fail or timeout (1 second), and
>> then here do the retry logic (10 senconds).
>>
>> So it's hard to validate on healthy NIC, maybe inject faults in low level
>> function, such as ice_sq_send_cmd().
> 
> In that case I would suggest adding something like this:
> 
> Found by inspection (or static analysis, or a specific tool if publicly
> available, ...).
> Compile tested only.
> 

Sure, I'll send v2 later.

>>
>>> And you can add my:
>>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>>
>>>
>>>> Fixes: 5f6df173f92e ("ice: implement and use rd32_poll_timeout for
>>>> ice_sq_done timeout")
>>>> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
>>>> ---
>>>>    drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
>>>> b/drivers/net/ethernet/intel/ice/ice_common.c
>>>> index 6fb0c1e8ae7c..5005c299deb1 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>>>> @@ -1885,7 +1885,7 @@ void ice_release_res(struct ice_hw *hw, enum
>>>> ice_aq_res_ids res)
>>>>    	/* there are some rare cases when trying to release the
>>>> resource
>>>>    	 * results in an admin queue timeout, so handle them correctly
>>>>    	 */
>>>> -	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
>>>> +	timeout = jiffies + 10 *
>>>> usecs_to_jiffies(ICE_CTL_Q_SQ_CMD_TIMEOUT);
>>>>    	do {
>>>>    		status = ice_aq_release_res(hw, res, 0, NULL);
>>>>    		if (status != -EIO)
>>>> --
>>>> 2.17.1
>>>
>>>
>>>
>>
>> -- 
>> Thanks,
>> - Ding Hui
>>
>>
> 
> 

-- 
Thanks,
-dinghui