| Message ID | 20200621095136.7xdbzkthoxuw2qow@debian.debian-2 (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 80b892fc8a90e91498babf0f6817139e5ec64b5c |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [-next] ath11k: Add checked value for ath11k_ahb_remove | expand |
+ 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?
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.
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
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
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 = {
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(-) -- 2.11.0