| Message ID | 20230712124259.15096-1-duminjie@vivo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v1] drivers: wireless: ath5k: fix parameter check in ath5k_debug_init_device | expand |
On Wed, Jul 12, 2023 at 08:42:59PM +0800, Minjie Du wrote: > Make IS_ERR() judge the debugfs_create_dir() function return > in ath5k_debug_init_device(). > > Signed-off-by: Minjie Du <duminjie@vivo.com> > --- > drivers/net/wireless/ath/ath5k/debug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c > index 4b41160e5..08058b3f7 100644 > --- a/drivers/net/wireless/ath/ath5k/debug.c > +++ b/drivers/net/wireless/ath/ath5k/debug.c > @@ -982,7 +982,7 @@ ath5k_debug_init_device(struct ath5k_hw *ah) > ah->debug.level = ath5k_debug; > > phydir = debugfs_create_dir("ath5k", ah->hw->wiphy->debugfsdir); > - if (!phydir) > + if (IS_ERR(phydir)) Please use IS_ERR_OR_NULL() instead. With that change added: Acked-by: Luis Chamberlain <mcgrof@kernel.org> Luis
On 12. 07. 23, 21:07, Markus Elfring wrote: >>> +++ b/drivers/net/wireless/ath/ath5k/debug.c >>> @@ -982,7 +982,7 @@ ath5k_debug_init_device(struct ath5k_hw *ah) >>> ah->debug.level = ath5k_debug; >>> >>> phydir = debugfs_create_dir("ath5k", ah->hw->wiphy->debugfsdir); >> - if (!phydir) >>> + if (IS_ERR(phydir)) >> >> Please use IS_ERR_OR_NULL() instead. > > I find your support for consistent error detection interesting here. > Contributors for other Linux software components provide different > development views. IS_ERR_OR_NULL() is almost never correct. It is used exceptionally in cases where one needs to distinct three states. It's not the case for sysfs/debugfs interfaces. > Would a subject like “[PATCH v2] ath5k: Fix an error check in ath5k_debug_init_device()” > be more appropriate? Yes (pointing out specifically "()" at the end). And add the "why" part to the message log too, please. thanks,
subject prefix should be "wifi: ath5k: " On 7/12/2023 5:42 AM, Minjie Du wrote: > Make IS_ERR() judge the debugfs_create_dir() function return > in ath5k_debug_init_device(). > > Signed-off-by: Minjie Du <duminjie@vivo.com> > --- > drivers/net/wireless/ath/ath5k/debug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c > index 4b41160e5..08058b3f7 100644 > --- a/drivers/net/wireless/ath/ath5k/debug.c > +++ b/drivers/net/wireless/ath/ath5k/debug.c > @@ -982,7 +982,7 @@ ath5k_debug_init_device(struct ath5k_hw *ah) > ah->debug.level = ath5k_debug; > > phydir = debugfs_create_dir("ath5k", ah->hw->wiphy->debugfsdir); > - if (!phydir) > + if (IS_ERR(phydir)) > return; > > debugfs_create_file("debug", 0600, phydir, ah, &fops_debug);
On Thu, Jul 13, 2023 at 07:47:53AM +0200, Jiri Slaby wrote: > On 12. 07. 23, 21:07, Markus Elfring wrote: > > > > +++ b/drivers/net/wireless/ath/ath5k/debug.c > > > > @@ -982,7 +982,7 @@ ath5k_debug_init_device(struct ath5k_hw *ah) > > > > ah->debug.level = ath5k_debug; > > > > > > > > phydir = debugfs_create_dir("ath5k", ah->hw->wiphy->debugfsdir); > > > - if (!phydir) > > > > + if (IS_ERR(phydir)) > > > > > > Please use IS_ERR_OR_NULL() instead. The correct thing for debugfs is to delete the error handling entirely. Checking for NULL is harmless but checking for IS_ERR() will break the driver if debugfs is turned off. Debugfs is a special case where the error handling was deliberately written so that it's basically impossible to do it correctly. Because as I said at the start, the correct thing is to delete it. regards, dan carpenter
diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c index 4b41160e5..08058b3f7 100644 --- a/drivers/net/wireless/ath/ath5k/debug.c +++ b/drivers/net/wireless/ath/ath5k/debug.c @@ -982,7 +982,7 @@ ath5k_debug_init_device(struct ath5k_hw *ah) ah->debug.level = ath5k_debug; phydir = debugfs_create_dir("ath5k", ah->hw->wiphy->debugfsdir); - if (!phydir) + if (IS_ERR(phydir)) return; debugfs_create_file("debug", 0600, phydir, ah, &fops_debug);
Make IS_ERR() judge the debugfs_create_dir() function return in ath5k_debug_init_device(). Signed-off-by: Minjie Du <duminjie@vivo.com> --- drivers/net/wireless/ath/ath5k/debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)