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 <[email protected]>
---
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
+ rajkumar
Bo YU <[email protected]> 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
Kalle Valo <[email protected]> writes:
> + rajkumar
>
> Bo YU <[email protected]> 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
On 2020-09-21 06:27, Kalle Valo wrote:
> Kalle Valo <[email protected]> writes:
>
>> + rajkumar
>>
>> Bo YU <[email protected]> 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
Bo YU <[email protected]> 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 <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
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