[PATCH -next] ath11k: Add checked value for ath11k_ahb_remove

[PATCH -next] ath11k: Add checked value for ath11k_ahb_remove

[Bo YU]

Return value form wait_for_completion_timeout should to be checked.

This is detected by Coverity,#CID:1464479 (CHECKED_RETURN)

FIXES: d5c65159f2895(ath11k: driver for Qualcomm IEEE 802.11ax devices)
Signed-off-by: Bo YU <tsu.yubo@gmail.com>
---
 drivers/net/wireless/ath/ath11k/ahb.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
index 30092841ac46..1bbe30dceaf9 100644
--- a/drivers/net/wireless/ath/ath11k/ahb.c
+++ b/drivers/net/wireless/ath/ath11k/ahb.c
@@ -981,12 +981,16 @@ static int ath11k_ahb_probe(struct platform_device *pdev)
 static int ath11k_ahb_remove(struct platform_device *pdev)
 {
 	struct ath11k_base *ab = platform_get_drvdata(pdev);
-
+	int ret = 0;
 	reinit_completion(&ab->driver_recovery);

 	if (test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
-		wait_for_completion_timeout(&ab->driver_recovery,
-					    ATH11K_AHB_RECOVERY_TIMEOUT);
+		if (!wait_for_completion_timeout(&ab->driver_recovery,
+						 ATH11K_AHB_RECOVERY_TIMEOUT)) {
+			ath11k_warn(ab, "fail to receive recovery response completion.\n");
+			ret = -ETIMEDOUT;
+		}
+

 	set_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags);
 	cancel_work_sync(&ab->restart_work);
@@ -999,7 +1003,7 @@ static int ath11k_ahb_remove(struct platform_device *pdev)
 	ath11k_core_free(ab);
 	platform_set_drvdata(pdev, NULL);

-	return 0;
+	return ret;
 }

 static struct platform_driver ath11k_ahb_driver = {
--
2.11.0

Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove

[Kalle Valo]

+ rajkumar

Bo YU <tsu.yubo@gmail.com> writes:

> Return value form wait_for_completion_timeout should to be checked.
>
> This is detected by Coverity,#CID:1464479 (CHECKED_RETURN)
>
> FIXES: d5c65159f2895(ath11k: driver for Qualcomm IEEE 802.11ax devices)

This should be

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")

But I can fix that.

> --- a/drivers/net/wireless/ath/ath11k/ahb.c
> +++ b/drivers/net/wireless/ath/ath11k/ahb.c
> @@ -981,12 +981,16 @@ static int ath11k_ahb_probe(struct platform_device *pdev)
> static int ath11k_ahb_remove(struct platform_device *pdev)
> {
> 	struct ath11k_base *ab = platform_get_drvdata(pdev);
> -
> +	int ret = 0;
> 	reinit_completion(&ab->driver_recovery);
>
> 	if (test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
> -		wait_for_completion_timeout(&ab->driver_recovery,
> -					    ATH11K_AHB_RECOVERY_TIMEOUT);
> +		if (!wait_for_completion_timeout(&ab->driver_recovery,
> +						 ATH11K_AHB_RECOVERY_TIMEOUT)) {
> +			ath11k_warn(ab, "fail to receive recovery response completion.\n");
> +			ret = -ETIMEDOUT;
> +		}

This is a good find. Rajkumar, can you take a look if this is ok?

>
> 	set_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags);
> 	cancel_work_sync(&ab->restart_work);
> @@ -999,7 +1003,7 @@ static int ath11k_ahb_remove(struct platform_device *pdev)
> 	ath11k_core_free(ab);
> 	platform_set_drvdata(pdev, NULL);
>
> -	return 0;
> +	return ret;
> }

Especially I wonder what happens if ath11k_ahb_remove() returns an
error. Should we just print a warning and return 0 instead?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove

[Kalle Valo]

Kalle Valo <kvalo@codeaurora.org> writes:

> + rajkumar
>
> Bo YU <tsu.yubo@gmail.com> writes:
>
>> Return value form wait_for_completion_timeout should to be checked.
>>
>> This is detected by Coverity,#CID:1464479 (CHECKED_RETURN)
>>
>> FIXES: d5c65159f2895(ath11k: driver for Qualcomm IEEE 802.11ax devices)
>
> This should be
>
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
>
> But I can fix that.
>
>> --- a/drivers/net/wireless/ath/ath11k/ahb.c
>> +++ b/drivers/net/wireless/ath/ath11k/ahb.c
>> @@ -981,12 +981,16 @@ static int ath11k_ahb_probe(struct platform_device *pdev)
>> static int ath11k_ahb_remove(struct platform_device *pdev)
>> {
>> 	struct ath11k_base *ab = platform_get_drvdata(pdev);
>> -
>> +	int ret = 0;
>> 	reinit_completion(&ab->driver_recovery);
>>
>> 	if (test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
>> -		wait_for_completion_timeout(&ab->driver_recovery,
>> -					    ATH11K_AHB_RECOVERY_TIMEOUT);
>> +		if (!wait_for_completion_timeout(&ab->driver_recovery,
>> +						 ATH11K_AHB_RECOVERY_TIMEOUT)) {
>> + ath11k_warn(ab, "fail to receive recovery response
>> completion.\n");
>> +			ret = -ETIMEDOUT;
>> +		}
>
> This is a good find. Rajkumar, can you take a look if this is ok?
>
>>
>> 	set_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags);
>> 	cancel_work_sync(&ab->restart_work);
>> @@ -999,7 +1003,7 @@ static int ath11k_ahb_remove(struct platform_device *pdev)
>> 	ath11k_core_free(ab);
>> 	platform_set_drvdata(pdev, NULL);
>>
>> -	return 0;
>> +	return ret;
>> }
>
> Especially I wonder what happens if ath11k_ahb_remove() returns an
> error. Should we just print a warning and return 0 instead?

I changed this patch so that we return 0 even if timeout happens, just
to be on the safe side. The patch is now in the pending branch.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove

[Rajkumar Manoharan]

On 2020-09-21 06:27, Kalle Valo wrote:
> Kalle Valo <kvalo@codeaurora.org> writes:
> 
>> + rajkumar
>> 
>> Bo YU <tsu.yubo@gmail.com> writes:
>> 
>>> Return value form wait_for_completion_timeout should to be checked.
>>> 
>>> This is detected by Coverity,#CID:1464479 (CHECKED_RETURN)
>>> 
>>> FIXES: d5c65159f2895(ath11k: driver for Qualcomm IEEE 802.11ax 
>>> devices)
>> 
>> This should be
>> 
>> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax 
>> devices")
>> 
>> But I can fix that.
>> 
>>> --- a/drivers/net/wireless/ath/ath11k/ahb.c
>>> +++ b/drivers/net/wireless/ath/ath11k/ahb.c
>>> @@ -981,12 +981,16 @@ static int ath11k_ahb_probe(struct 
>>> platform_device *pdev)
>>> static int ath11k_ahb_remove(struct platform_device *pdev)
>>> {
>>> 	struct ath11k_base *ab = platform_get_drvdata(pdev);
>>> -
>>> +	int ret = 0;
>>> 	reinit_completion(&ab->driver_recovery);
>>> 
>>> 	if (test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
>>> -		wait_for_completion_timeout(&ab->driver_recovery,
>>> -					    ATH11K_AHB_RECOVERY_TIMEOUT);
>>> +		if (!wait_for_completion_timeout(&ab->driver_recovery,
>>> +						 ATH11K_AHB_RECOVERY_TIMEOUT)) {
>>> + ath11k_warn(ab, "fail to receive recovery response
>>> completion.\n");
> 

>>> +			ret = -ETIMEDOUT;
>>> +		}
>> 
>> This is a good find. Rajkumar, can you take a look if this is ok?
> 
Sorry for the delay. wait_for_completion status check LGTM. But return 0 
is
intentional as it is required to complete platform deinit properly. 
Better
to improve the logging message.

>>> 
>>> 	set_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags);
>>> 	cancel_work_sync(&ab->restart_work);
>>> @@ -999,7 +1003,7 @@ static int ath11k_ahb_remove(struct 
>>> platform_device *pdev)
>>> 	ath11k_core_free(ab);
>>> 	platform_set_drvdata(pdev, NULL);
>>> 
>>> -	return 0;
>>> +	return ret;
>>> }
>> 
>> Especially I wonder what happens if ath11k_ahb_remove() returns an
>> error. Should we just print a warning and return 0 instead?
> 
> I changed this patch so that we return 0 even if timeout happens, just
> to be on the safe side. The patch is now in the pending branch.
> 

Thanks for taking care of this.

Rajkumar

Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove

[Kalle Valo]

Bo YU <tsu.yubo@gmail.com> wrote:

> Return value form wait_for_completion_timeout should to be checked.
> 
> This is detected by Coverity: #CID:1464479 (CHECKED_RETURN)
> 
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
> Signed-off-by: Bo YU <tsu.yubo@gmail.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

80b892fc8a90 ath11k: Add checked value for ath11k_ahb_remove

-- 
https://patchwork.kernel.org/patch/11616495/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches