| Message ID | 20240418121116.22184-9-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Arnd Bergmann |
| Headers | show |
| Series | Turris Omnia MCU driver | expand |
On 4/18/24 15:11, Marek Behún wrote: > A few drivers register a devm action to remove a debugfs directory, > implementing a one-liner function that calls debufs_remove_recursive(). > Help drivers avoid this repeated implementations by adding managed > version of debugfs directory create function. > > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > include/linux/devm-helpers.h | 40 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h > index fe62b28baf03..58e0a7fbd549 100644 > --- a/include/linux/devm-helpers.h > +++ b/include/linux/devm-helpers.h > @@ -23,6 +23,7 @@ > * already ran. > */ > > +#include <linux/debugfs.h> With the same remark that I did for 06/11 Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> Yours, -- Matti
On Thu, Apr 18, 2024 at 02:11:13PM +0200, Marek Behún wrote: > A few drivers register a devm action to remove a debugfs directory, > implementing a one-liner function that calls debufs_remove_recursive(). > Help drivers avoid this repeated implementations by adding managed > version of debugfs directory create function. ... > +static inline struct dentry * > +devm_debugfs_create_dir(struct device *dev, const char *name, > + struct dentry *parent) > +{ > + struct dentry *dentry; > + int err; > + dentry = debugfs_create_dir(name, parent); > + if (IS_ERR(dentry)) > + return dentry; Why do we care? > + err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, > + dentry); > + if (err < 0) > + return ERR_PTR(err); > + > + return dentry; > +} static inline struct dentry * devm_debugfs_create_dir(struct device *dev, const char *name, struct dentry *parent) { struct dentry *dentry; int err; dentry = debugfs_create_dir(name, parent); err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, dentry); if (err) return ERR_PTR(err); return dentry; } ?
On Tue, 23 Apr 2024 19:02:21 +0300 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Thu, Apr 18, 2024 at 02:11:13PM +0200, Marek Behún wrote: > > A few drivers register a devm action to remove a debugfs directory, > > implementing a one-liner function that calls debufs_remove_recursive(). > > Help drivers avoid this repeated implementations by adding managed > > version of debugfs directory create function. > > ... > > > +static inline struct dentry * > > +devm_debugfs_create_dir(struct device *dev, const char *name, > > + struct dentry *parent) > > +{ > > + struct dentry *dentry; > > + int err; > > > + dentry = debugfs_create_dir(name, parent); > > + if (IS_ERR(dentry)) > > + return dentry; > > Why do we care? > > > + err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, > > + dentry); > > + if (err < 0) > > + return ERR_PTR(err); > > + > > + return dentry; > > +} > > static inline struct dentry * > devm_debugfs_create_dir(struct device *dev, const char *name, struct dentry *parent) > { > struct dentry *dentry; > int err; > > dentry = debugfs_create_dir(name, parent); > > err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, dentry); > if (err) > return ERR_PTR(err); But if the dir creation fails, there is no need to allocate devm destructor...
On Tue, Apr 23, 2024 at 07:02:21PM +0300, Andy Shevchenko wrote: > On Thu, Apr 18, 2024 at 02:11:13PM +0200, Marek Behún wrote: > > A few drivers register a devm action to remove a debugfs directory, > > implementing a one-liner function that calls debufs_remove_recursive(). > > Help drivers avoid this repeated implementations by adding managed > > version of debugfs directory create function. > > ... > > > +static inline struct dentry * > > +devm_debugfs_create_dir(struct device *dev, const char *name, > > + struct dentry *parent) > > +{ > > + struct dentry *dentry; > > + int err; > > > + dentry = debugfs_create_dir(name, parent); > > + if (IS_ERR(dentry)) > > + return dentry; > > Why do we care? We don't. I am missing some patches, but I suspect this breaks when CONFIG_DEBUGFS is disabled. Errors from debugfs functions are supposed to be ignored. regards, dan carpenter
On Tue, Apr 23, 2024 at 06:20:14PM +0200, Marek Behún wrote: > On Tue, 23 Apr 2024 19:02:21 +0300 > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Thu, Apr 18, 2024 at 02:11:13PM +0200, Marek Behún wrote: ... > > > +static inline struct dentry * > > > +devm_debugfs_create_dir(struct device *dev, const char *name, > > > + struct dentry *parent) > > > +{ > > > + struct dentry *dentry; > > > + int err; > > > > > + dentry = debugfs_create_dir(name, parent); > > > + if (IS_ERR(dentry)) > > > + return dentry; > > > > Why do we care? > > > > > + err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, > > > + dentry); > > > + if (err < 0) > > > + return ERR_PTR(err); > > > + > > > + return dentry; > > > +} > > > > static inline struct dentry * > > devm_debugfs_create_dir(struct device *dev, const char *name, struct dentry *parent) > > { > > struct dentry *dentry; > > int err; > > > > dentry = debugfs_create_dir(name, parent); > > > > err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, dentry); > > if (err) > > return ERR_PTR(err); > > But if the dir creation fails, there is no need to allocate devm > destructor... Yes, but why do we care about that? debugfs is designed the way that we should ignore errors from it. This discussion let me think, that probably the best solution is to add a field to struct device_driver to handle this. debugfs will appear only when device is completely instantiated, removed before anything, and a lot of other benefits (see the story behind dev_groups as a reference).
On Tue, 23 Apr 2024 19:33:20 +0300 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Tue, Apr 23, 2024 at 06:20:14PM +0200, Marek Behún wrote: > > On Tue, 23 Apr 2024 19:02:21 +0300 > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > On Thu, Apr 18, 2024 at 02:11:13PM +0200, Marek Behún wrote: > > ... > > > > > +static inline struct dentry * > > > > +devm_debugfs_create_dir(struct device *dev, const char *name, > > > > + struct dentry *parent) > > > > +{ > > > > + struct dentry *dentry; > > > > + int err; > > > > > > > + dentry = debugfs_create_dir(name, parent); > > > > + if (IS_ERR(dentry)) > > > > + return dentry; > > > > > > Why do we care? > > > > > > > + err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, > > > > + dentry); > > > > + if (err < 0) > > > > + return ERR_PTR(err); > > > > + > > > > + return dentry; > > > > +} > > > > > > static inline struct dentry * > > > devm_debugfs_create_dir(struct device *dev, const char *name, struct dentry *parent) > > > { > > > struct dentry *dentry; > > > int err; > > > > > > dentry = debugfs_create_dir(name, parent); > > > > > > err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, dentry); > > > if (err) > > > return ERR_PTR(err); > > > > But if the dir creation fails, there is no need to allocate devm > > destructor... > > Yes, but why do we care about that? > debugfs is designed the way that we should ignore errors from it. > > This discussion let me think, that probably the best solution is to add a field > to struct device_driver to handle this. > > debugfs will appear only when device is completely instantiated, removed before > anything, and a lot of other benefits (see the story behind dev_groups as a > reference). > Okay, this seems way beyond the scope of what I am trying to do here. Since Matti is also not entirely content with these additions to the devm-helpers.h header and you suggested to use gpiod_to_irq(), I shall drop these changes from the next version and just use devm_add_action_or_reset() in the driver directly. Marek
diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h index fe62b28baf03..58e0a7fbd549 100644 --- a/include/linux/devm-helpers.h +++ b/include/linux/devm-helpers.h @@ -23,6 +23,7 @@ * already ran. */ +#include <linux/debugfs.h> #include <linux/device.h> #include <linux/kconfig.h> #include <linux/irqdomain.h> @@ -130,4 +131,43 @@ static inline int devm_irq_create_mapping(struct device *dev, return virq; } +static inline void devm_debugfs_dir_recursive_drop(void *res) +{ + debugfs_remove_recursive(res); +} + +/** + * devm_debugfs_create_dir - Resource managed debugfs directory creation + * @dev: Device which lifetime the directory is bound to + * @name: a pointer to a string containing the name of the directory to + * create + * @parent: a pointer to the parent dentry for this file. This should be a + * directory dentry if set. If this parameter is NULL, then the + * directory will be created in the root of the debugfs filesystem. + * + * Create a debugfs directory which is automatically recursively removed when + * the driver is detached. A few drivers create debugfs directories which they + * want removed before driver is detached. + * devm_debugfs_create_dir() can be used to omit the explicit + * debugfs_remove_recursive() call when driver is detached. + */ +static inline struct dentry * +devm_debugfs_create_dir(struct device *dev, const char *name, + struct dentry *parent) +{ + struct dentry *dentry; + int err; + + dentry = debugfs_create_dir(name, parent); + if (IS_ERR(dentry)) + return dentry; + + err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, + dentry); + if (err < 0) + return ERR_PTR(err); + + return dentry; +} + #endif
A few drivers register a devm action to remove a debugfs directory, implementing a one-liner function that calls debufs_remove_recursive(). Help drivers avoid this repeated implementations by adding managed version of debugfs directory create function. Signed-off-by: Marek Behún <kabel@kernel.org> --- include/linux/devm-helpers.h | 40 ++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)