| Message ID | 20240323164359.21642-1-kabel@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Turris Omnia MCU driver | expand |
Le 23/03/2024 à 17:43, Marek Behún a écrit : > 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. > > Use the new function devm_debugfs_create_dir() in the following > drivers: > drivers/crypto/caam/ctrl.c > drivers/gpu/drm/bridge/ti-sn65dsi86.c > drivers/hwmon/hp-wmi-sensors.c > drivers/hwmon/mr75203.c > drivers/hwmon/pmbus/pmbus_core.c > > Also use the action function devm_debugfs_dir_recursive_drop() in > driver > drivers/gpio/gpio-mockup.c > > As per Dan Williams' request [1], do not touch the driver > drivers/cxl/mem.c > > [1] https://lore.kernel.org/linux-gpio/65d7918b358a5_1ee3129432@dwillia2-mobl3.amr.corp.intel.com.notmuch/ > > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > drivers/crypto/caam/ctrl.c | 16 +++-------- > drivers/gpio/gpio-mockup.c | 11 ++------ > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 ++------- > drivers/hwmon/hp-wmi-sensors.c | 15 ++-------- > drivers/hwmon/mr75203.c | 15 ++++------ > drivers/hwmon/pmbus/pmbus_core.c | 16 ++++------- > include/linux/devm-helpers.h | 40 +++++++++++++++++++++++++++ > 7 files changed, 61 insertions(+), 65 deletions(-) ... > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 455eecf6380e..adbe0fe09490 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -12,6 +12,7 @@ > #include <linux/cleanup.h> > #include <linux/debugfs.h> > #include <linux/device.h> > +#include <linux/devm-helpers.h> > #include <linux/gpio/driver.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > @@ -390,13 +391,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev, > } > } > > -static void gpio_mockup_debugfs_cleanup(void *data) > -{ > - struct gpio_mockup_chip *chip = data; > - > - debugfs_remove_recursive(chip->dbg_dir); > -} > - > static void gpio_mockup_dispose_mappings(void *data) > { > struct gpio_mockup_chip *chip = data; > @@ -480,7 +474,8 @@ static int gpio_mockup_probe(struct platform_device *pdev) > > gpio_mockup_debugfs_setup(dev, chip); > > - return devm_add_action_or_reset(dev, gpio_mockup_debugfs_cleanup, chip); > + return devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, > + chip->dbg_dir); This look strange. Shouldn't the debugfs_create_dir() call in gpio_mockup_debugfs_setup() be changed, instead? (I've not look in the previous version if something was said about it, so apologies if already discussed) > } > > static const struct of_device_id gpio_mockup_of_match[] = { ... > diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c > index 50a8b9c3f94d..50f348fca108 100644 > --- a/drivers/hwmon/mr75203.c > +++ b/drivers/hwmon/mr75203.c > @@ -10,6 +10,7 @@ > #include <linux/bits.h> > #include <linux/clk.h> > #include <linux/debugfs.h> > +#include <linux/devm-helpers.h> > #include <linux/hwmon.h> > #include <linux/kstrtox.h> > #include <linux/module.h> > @@ -216,17 +217,11 @@ static const struct file_operations pvt_ts_coeff_j_fops = { > .llseek = default_llseek, > }; > > -static void devm_pvt_ts_dbgfs_remove(void *data) > -{ > - struct pvt_device *pvt = (struct pvt_device *)data; > - > - debugfs_remove_recursive(pvt->dbgfs_dir); > - pvt->dbgfs_dir = NULL; > -} > - > static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > { > - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); > + if (IS_ERR(pvt->dbgfs_dir)) > + return PTR_ERR(pvt->dbgfs_dir); Not sure if the test and error handling should be added here. *If I'm correct*, functions related to debugfs already handle this case and just do nothing. And failure in debugfs related code is not considered as something that need to be reported and abort a probe function. Maybe the same other (already existing) tests in this patch should be removed as well, in a separated patch. just my 2c CJ > > debugfs_create_u32("ts_coeff_h", 0644, pvt->dbgfs_dir, > &pvt->ts_coeff.h); > @@ -237,7 +232,7 @@ static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt, > &pvt_ts_coeff_j_fops); > > - return devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt); > + return 0; > } ...
On Sat, 23 Mar 2024 22:10:40 +0100 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > - return devm_add_action_or_reset(dev, gpio_mockup_debugfs_cleanup, chip); > > + return devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop, > > + chip->dbg_dir); > > This look strange. Shouldn't the debugfs_create_dir() call in > gpio_mockup_debugfs_setup() be changed, instead? > > (I've not look in the previous version if something was said about it, > so apologies if already discussed) Yeah, this specific user needs a more complicated refactoring. I was reluctant to do more complicated refactors in the patch that also introduces these helpers. But Guenter asked me to split the patch anyway... > > static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > > { > > - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > > + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); > > + if (IS_ERR(pvt->dbgfs_dir)) > > + return PTR_ERR(pvt->dbgfs_dir); > > Not sure if the test and error handling should be added here. > *If I'm correct*, functions related to debugfs already handle this case > and just do nothing. And failure in debugfs related code is not > considered as something that need to be reported and abort a probe function. > > Maybe the same other (already existing) tests in this patch should be > removed as well, in a separated patch. Functions related to debugfs maybe do, but devm_ resource management functions may fail to allocate release structure, and those errors need to be handled, AFAIK. Marek
Le 23/03/2024 à 22:25, Marek Behún a écrit : > On Sat, 23 Mar 2024 22:10:40 +0100 > Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > ... >>> static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) >>> { >>> - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); >>> + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); >>> + if (IS_ERR(pvt->dbgfs_dir)) >>> + return PTR_ERR(pvt->dbgfs_dir); >> >> Not sure if the test and error handling should be added here. >> *If I'm correct*, functions related to debugfs already handle this case >> and just do nothing. And failure in debugfs related code is not >> considered as something that need to be reported and abort a probe function. >> >> Maybe the same other (already existing) tests in this patch should be >> removed as well, in a separated patch. > > Functions related to debugfs maybe do, but devm_ resource management > functions may fail to allocate release structure, and those errors need > to be handled, AFAIK. I would say no. If this memory allocation fails, then debugfs_create_dir() will not be called, but that's not a really big deal if the driver itself can still run normally without it. Up to you to leave it as-is or remove what I think is a useless error handling. At least, maybe it could be said in the commit log, so that maintainers can comment on it, if they don't spot the error handling you introduce. CJ > > Marek >
On Sun, 24 Mar 2024 10:21:28 +0100 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > Le 23/03/2024 à 22:25, Marek Behún a écrit : > > On Sat, 23 Mar 2024 22:10:40 +0100 > > Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > > > ... > > >>> static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > >>> { > >>> - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > >>> + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); > >>> + if (IS_ERR(pvt->dbgfs_dir)) > >>> + return PTR_ERR(pvt->dbgfs_dir); > >> > >> Not sure if the test and error handling should be added here. > >> *If I'm correct*, functions related to debugfs already handle this case > >> and just do nothing. And failure in debugfs related code is not > >> considered as something that need to be reported and abort a probe function. > >> > >> Maybe the same other (already existing) tests in this patch should be > >> removed as well, in a separated patch. > > > > Functions related to debugfs maybe do, but devm_ resource management > > functions may fail to allocate release structure, and those errors need > > to be handled, AFAIK. > > I would say no. > If this memory allocation fails, then debugfs_create_dir() will not be > called, but that's not a really big deal if the driver itself can still > run normally without it. debugfs_create_dir() will always be called. Resource allocation is done afterwards, and if it fails, then the created dir will be removed. But now I don't know what to do, because yes, it seems that the debugfs errors are being ignored at many places... > > Up to you to leave it as-is or remove what I think is a useless error > handling. > At least, maybe it could be said in the commit log, so that maintainers > can comment on it, if they don't spot the error handling you introduce. > > CJ > > > > > Marek > > >
On Sat, Mar 23, 2024 at 10:10:40PM +0100, Christophe JAILLET wrote: > > static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev) > > { > > - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > > + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL); > > + if (IS_ERR(pvt->dbgfs_dir)) > > + return PTR_ERR(pvt->dbgfs_dir); > > Not sure if the test and error handling should be added here. Yep. debugfs_create_dir() is not supposed to be checked here. It breaks the driver if CONFIG_DEBUGFS is disabled. I have written a blog about this: https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/ regards, dan carpenter
Hello Andy, Linus, Arnd, Gregory, and others, I am sending v5 of the series adding Turris Omnia MCU driver. See the cover letters for v1, v2, v3 and v4: https://patchwork.kernel.org/project/linux-soc/cover/20230823161012.6986-1-kabel@kernel.org/ https://patchwork.kernel.org/project/linux-soc/cover/20230919103815.16818-1-kabel@kernel.org/ https://patchwork.kernel.org/project/linux-soc/cover/20231023143130.11602-1-kabel@kernel.org/ https://patchwork.kernel.org/project/linux-soc/cover/20231026161803.16750-1-kabel@kernel.org/ Changes since v4: - added new patches 06/11 devm-helpers: Add resource managed version of irq_create_mapping() 07/11 platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG 08/11 devm-helpers: Add resource managed version of debugfs directory create function 09/11 platform: cznic: turris-omnia-mcu: Add support for digital message signing via debugfs - for changes specific to patches which were also sent in previous versions see the notes in those patches Marek Behún (11): dt-bindings: arm: add cznic,turris-omnia-mcu binding platform: cznic: Add preliminary support for Turris Omnia MCU platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup platform: cznic: turris-omnia-mcu: Add support for MCU watchdog devm-helpers: Add resource managed version of irq_create_mapping() platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG devm-helpers: Add resource managed version of debugfs directory create function platform: cznic: turris-omnia-mcu: Add support for digital message signing via debugfs ARM: dts: turris-omnia: Add MCU system-controller node ARM: dts: turris-omnia: Add GPIO key node for front button .../ABI/testing/debugfs-turris-omnia-mcu | 13 + .../sysfs-bus-i2c-devices-turris-omnia-mcu | 126 ++ .../bindings/arm/cznic,turris-omnia-mcu.yaml | 86 ++ MAINTAINERS | 5 + .../dts/marvell/armada-385-turris-omnia.dts | 35 +- drivers/crypto/caam/ctrl.c | 16 +- drivers/crypto/caam/jr.c | 8 +- drivers/gpio/gpio-mockup.c | 11 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 +- drivers/hwmon/hp-wmi-sensors.c | 15 +- drivers/hwmon/mr75203.c | 15 +- drivers/hwmon/pmbus/pmbus_core.c | 16 +- drivers/platform/Kconfig | 2 + drivers/platform/Makefile | 1 + drivers/platform/cznic/Kconfig | 51 + drivers/platform/cznic/Makefile | 10 + .../platform/cznic/turris-omnia-mcu-base.c | 406 +++++++ .../platform/cznic/turris-omnia-mcu-debugfs.c | 216 ++++ .../platform/cznic/turris-omnia-mcu-gpio.c | 1056 +++++++++++++++++ .../cznic/turris-omnia-mcu-sys-off-wakeup.c | 258 ++++ .../platform/cznic/turris-omnia-mcu-trng.c | 89 ++ .../cznic/turris-omnia-mcu-watchdog.c | 122 ++ drivers/platform/cznic/turris-omnia-mcu.h | 214 ++++ include/linux/devm-helpers.h | 94 ++ include/linux/turris-omnia-mcu-interface.h | 238 ++++ 25 files changed, 3044 insertions(+), 72 deletions(-) create mode 100644 Documentation/ABI/testing/debugfs-turris-omnia-mcu create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu create mode 100644 Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml create mode 100644 drivers/platform/cznic/Kconfig create mode 100644 drivers/platform/cznic/Makefile create mode 100644 drivers/platform/cznic/turris-omnia-mcu-base.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-debugfs.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu-watchdog.c create mode 100644 drivers/platform/cznic/turris-omnia-mcu.h create mode 100644 include/linux/turris-omnia-mcu-interface.h