| Message ID | 20231113044113.74732-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [wireless-next] wlcore: debugfs: add an error code check in beacon_filtering_write | expand |
Su Hui <suhui@nfschina.com> writes: > wl1271_acx_beacon_filter_opt() return error code if failed, add a check > for this is better and safer. > > Signed-off-by: Su Hui <suhui@nfschina.com> How did you test this?
On 2023/11/13 14:16, Kalle Valo wrote: > Su Hui <suhui@nfschina.com> writes: > >> wl1271_acx_beacon_filter_opt() return error code if failed, add a check >> for this is better and safer. >> >> Signed-off-by: Su Hui <suhui@nfschina.com> > How did you test this? > Hi， Only compile test. Clang static checker complains about this code that value stored to 'ret' is never read. And most of the caller check the error code of wl1271_acx_beacon_filter_opt(). Such like this: -- drivers/net/wireless/ti/wlcore/init.c:277: ret = wl1271_acx_beacon_filter_opt(wl, wlvif, false); drivers/net/wireless/ti/wlcore/init.c-278- if (ret < 0) drivers/net/wireless/ti/wlcore/init.c-279- return ret; -- drivers/net/wireless/ti/wlcore/init.c:410: ret = wl1271_acx_beacon_filter_opt(wl, wlvif, false); drivers/net/wireless/ti/wlcore/init.c-411- if (ret < 0) drivers/net/wireless/ti/wlcore/init.c-412- return ret; -- drivers/net/wireless/ti/wlcore/main.c:1652: ret = wl1271_acx_beacon_filter_opt(wl, wlvif, true); drivers/net/wireless/ti/wlcore/main.c-1653- if (ret < 0) drivers/net/wireless/ti/wlcore/main.c-1654- goto out; Sorry for the few test. Su Hui
Su Hui <suhui@nfschina.com> writes: > On 2023/11/13 14:16, Kalle Valo wrote: > >> Su Hui <suhui@nfschina.com> writes: >> >>> wl1271_acx_beacon_filter_opt() return error code if failed, add a check >>> for this is better and safer. >>> >>> Signed-off-by: Su Hui <suhui@nfschina.com> >> How did you test this? > > Only compile test. If you have only compile tested please document that clearly in the commit message. > Clang static checker complains about this code that value stored to > 'ret' is never read. This would be good to also have in the commit message. > And most of the caller check the error code of > wl1271_acx_beacon_filter_opt(). This might still break something so I would prefer to test this in a real device before taking it.
On 2023/11/13 20:07, Kalle Valo wrote: > Su Hui <suhui@nfschina.com> writes: > >> On 2023/11/13 14:16, Kalle Valo wrote: >> >>> Su Hui <suhui@nfschina.com> writes: >>> >>>> wl1271_acx_beacon_filter_opt() return error code if failed, add a check >>>> for this is better and safer. >>>> >>>> Signed-off-by: Su Hui <suhui@nfschina.com> >>> How did you test this? >> Only compile test. > If you have only compile tested please document that clearly in the > commit message. Sorry for the unclear commit message. >> Clang static checker complains about this code that value stored to >> 'ret' is never read. > This would be good to also have in the commit message. I will add this to v2 patch if pass a test in a real device. >> And most of the caller check the error code of >> wl1271_acx_beacon_filter_opt(). > This might still break something so I would prefer to test this in a > real device before taking it. This might take some time, I try to find a wlcore device to test this. Thanks for your reply! Su Hui
diff --git a/drivers/net/wireless/ti/wlcore/debugfs.c b/drivers/net/wireless/ti/wlcore/debugfs.c index eb3d3f0e0b4d..d4071291a8cd 100644 --- a/drivers/net/wireless/ti/wlcore/debugfs.c +++ b/drivers/net/wireless/ti/wlcore/debugfs.c @@ -932,13 +932,15 @@ static ssize_t beacon_filtering_write(struct file *file, wl12xx_for_each_wlvif(wl, wlvif) { ret = wl1271_acx_beacon_filter_opt(wl, wlvif, !!value); + if (ret < 0) + break; } pm_runtime_mark_last_busy(wl->dev); pm_runtime_put_autosuspend(wl->dev); out: mutex_unlock(&wl->mutex); - return count; + return ret < 0 ? ret : count; } static const struct file_operations beacon_filtering_ops = {
wl1271_acx_beacon_filter_opt() return error code if failed, add a check for this is better and safer. Signed-off-by: Su Hui <suhui@nfschina.com> --- drivers/net/wireless/ti/wlcore/debugfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)