* Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove [not found] <20200621095136.7xdbzkthoxuw2qow@debian.debian-2> @ 2020-09-07 10:28 ` Kalle Valo 2020-09-21 13:27 ` Kalle Valo 2020-09-22 7:41 ` Kalle Valo 1 sibling, 1 reply; 4+ messages in thread From: Kalle Valo @ 2020-09-07 10:28 UTC (permalink / raw) To: Bo YU; +Cc: rmanohar, linux-wireless, ath11k, kuba, davem, kvalo + 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 -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove 2020-09-07 10:28 ` [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove Kalle Valo @ 2020-09-21 13:27 ` Kalle Valo 2020-09-21 17:21 ` Rajkumar Manoharan 0 siblings, 1 reply; 4+ messages in thread From: Kalle Valo @ 2020-09-21 13:27 UTC (permalink / raw) To: Bo YU; +Cc: kuba, linux-wireless, ath11k, rmanohar, davem 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 -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove 2020-09-21 13:27 ` Kalle Valo @ 2020-09-21 17:21 ` Rajkumar Manoharan 0 siblings, 0 replies; 4+ messages in thread From: Rajkumar Manoharan @ 2020-09-21 17:21 UTC (permalink / raw) To: Kalle Valo; +Cc: linux-wireless, kuba, Bo YU, ath11k, davem 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 -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove [not found] <20200621095136.7xdbzkthoxuw2qow@debian.debian-2> 2020-09-07 10:28 ` [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove Kalle Valo @ 2020-09-22 7:41 ` Kalle Valo 1 sibling, 0 replies; 4+ messages in thread From: Kalle Valo @ 2020-09-22 7:41 UTC (permalink / raw) To: Bo YU; +Cc: kuba, linux-wireless, davem, ath11k 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 -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-22 7:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200621095136.7xdbzkthoxuw2qow@debian.debian-2>
2020-09-07 10:28 ` [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove Kalle Valo
2020-09-21 13:27 ` Kalle Valo
2020-09-21 17:21 ` Rajkumar Manoharan
2020-09-22 7:41 ` Kalle Valo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox