| Message ID | 20210203215403.290792-13-pobrn@protonmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | platform/x86: ideapad-laptop: cleanup, keyboard backlight and "always on USB charging" control support, reenable touchpad control | expand |
Hi Barnabás, On 2/3/21 10:55 PM, Barnabás Pőcze wrote: > debugfs_create_dir() may return an ERR_PTR(), > add a check to ideapad_debugfs_init() that > handles the case when that occurs. debugfs functions should not be error-checked: 1. They are for debugging so if they don't work it is not an issue (note your own error handling also just returns without propagating the error). 2. Subsequent debugfs calls taking the ERR-PTR will detect this and turn into no-op-s So I'm going to skip this patch while applying this series. Regards, Hans > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 1aa3a05c3360..ba0bd344f5ed 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -14,6 +14,7 @@ > #include <linux/debugfs.h> > #include <linux/device.h> > #include <linux/dmi.h> > +#include <linux/err.h> > #include <linux/fb.h> > #include <linux/i8042.h> > #include <linux/init.h> > @@ -350,9 +351,11 @@ DEFINE_SHOW_ATTRIBUTE(debugfs_cfg); > > static void ideapad_debugfs_init(struct ideapad_private *priv) > { > - struct dentry *dir; > + struct dentry *dir = debugfs_create_dir("ideapad", NULL); > + > + if (IS_ERR(dir)) > + return; > > - dir = debugfs_create_dir("ideapad", NULL); > priv->debug = dir; > > debugfs_create_file("cfg", S_IRUGO, dir, priv, &debugfs_cfg_fops); >
Hi 2021. február 4., csütörtök 9:12 keltezéssel, Hans de Goede írta: > Hi Barnabás, > > On 2/3/21 10:55 PM, Barnabás Pőcze wrote: > > > debugfs_create_dir() may return an ERR_PTR(), > > add a check to ideapad_debugfs_init() that > > handles the case when that occurs. > > debugfs functions should not be error-checked: > > 1. They are for debugging so if they don't work it is not an issue > (note your own error handling also just returns without propagating > the error). > > 2. Subsequent debugfs calls taking the ERR-PTR will detect this and > turn into no-op-s > [...] My idea was to prevent the ERR_PTR() from being passed to debugfs_create_file() because I was not aware that it checked for that, so thanks for catching it! Regards, Barnabás Pőcze
Hi, On 2/4/21 1:21 PM, Barnabás Pőcze wrote: > Hi > > > 2021. február 4., csütörtök 9:12 keltezéssel, Hans de Goede írta: > >> Hi Barnabás, >> >> On 2/3/21 10:55 PM, Barnabás Pőcze wrote: >> >>> debugfs_create_dir() may return an ERR_PTR(), >>> add a check to ideapad_debugfs_init() that >>> handles the case when that occurs. >> >> debugfs functions should not be error-checked: >> >> 1. They are for debugging so if they don't work it is not an issue >> (note your own error handling also just returns without propagating >> the error). >> >> 2. Subsequent debugfs calls taking the ERR-PTR will detect this and >> turn into no-op-s >> [...] > > My idea was to prevent the ERR_PTR() from being passed to > debugfs_create_file() because I was not aware that it checked > for that, so thanks for catching it! I understand, debugfs functions are special, because normally error checking return values is encouraged, but with debugfs functions there is an (unwritten?) rule that they should not be error checked. Regards, Hans
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index 1aa3a05c3360..ba0bd344f5ed 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -14,6 +14,7 @@ #include <linux/debugfs.h> #include <linux/device.h> #include <linux/dmi.h> +#include <linux/err.h> #include <linux/fb.h> #include <linux/i8042.h> #include <linux/init.h> @@ -350,9 +351,11 @@ DEFINE_SHOW_ATTRIBUTE(debugfs_cfg); static void ideapad_debugfs_init(struct ideapad_private *priv) { - struct dentry *dir; + struct dentry *dir = debugfs_create_dir("ideapad", NULL); + + if (IS_ERR(dir)) + return; - dir = debugfs_create_dir("ideapad", NULL); priv->debug = dir; debugfs_create_file("cfg", S_IRUGO, dir, priv, &debugfs_cfg_fops);
debugfs_create_dir() may return an ERR_PTR(), add a check to ideapad_debugfs_init() that handles the case when that occurs. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>