| Message ID | 20180529153107.12791-2-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 29, 2018 at 05:30:47PM +0200, Greg Kroah-Hartman wrote: > When calling debugfs functions, there is no need to ever check the > return value. The function can work or not, but the code logic should > never do something different based on this. > > Clean up the fsusb302 driver to not care about the dentry being created, > or if the root directory was created, as the code should work properly > either way. We do not need to save the dentry anymore as things are > properly cleaned up when we remove the root directory. > > Note, this driver seems to only work for one device per system, > otherwise the debugfs directories are going to get confused when a > device is removed. > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/usb/typec/fusb302/fusb302.c | 24 +++++++----------------- > 1 file changed, 7 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c > index 9c1eba9ea004..07b07ddf6af0 100644 > --- a/drivers/usb/typec/fusb302/fusb302.c > +++ b/drivers/usb/typec/fusb302/fusb302.c > @@ -117,7 +117,6 @@ struct fusb302_chip { > u32 snk_pdo[PDO_MAX_OBJECTS]; > > #ifdef CONFIG_DEBUG_FS > - struct dentry *dentry; > /* lock for log buffer access */ > struct mutex logbuffer_lock; > int logbuffer_head; > @@ -215,33 +214,26 @@ DEFINE_SHOW_ATTRIBUTE(fusb302_debug); > > static struct dentry *rootdir; > > -static int fusb302_debugfs_init(struct fusb302_chip *chip) > +static void fusb302_debugfs_init(struct fusb302_chip *chip) > { > mutex_init(&chip->logbuffer_lock); > - if (!rootdir) { > + if (!rootdir) > rootdir = debugfs_create_dir("fusb302", NULL); > - if (!rootdir) > - return -ENOMEM; > - } > - > - chip->dentry = debugfs_create_file(dev_name(chip->dev), > - S_IFREG | 0444, rootdir, > - chip, &fusb302_debug_fops); > > - return 0; > + debugfs_create_file(dev_name(chip->dev), S_IFREG | 0444, rootdir, chip, > + &fusb302_debug_fops); > } > > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > { > - debugfs_remove(chip->dentry); > - debugfs_remove(rootdir); > + debugfs_remove_recursive(rootdir); The idea was that debugfs_remove() would only remove the root directory if nothing else was in it. This was the reason for the preceding debugfs_remove(chip->dentry) followed by debugfs_remove(). Your patch changes this behavior. "... this driver seems to only work for one device per system" is introduced by your patch and was not previously the case. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 29, 2018 at 09:15:30AM -0700, Guenter Roeck wrote: > On Tue, May 29, 2018 at 05:30:47PM +0200, Greg Kroah-Hartman wrote: > > When calling debugfs functions, there is no need to ever check the > > return value. The function can work or not, but the code logic should > > never do something different based on this. > > > > Clean up the fsusb302 driver to not care about the dentry being created, > > or if the root directory was created, as the code should work properly > > either way. We do not need to save the dentry anymore as things are > > properly cleaned up when we remove the root directory. > > > > Note, this driver seems to only work for one device per system, > > otherwise the debugfs directories are going to get confused when a > > device is removed. > > > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Cc: Guenter Roeck <linux@roeck-us.net> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > drivers/usb/typec/fusb302/fusb302.c | 24 +++++++----------------- > > 1 file changed, 7 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c > > index 9c1eba9ea004..07b07ddf6af0 100644 > > --- a/drivers/usb/typec/fusb302/fusb302.c > > +++ b/drivers/usb/typec/fusb302/fusb302.c > > @@ -117,7 +117,6 @@ struct fusb302_chip { > > u32 snk_pdo[PDO_MAX_OBJECTS]; > > > > #ifdef CONFIG_DEBUG_FS > > - struct dentry *dentry; > > /* lock for log buffer access */ > > struct mutex logbuffer_lock; > > int logbuffer_head; > > @@ -215,33 +214,26 @@ DEFINE_SHOW_ATTRIBUTE(fusb302_debug); > > > > static struct dentry *rootdir; > > > > -static int fusb302_debugfs_init(struct fusb302_chip *chip) > > +static void fusb302_debugfs_init(struct fusb302_chip *chip) > > { > > mutex_init(&chip->logbuffer_lock); > > - if (!rootdir) { > > + if (!rootdir) > > rootdir = debugfs_create_dir("fusb302", NULL); > > - if (!rootdir) > > - return -ENOMEM; > > - } > > - > > - chip->dentry = debugfs_create_file(dev_name(chip->dev), > > - S_IFREG | 0444, rootdir, > > - chip, &fusb302_debug_fops); > > > > - return 0; > > + debugfs_create_file(dev_name(chip->dev), S_IFREG | 0444, rootdir, chip, > > + &fusb302_debug_fops); > > } > > > > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > > { > > - debugfs_remove(chip->dentry); > > - debugfs_remove(rootdir); > > + debugfs_remove_recursive(rootdir); > > The idea was that debugfs_remove() would only remove the root directory > if nothing else was in it. This was the reason for the preceding > debugfs_remove(chip->dentry) followed by debugfs_remove(). Your patch > changes this behavior. "... this driver seems to only work for one > device per system" is introduced by your patch and was not previously > the case. Ah, that makes more sense. Nice hack :) Why not just have a new directory per device like "most" drivers have? That would make this a bit simpler to manage. Mind of I change the code that way? Hm, you still end up having to save off a dentry that way, ah, it's the same either way... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 29, 2018 at 06:27:03PM +0200, Greg Kroah-Hartman wrote: > On Tue, May 29, 2018 at 09:15:30AM -0700, Guenter Roeck wrote: > > On Tue, May 29, 2018 at 05:30:47PM +0200, Greg Kroah-Hartman wrote: > > > When calling debugfs functions, there is no need to ever check the > > > return value. The function can work or not, but the code logic should > > > never do something different based on this. > > > > > > Clean up the fsusb302 driver to not care about the dentry being created, > > > or if the root directory was created, as the code should work properly > > > either way. We do not need to save the dentry anymore as things are > > > properly cleaned up when we remove the root directory. > > > > > > Note, this driver seems to only work for one device per system, > > > otherwise the debugfs directories are going to get confused when a > > > device is removed. > > > > > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Cc: Guenter Roeck <linux@roeck-us.net> > > > Cc: Hans de Goede <hdegoede@redhat.com> > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > --- > > > drivers/usb/typec/fusb302/fusb302.c | 24 +++++++----------------- > > > 1 file changed, 7 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c > > > index 9c1eba9ea004..07b07ddf6af0 100644 > > > --- a/drivers/usb/typec/fusb302/fusb302.c > > > +++ b/drivers/usb/typec/fusb302/fusb302.c > > > @@ -117,7 +117,6 @@ struct fusb302_chip { > > > u32 snk_pdo[PDO_MAX_OBJECTS]; > > > > > > #ifdef CONFIG_DEBUG_FS > > > - struct dentry *dentry; > > > /* lock for log buffer access */ > > > struct mutex logbuffer_lock; > > > int logbuffer_head; > > > @@ -215,33 +214,26 @@ DEFINE_SHOW_ATTRIBUTE(fusb302_debug); > > > > > > static struct dentry *rootdir; > > > > > > -static int fusb302_debugfs_init(struct fusb302_chip *chip) > > > +static void fusb302_debugfs_init(struct fusb302_chip *chip) > > > { > > > mutex_init(&chip->logbuffer_lock); > > > - if (!rootdir) { > > > + if (!rootdir) > > > rootdir = debugfs_create_dir("fusb302", NULL); > > > - if (!rootdir) > > > - return -ENOMEM; > > > - } > > > - > > > - chip->dentry = debugfs_create_file(dev_name(chip->dev), > > > - S_IFREG | 0444, rootdir, > > > - chip, &fusb302_debug_fops); > > > > > > - return 0; > > > + debugfs_create_file(dev_name(chip->dev), S_IFREG | 0444, rootdir, chip, > > > + &fusb302_debug_fops); > > > } > > > > > > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > > > { > > > - debugfs_remove(chip->dentry); > > > - debugfs_remove(rootdir); > > > + debugfs_remove_recursive(rootdir); > > > > The idea was that debugfs_remove() would only remove the root directory > > if nothing else was in it. This was the reason for the preceding > > debugfs_remove(chip->dentry) followed by debugfs_remove(). Your patch > > changes this behavior. "... this driver seems to only work for one > > device per system" is introduced by your patch and was not previously > > the case. > > Ah, that makes more sense. Nice hack :) > > Why not just have a new directory per device like "most" drivers have? > That would make this a bit simpler to manage. Mind of I change the code > that way? Hm, you still end up having to save off a dentry that way, > ah, it's the same either way... > I personally prefer some kind of grouping, such as done for regulators. Of course, that is easier if there is a subsystem behind it. No strong opinion either way. And, yes, you'll still need to store dentry somewhere to be able to remove it. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c index 9c1eba9ea004..07b07ddf6af0 100644 --- a/drivers/usb/typec/fusb302/fusb302.c +++ b/drivers/usb/typec/fusb302/fusb302.c @@ -117,7 +117,6 @@ struct fusb302_chip { u32 snk_pdo[PDO_MAX_OBJECTS]; #ifdef CONFIG_DEBUG_FS - struct dentry *dentry; /* lock for log buffer access */ struct mutex logbuffer_lock; int logbuffer_head; @@ -215,33 +214,26 @@ DEFINE_SHOW_ATTRIBUTE(fusb302_debug); static struct dentry *rootdir; -static int fusb302_debugfs_init(struct fusb302_chip *chip) +static void fusb302_debugfs_init(struct fusb302_chip *chip) { mutex_init(&chip->logbuffer_lock); - if (!rootdir) { + if (!rootdir) rootdir = debugfs_create_dir("fusb302", NULL); - if (!rootdir) - return -ENOMEM; - } - - chip->dentry = debugfs_create_file(dev_name(chip->dev), - S_IFREG | 0444, rootdir, - chip, &fusb302_debug_fops); - return 0; + debugfs_create_file(dev_name(chip->dev), S_IFREG | 0444, rootdir, chip, + &fusb302_debug_fops); } static void fusb302_debugfs_exit(struct fusb302_chip *chip) { - debugfs_remove(chip->dentry); - debugfs_remove(rootdir); + debugfs_remove_recursive(rootdir); } #else static void fusb302_log(const struct fusb302_chip *chip, const char *fmt, ...) { } -static int fusb302_debugfs_init(const struct fusb302_chip *chip) { return 0; } +static void fusb302_debugfs_init(const struct fusb302_chip *chip) { } static void fusb302_debugfs_exit(const struct fusb302_chip *chip) { } #endif @@ -1773,9 +1765,7 @@ static int fusb302_probe(struct i2c_client *client, return -EPROBE_DEFER; } - ret = fusb302_debugfs_init(chip); - if (ret < 0) - return ret; + fusb302_debugfs_init(chip); chip->wq = create_singlethread_workqueue(dev_name(chip->dev)); if (!chip->wq) {
When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this. Clean up the fsusb302 driver to not care about the dentry being created, or if the root directory was created, as the code should work properly either way. We do not need to save the dentry anymore as things are properly cleaned up when we remove the root directory. Note, this driver seems to only work for one device per system, otherwise the debugfs directories are going to get confused when a device is removed. Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/usb/typec/fusb302/fusb302.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)