| Message ID | 20241023074051.309-1-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Johnson |
| Headers | show |
| Series | [1/1] ath11k: fix return value check in ath11k_spectral_debug_register() | expand |
On 10/23/2024 12:40 AM, Zhen Lei wrote: > Fix the incorrect return value check for debugfs_create_file(), which > returns ERR_PTR(-ERROR) instead of NULL when it fails. Based upon the commit text this change is incorrect. * NOTE: it's expected that most callers should _ignore_ the errors returned * by this function. Other debugfs functions handle the fact that the "dentry" * passed to them could be an error and they don't crash in that case. * Drivers should generally work fine even if debugfs fails to init anyway. So ath11k should not be checking the return value at all, and definitely should not be returning -EINVAL since the driver should still operate even if creating a debugfs file fails. > > Fixes: 9d11b7bff950 ("ath11k: add support for spectral scan") > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > drivers/net/wireless/ath/ath11k/spectral.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/spectral.c b/drivers/net/wireless/ath/ath11k/spectral.c > index 79e091134515b43..4c545231292142a 100644 > --- a/drivers/net/wireless/ath/ath11k/spectral.c > +++ b/drivers/net/wireless/ath/ath11k/spectral.c > @@ -942,7 +942,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) > 0600, > ar->debug.debugfs_pdev, ar, > &fops_scan_ctl); > - if (!ar->spectral.scan_ctl) { > + if (IS_ERR(ar->spectral.scan_ctl)) { > ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", > ar->pdev_idx); > ret = -EINVAL; > @@ -953,7 +953,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) > 0600, > ar->debug.debugfs_pdev, ar, > &fops_scan_count); > - if (!ar->spectral.scan_count) { > + if (IS_ERR(ar->spectral.scan_count)) { > ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", > ar->pdev_idx); > ret = -EINVAL; > @@ -964,7 +964,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) > 0600, > ar->debug.debugfs_pdev, ar, > &fops_scan_bins); > - if (!ar->spectral.scan_bins) { > + if (IS_ERR(ar->spectral.scan_bins)) { > ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", > ar->pdev_idx); > ret = -EINVAL;
On 2024/10/23 22:02, Jeff Johnson wrote: > On 10/23/2024 12:40 AM, Zhen Lei wrote: >> Fix the incorrect return value check for debugfs_create_file(), which >> returns ERR_PTR(-ERROR) instead of NULL when it fails. > > Based upon the commit text this change is incorrect. > > * NOTE: it's expected that most callers should _ignore_ the errors returned > * by this function. Other debugfs functions handle the fact that the "dentry" > * passed to them could be an error and they don't crash in that case. > * Drivers should generally work fine even if debugfs fails to init anyway. > > So ath11k should not be checking the return value at all, and definitely > should not be returning -EINVAL since the driver should still operate even if > creating a debugfs file fails. OK, so that members such as scan_ctl in struct ath11k_spectral can be removed. I will update accordingly in v2. > >> >> Fixes: 9d11b7bff950 ("ath11k: add support for spectral scan") >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> drivers/net/wireless/ath/ath11k/spectral.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath11k/spectral.c b/drivers/net/wireless/ath/ath11k/spectral.c >> index 79e091134515b43..4c545231292142a 100644 >> --- a/drivers/net/wireless/ath/ath11k/spectral.c >> +++ b/drivers/net/wireless/ath/ath11k/spectral.c >> @@ -942,7 +942,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) >> 0600, >> ar->debug.debugfs_pdev, ar, >> &fops_scan_ctl); >> - if (!ar->spectral.scan_ctl) { >> + if (IS_ERR(ar->spectral.scan_ctl)) { >> ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", >> ar->pdev_idx); >> ret = -EINVAL; >> @@ -953,7 +953,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) >> 0600, >> ar->debug.debugfs_pdev, ar, >> &fops_scan_count); >> - if (!ar->spectral.scan_count) { >> + if (IS_ERR(ar->spectral.scan_count)) { >> ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", >> ar->pdev_idx); >> ret = -EINVAL; >> @@ -964,7 +964,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) >> 0600, >> ar->debug.debugfs_pdev, ar, >> &fops_scan_bins); >> - if (!ar->spectral.scan_bins) { >> + if (IS_ERR(ar->spectral.scan_bins)) { >> ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", >> ar->pdev_idx); >> ret = -EINVAL; > > > . >
On 2024/10/24 9:30, Leizhen (ThunderTown) wrote: > > > On 2024/10/23 22:02, Jeff Johnson wrote: >> On 10/23/2024 12:40 AM, Zhen Lei wrote: >>> Fix the incorrect return value check for debugfs_create_file(), which >>> returns ERR_PTR(-ERROR) instead of NULL when it fails. >> >> Based upon the commit text this change is incorrect. >> >> * NOTE: it's expected that most callers should _ignore_ the errors returned >> * by this function. Other debugfs functions handle the fact that the "dentry" >> * passed to them could be an error and they don't crash in that case. >> * Drivers should generally work fine even if debugfs fails to init anyway. >> >> So ath11k should not be checking the return value at all, and definitely >> should not be returning -EINVAL since the driver should still operate even if >> creating a debugfs file fails. > > OK, so that members such as scan_ctl in struct ath11k_spectral can be removed. Sorry, the ath11k driver code is more complex than I think. I just went through the code, and those members still need to be kept. > I will update accordingly in v2.> >> >>> >>> Fixes: 9d11b7bff950 ("ath11k: add support for spectral scan") >>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>> --- >>> drivers/net/wireless/ath/ath11k/spectral.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath11k/spectral.c b/drivers/net/wireless/ath/ath11k/spectral.c >>> index 79e091134515b43..4c545231292142a 100644 >>> --- a/drivers/net/wireless/ath/ath11k/spectral.c >>> +++ b/drivers/net/wireless/ath/ath11k/spectral.c >>> @@ -942,7 +942,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) >>> 0600, >>> ar->debug.debugfs_pdev, ar, >>> &fops_scan_ctl); >>> - if (!ar->spectral.scan_ctl) { >>> + if (IS_ERR(ar->spectral.scan_ctl)) { >>> ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", >>> ar->pdev_idx); >>> ret = -EINVAL; >>> @@ -953,7 +953,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) >>> 0600, >>> ar->debug.debugfs_pdev, ar, >>> &fops_scan_count); >>> - if (!ar->spectral.scan_count) { >>> + if (IS_ERR(ar->spectral.scan_count)) { >>> ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", >>> ar->pdev_idx); >>> ret = -EINVAL; >>> @@ -964,7 +964,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) >>> 0600, >>> ar->debug.debugfs_pdev, ar, >>> &fops_scan_bins); >>> - if (!ar->spectral.scan_bins) { >>> + if (IS_ERR(ar->spectral.scan_bins)) { >>> ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", >>> ar->pdev_idx); >>> ret = -EINVAL; >> >> >> . >> >
diff --git a/drivers/net/wireless/ath/ath11k/spectral.c b/drivers/net/wireless/ath/ath11k/spectral.c index 79e091134515b43..4c545231292142a 100644 --- a/drivers/net/wireless/ath/ath11k/spectral.c +++ b/drivers/net/wireless/ath/ath11k/spectral.c @@ -942,7 +942,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) 0600, ar->debug.debugfs_pdev, ar, &fops_scan_ctl); - if (!ar->spectral.scan_ctl) { + if (IS_ERR(ar->spectral.scan_ctl)) { ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", ar->pdev_idx); ret = -EINVAL; @@ -953,7 +953,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) 0600, ar->debug.debugfs_pdev, ar, &fops_scan_count); - if (!ar->spectral.scan_count) { + if (IS_ERR(ar->spectral.scan_count)) { ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", ar->pdev_idx); ret = -EINVAL; @@ -964,7 +964,7 @@ static inline int ath11k_spectral_debug_register(struct ath11k *ar) 0600, ar->debug.debugfs_pdev, ar, &fops_scan_bins); - if (!ar->spectral.scan_bins) { + if (IS_ERR(ar->spectral.scan_bins)) { ath11k_warn(ar->ab, "failed to open debugfs in pdev %d\n", ar->pdev_idx); ret = -EINVAL;
Fix the incorrect return value check for debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL when it fails. Fixes: 9d11b7bff950 ("ath11k: add support for spectral scan") Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- drivers/net/wireless/ath/ath11k/spectral.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)