linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
@ 2024-02-22 14:58 Marek Behún
  2024-02-22 14:58 ` [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function Marek Behún
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Marek Behún @ 2024-02-22 14:58 UTC (permalink / raw)
  To: linux-kernel, Hans de Goede, Matti Vaittinen
  Cc: Marek Behún, Linus Walleij, Bartosz Golaszewski,
	Lucas De Marchi, Oded Gabbay, Thomas Hellström,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Aleksandr Mezin, Jean Delvare,
	Guenter Roeck, Pavel Machek, Lee Jones, Sebastian Reichel,
	Matthias Brugger, AngeloGioacchino Del Regno, linux-gpio,
	intel-xe, dri-devel, linux-hwmon, linux-leds, linux-pm,
	linux-arm-kernel, linux-mediatek

A few drivers are doing resource-managed mutex initialization by
implementing ad-hoc one-liner mutex dropping functions and using them
with devm_add_action_or_reset(). Help drivers avoid these repeated
one-liners by adding managed version of mutex initialization.

Use the new function devm_mutex_init() in the following drivers:
  drivers/gpio/gpio-pisosr.c
  drivers/gpio/gpio-sim.c
  drivers/gpu/drm/xe/xe_hwmon.c
  drivers/hwmon/nzxt-smart2.c
  drivers/leds/leds-is31fl319x.c
  drivers/power/supply/mt6370-charger.c
  drivers/power/supply/rt9467-charger.c

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/gpio/gpio-pisosr.c            |  9 ++-----
 drivers/gpio/gpio-sim.c               | 12 ++--------
 drivers/gpu/drm/xe/xe_hwmon.c         | 11 ++-------
 drivers/hwmon/nzxt-smart2.c           |  9 ++-----
 drivers/leds/leds-is31fl319x.c        |  9 ++-----
 drivers/power/supply/mt6370-charger.c | 11 +--------
 drivers/power/supply/rt9467-charger.c | 34 ++++-----------------------
 include/linux/devm-helpers.h          | 32 +++++++++++++++++++++++++
 8 files changed, 47 insertions(+), 80 deletions(-)

diff --git a/drivers/gpio/gpio-pisosr.c b/drivers/gpio/gpio-pisosr.c
index e3013e778e15..dddbf37e855f 100644
--- a/drivers/gpio/gpio-pisosr.c
+++ b/drivers/gpio/gpio-pisosr.c
@@ -7,6 +7,7 @@
 #include <linux/bitmap.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/devm-helpers.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
@@ -116,11 +117,6 @@ static const struct gpio_chip template_chip = {
 	.can_sleep		= true,
 };
 
-static void pisosr_mutex_destroy(void *lock)
-{
-	mutex_destroy(lock);
-}
-
 static int pisosr_gpio_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
@@ -147,8 +143,7 @@ static int pisosr_gpio_probe(struct spi_device *spi)
 		return dev_err_probe(dev, PTR_ERR(gpio->load_gpio),
 				     "Unable to allocate load GPIO\n");
 
-	mutex_init(&gpio->lock);
-	ret = devm_add_action_or_reset(dev, pisosr_mutex_destroy, &gpio->lock);
+	ret = devm_mutex_init(dev, &gpio->lock);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index c4106e37e6db..fcfcaa4efe70 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -12,6 +12,7 @@
 #include <linux/completion.h>
 #include <linux/configfs.h>
 #include <linux/device.h>
+#include <linux/devm-helpers.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
@@ -307,13 +308,6 @@ static ssize_t gpio_sim_sysfs_pull_store(struct device *dev,
 	return len;
 }
 
-static void gpio_sim_mutex_destroy(void *data)
-{
-	struct mutex *lock = data;
-
-	mutex_destroy(lock);
-}
-
 static void gpio_sim_put_device(void *data)
 {
 	struct device *dev = data;
@@ -457,9 +451,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 	if (ret)
 		return ret;
 
-	mutex_init(&chip->lock);
-	ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,
-				       &chip->lock);
+	ret = devm_mutex_init(dev, &chip->lock);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index 174ed2185481..bb88ae1c196c 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -3,6 +3,7 @@
  * Copyright © 2023 Intel Corporation
  */
 
+#include <linux/devm-helpers.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/hwmon.h>
 #include <linux/types.h>
@@ -729,13 +730,6 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
 		xe_hwmon_energy_get(hwmon, &energy);
 }
 
-static void xe_hwmon_mutex_destroy(void *arg)
-{
-	struct xe_hwmon *hwmon = arg;
-
-	mutex_destroy(&hwmon->hwmon_lock);
-}
-
 void xe_hwmon_register(struct xe_device *xe)
 {
 	struct device *dev = xe->drm.dev;
@@ -751,8 +745,7 @@ void xe_hwmon_register(struct xe_device *xe)
 
 	xe->hwmon = hwmon;
 
-	mutex_init(&hwmon->hwmon_lock);
-	if (devm_add_action_or_reset(dev, xe_hwmon_mutex_destroy, hwmon))
+	if (devm_mutex_init(dev, &hwmon->hwmon_lock))
 		return;
 
 	/* primary GT to access device level properties */
diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
index 7aa586eb74be..00bc89607673 100644
--- a/drivers/hwmon/nzxt-smart2.c
+++ b/drivers/hwmon/nzxt-smart2.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2021 Aleksandr Mezin
  */
 
+#include <linux/devm-helpers.h>
 #include <linux/hid.h>
 #include <linux/hwmon.h>
 #include <linux/math.h>
@@ -721,11 +722,6 @@ static int __maybe_unused nzxt_smart2_hid_reset_resume(struct hid_device *hdev)
 	return init_device(drvdata, drvdata->update_interval);
 }
 
-static void mutex_fini(void *lock)
-{
-	mutex_destroy(lock);
-}
-
 static int nzxt_smart2_hid_probe(struct hid_device *hdev,
 				 const struct hid_device_id *id)
 {
@@ -741,8 +737,7 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
 
 	init_waitqueue_head(&drvdata->wq);
 
-	mutex_init(&drvdata->mutex);
-	ret = devm_add_action_or_reset(&hdev->dev, mutex_fini, &drvdata->mutex);
+	ret = devm_mutex_init(&hdev->dev, &drvdata->mutex);
 	if (ret)
 		return ret;
 
diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
index 66c65741202e..e9d7cf6a386c 100644
--- a/drivers/leds/leds-is31fl319x.c
+++ b/drivers/leds/leds-is31fl319x.c
@@ -8,6 +8,7 @@
  * effect LEDs.
  */
 
+#include <linux/devm-helpers.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
@@ -495,11 +496,6 @@ static inline int is31fl3196_db_to_gain(u32 dezibel)
 	return dezibel / IS31FL3196_AUDIO_GAIN_DB_STEP;
 }
 
-static void is31f1319x_mutex_destroy(void *lock)
-{
-	mutex_destroy(lock);
-}
-
 static int is31fl319x_probe(struct i2c_client *client)
 {
 	struct is31fl319x_chip *is31;
@@ -515,8 +511,7 @@ static int is31fl319x_probe(struct i2c_client *client)
 	if (!is31)
 		return -ENOMEM;
 
-	mutex_init(&is31->lock);
-	err = devm_add_action_or_reset(dev, is31f1319x_mutex_destroy, &is31->lock);
+	err = devm_mutex_init(dev, &is31->lock);
 	if (err)
 		return err;
 
diff --git a/drivers/power/supply/mt6370-charger.c b/drivers/power/supply/mt6370-charger.c
index e24fce087d80..fa0517d0352d 100644
--- a/drivers/power/supply/mt6370-charger.c
+++ b/drivers/power/supply/mt6370-charger.c
@@ -766,13 +766,6 @@ static int mt6370_chg_init_psy(struct mt6370_priv *priv)
 	return PTR_ERR_OR_ZERO(priv->psy);
 }
 
-static void mt6370_chg_destroy_attach_lock(void *data)
-{
-	struct mutex *attach_lock = data;
-
-	mutex_destroy(attach_lock);
-}
-
 static void mt6370_chg_destroy_wq(void *data)
 {
 	struct workqueue_struct *wq = data;
@@ -900,9 +893,7 @@ static int mt6370_chg_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init psy\n");
 
-	mutex_init(&priv->attach_lock);
-	ret = devm_add_action_or_reset(dev, mt6370_chg_destroy_attach_lock,
-				       &priv->attach_lock);
+	ret = devm_mutex_init(dev, &priv->attach_lock);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init attach lock\n");
 
diff --git a/drivers/power/supply/rt9467-charger.c b/drivers/power/supply/rt9467-charger.c
index fdfdc83ab045..84f07c22077f 100644
--- a/drivers/power/supply/rt9467-charger.c
+++ b/drivers/power/supply/rt9467-charger.c
@@ -10,6 +10,7 @@
 #include <linux/bitfield.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/devm-helpers.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -1149,27 +1150,6 @@ static int rt9467_reset_chip(struct rt9467_chg_data *data)
 	return regmap_field_write(data->rm_field[F_RST], 1);
 }
 
-static void rt9467_chg_destroy_adc_lock(void *data)
-{
-	struct mutex *adc_lock = data;
-
-	mutex_destroy(adc_lock);
-}
-
-static void rt9467_chg_destroy_attach_lock(void *data)
-{
-	struct mutex *attach_lock = data;
-
-	mutex_destroy(attach_lock);
-}
-
-static void rt9467_chg_destroy_ichg_ieoc_lock(void *data)
-{
-	struct mutex *ichg_ieoc_lock = data;
-
-	mutex_destroy(ichg_ieoc_lock);
-}
-
 static void rt9467_chg_complete_aicl_done(void *data)
 {
 	struct completion *aicl_done = data;
@@ -1222,21 +1202,15 @@ static int rt9467_charger_probe(struct i2c_client *i2c)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to add irq chip\n");
 
-	mutex_init(&data->adc_lock);
-	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_adc_lock,
-				       &data->adc_lock);
+	ret = devm_mutex_init(dev, &data->adc_lock);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init ADC lock\n");
 
-	mutex_init(&data->attach_lock);
-	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_attach_lock,
-				       &data->attach_lock);
+	ret = devm_mutex_init(dev, &data->attach_lock);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init attach lock\n");
 
-	mutex_init(&data->ichg_ieoc_lock);
-	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_ichg_ieoc_lock,
-				       &data->ichg_ieoc_lock);
+	ret = devm_mutex_init(dev, &data->ichg_ieoc_lock);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init ICHG/IEOC lock\n");
 
diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
index 74891802200d..70640fb96117 100644
--- a/include/linux/devm-helpers.h
+++ b/include/linux/devm-helpers.h
@@ -24,6 +24,8 @@
  */
 
 #include <linux/device.h>
+#include <linux/kconfig.h>
+#include <linux/mutex.h>
 #include <linux/workqueue.h>
 
 static inline void devm_delayed_work_drop(void *res)
@@ -76,4 +78,34 @@ static inline int devm_work_autocancel(struct device *dev,
 	return devm_add_action(dev, devm_work_drop, w);
 }
 
+static inline void devm_mutex_drop(void *res)
+{
+	mutex_destroy(res);
+}
+
+/**
+ * devm_mutex_init - Resource managed mutex initialization
+ * @dev:	Device which lifetime mutex is bound to
+ * @lock:	Mutex to be initialized (and automatically destroyed)
+ *
+ * Initialize mutex which is automatically destroyed when driver is detached.
+ * A few drivers initialize mutexes which they want destroyed before driver is
+ * detached, for debugging purposes.
+ * devm_mutex_init() can be used to omit the explicit mutex_destroy() call when
+ * driver is detached.
+ */
+static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	mutex_init(lock);
+
+	/*
+	 * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
+	 * disabled. No need to allocate an action in that case.
+	 */
+	if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
+		return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
+	else
+		return 0;
+}
+
 #endif
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function
  2024-02-22 14:58 [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Marek Behún
@ 2024-02-22 14:58 ` Marek Behún
  2024-02-22 15:23   ` Guenter Roeck
  2024-02-22 17:23   ` Dave Jiang
  2024-02-22 15:11 ` [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Bartosz Golaszewski
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 14+ messages in thread
From: Marek Behún @ 2024-02-22 14:58 UTC (permalink / raw)
  To: linux-kernel, Hans de Goede, Matti Vaittinen
  Cc: Marek Behún, Horia Geantă,
	Pankaj Gupta, Gaurav Jain, Herbert Xu, David S. Miller,
	Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Bamvor Jian Zhang,
	Linus Walleij, Bartosz Golaszewski, Douglas Anderson,
	Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, James Seo,
	Jean Delvare, Guenter Roeck, linux-crypto, linux-cxl, linux-gpio,
	dri-devel, linux-hwmon

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
drivers
  drivers/cxl/mem.c
  drivers/gpio/gpio-mockup.c

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/crypto/caam/ctrl.c            | 16 +++------
 drivers/cxl/mem.c                     |  9 ++---
 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          | 48 +++++++++++++++++++++++++++
 8 files changed, 72 insertions(+), 71 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index bdf367f3f679..ea3ed9a17f1a 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/devm-helpers.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
@@ -604,11 +605,6 @@ static int init_clocks(struct device *dev, const struct caam_imx_data *data)
 	return devm_add_action_or_reset(dev, disable_clocks, ctrlpriv);
 }
 
-static void caam_remove_debugfs(void *root)
-{
-	debugfs_remove_recursive(root);
-}
-
 #ifdef CONFIG_FSL_MC_BUS
 static bool check_version(struct fsl_mc_version *mc_version, u32 major,
 			  u32 minor, u32 revision)
@@ -1058,13 +1054,9 @@ static int caam_probe(struct platform_device *pdev)
 	ctrlpriv->era = caam_get_era(perfmon);
 	ctrlpriv->domain = iommu_get_domain_for_dev(dev);
 
-	dfs_root = debugfs_create_dir(dev_name(dev), NULL);
-	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
-		ret = devm_add_action_or_reset(dev, caam_remove_debugfs,
-					       dfs_root);
-		if (ret)
-			return ret;
-	}
+	dfs_root = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
+	if (IS_ERR(dfs_root))
+		return PTR_ERR(dfs_root);
 
 	caam_debugfs_init(ctrlpriv, perfmon, dfs_root);
 
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index c5c9d8e0d88d..4b38514887a4 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
 #include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/devm-helpers.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 
@@ -30,11 +31,6 @@ static void enable_suspend(void *data)
 	cxl_mem_active_dec();
 }
 
-static void remove_debugfs(void *dentry)
-{
-	debugfs_remove_recursive(dentry);
-}
-
 static int cxl_mem_dpa_show(struct seq_file *file, void *data)
 {
 	struct device *dev = file->private;
@@ -138,7 +134,8 @@ static int cxl_mem_probe(struct device *dev)
 		debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
 				    &cxl_poison_clear_fops);
 
-	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
+	rc = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
+				      dentry);
 	if (rc)
 		return rc;
 
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);
 }
 
 static const struct of_device_id gpio_mockup_of_match[] = {
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 62cc3893dca5..ad0ed2459394 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -10,6 +10,7 @@
 #include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/i2c.h>
@@ -427,18 +428,12 @@ static int status_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(status);
 
-static void ti_sn65dsi86_debugfs_remove(void *data)
-{
-	debugfs_remove_recursive(data);
-}
-
 static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
 {
 	struct device *dev = pdata->dev;
 	struct dentry *debugfs;
-	int ret;
 
-	debugfs = debugfs_create_dir(dev_name(dev), NULL);
+	debugfs = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
 
 	/*
 	 * We might get an error back if debugfs wasn't enabled in the kernel
@@ -447,10 +442,6 @@ static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
 	if (IS_ERR_OR_NULL(debugfs))
 		return;
 
-	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove, debugfs);
-	if (ret)
-		return;
-
 	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
 }
 
diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
index b5325d0e72b9..2a7c33763ce8 100644
--- a/drivers/hwmon/hp-wmi-sensors.c
+++ b/drivers/hwmon/hp-wmi-sensors.c
@@ -23,6 +23,7 @@
 
 #include <linux/acpi.h>
 #include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
 #include <linux/hwmon.h>
 #include <linux/jiffies.h>
 #include <linux/mutex.h>
@@ -1304,12 +1305,6 @@ static int current_reading_show(struct seq_file *seqf, void *ignored)
 }
 DEFINE_SHOW_ATTRIBUTE(current_reading);
 
-/* hp_wmi_devm_debugfs_remove - devm callback for debugfs cleanup */
-static void hp_wmi_devm_debugfs_remove(void *res)
-{
-	debugfs_remove_recursive(res);
-}
-
 /* hp_wmi_debugfs_init - create and populate debugfs directory tree */
 static void hp_wmi_debugfs_init(struct device *dev, struct hp_wmi_info *info,
 				struct hp_wmi_platform_events *pevents,
@@ -1320,21 +1315,15 @@ static void hp_wmi_debugfs_init(struct device *dev, struct hp_wmi_info *info,
 	struct dentry *debugfs;
 	struct dentry *entries;
 	struct dentry *dir;
-	int err;
 	u8 i;
 
 	/* dev_name() gives a not-very-friendly GUID for WMI devices. */
 	scnprintf(buf, sizeof(buf), "hp-wmi-sensors-%u", dev->id);
 
-	debugfs = debugfs_create_dir(buf, NULL);
+	debugfs = devm_debugfs_create_dir(dev, buf, NULL);
 	if (IS_ERR(debugfs))
 		return;
 
-	err = devm_add_action_or_reset(dev, hp_wmi_devm_debugfs_remove,
-				       debugfs);
-	if (err)
-		return;
-
 	entries = debugfs_create_dir("sensor", debugfs);
 
 	for (i = 0; i < icount; i++, info++) {
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);
 
 	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;
 }
 
 static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 1363d9f89181..e0f956a21125 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
 #include <linux/kernel.h>
 #include <linux/math64.h>
 #include <linux/module.h>
@@ -3336,13 +3337,6 @@ static const struct file_operations pmbus_debugfs_ops_mfr = {
 	.open = simple_open,
 };
 
-static void pmbus_remove_debugfs(void *data)
-{
-	struct dentry *entry = data;
-
-	debugfs_remove_recursive(entry);
-}
-
 static int pmbus_init_debugfs(struct i2c_client *client,
 			      struct pmbus_data *data)
 {
@@ -3357,8 +3351,9 @@ static int pmbus_init_debugfs(struct i2c_client *client,
 	 * Create the debugfs directory for this device. Use the hwmon device
 	 * name to avoid conflicts (hwmon numbers are globally unique).
 	 */
-	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
-					   pmbus_debugfs_dir);
+	data->debugfs = devm_debugfs_create_dir(data->dev,
+						dev_name(data->hwmon_dev),
+						pmbus_debugfs_dir);
 	if (IS_ERR_OR_NULL(data->debugfs)) {
 		data->debugfs = NULL;
 		return -ENODEV;
@@ -3542,8 +3537,7 @@ static int pmbus_init_debugfs(struct i2c_client *client,
 		}
 	}
 
-	return devm_add_action_or_reset(data->dev,
-					pmbus_remove_debugfs, data->debugfs);
+	return 0;
 }
 #else
 static int pmbus_init_debugfs(struct i2c_client *client,
diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
index 70640fb96117..39d743175ec4 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/mutex.h>
@@ -108,4 +109,51 @@ static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
 		return 0;
 }
 
+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;
+
+	dentry = debugfs_create_dir(name, parent);
+	if (IS_ERR(dentry))
+		return dentry;
+
+	/*
+	 * debugfs_remove_recursive() is an empty function if CONFIG_DEBUG_FS is
+	 * disabled. No need to register an action in that case.
+	 */
+	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
+		int err;
+
+		err = devm_add_action_or_reset(dev,
+					       devm_debugfs_dir_recursive_drop,
+					       dentry);
+		if (err < 0)
+			return ERR_PTR(err);
+	}
+
+	return dentry;
+}
+
 #endif
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
  2024-02-22 14:58 [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Marek Behún
  2024-02-22 14:58 ` [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function Marek Behún
@ 2024-02-22 15:11 ` Bartosz Golaszewski
  2024-02-22 15:24 ` Guenter Roeck
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2024-02-22 15:11 UTC (permalink / raw)
  To: Marek Behún
  Cc: linux-kernel, Hans de Goede, Matti Vaittinen, Linus Walleij,
	Lucas De Marchi, Oded Gabbay, Thomas Hellström,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Aleksandr Mezin, Jean Delvare,
	Guenter Roeck, Pavel Machek, Lee Jones, Sebastian Reichel,
	Matthias Brugger, AngeloGioacchino Del Regno, linux-gpio,
	intel-xe, dri-devel, linux-hwmon, linux-leds, linux-pm,
	linux-arm-kernel, linux-mediatek

On Thu, Feb 22, 2024 at 3:58 PM Marek Behún <kabel@kernel.org> wrote:
>
> A few drivers are doing resource-managed mutex initialization by
> implementing ad-hoc one-liner mutex dropping functions and using them
> with devm_add_action_or_reset(). Help drivers avoid these repeated
> one-liners by adding managed version of mutex initialization.
>
> Use the new function devm_mutex_init() in the following drivers:
>   drivers/gpio/gpio-pisosr.c
>   drivers/gpio/gpio-sim.c

Yes, please!

For GPIO part:

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function
  2024-02-22 14:58 ` [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function Marek Behún
@ 2024-02-22 15:23   ` Guenter Roeck
  2024-02-22 17:23   ` Dave Jiang
  1 sibling, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2024-02-22 15:23 UTC (permalink / raw)
  To: Marek Behún, linux-kernel, Hans de Goede, Matti Vaittinen
  Cc: Horia Geantă,
	Pankaj Gupta, Gaurav Jain, Herbert Xu, David S. Miller,
	Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Bamvor Jian Zhang,
	Linus Walleij, Bartosz Golaszewski, Douglas Anderson,
	Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, James Seo,
	Jean Delvare, linux-crypto, linux-cxl, linux-gpio, dri-devel,
	linux-hwmon

On 2/22/24 06:58, 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.
> 
> 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
> drivers
>    drivers/cxl/mem.c
>    drivers/gpio/gpio-mockup.c
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>   drivers/crypto/caam/ctrl.c            | 16 +++------
>   drivers/cxl/mem.c                     |  9 ++---
>   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          | 48 +++++++++++++++++++++++++++
>   8 files changed, 72 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index bdf367f3f679..ea3ed9a17f1a 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -7,6 +7,7 @@
>    */
>   
>   #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/of_address.h>
>   #include <linux/of_irq.h>
>   #include <linux/platform_device.h>
> @@ -604,11 +605,6 @@ static int init_clocks(struct device *dev, const struct caam_imx_data *data)
>   	return devm_add_action_or_reset(dev, disable_clocks, ctrlpriv);
>   }
>   
> -static void caam_remove_debugfs(void *root)
> -{
> -	debugfs_remove_recursive(root);
> -}
> -
>   #ifdef CONFIG_FSL_MC_BUS
>   static bool check_version(struct fsl_mc_version *mc_version, u32 major,
>   			  u32 minor, u32 revision)
> @@ -1058,13 +1054,9 @@ static int caam_probe(struct platform_device *pdev)
>   	ctrlpriv->era = caam_get_era(perfmon);
>   	ctrlpriv->domain = iommu_get_domain_for_dev(dev);
>   
> -	dfs_root = debugfs_create_dir(dev_name(dev), NULL);
> -	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> -		ret = devm_add_action_or_reset(dev, caam_remove_debugfs,
> -					       dfs_root);
> -		if (ret)
> -			return ret;
> -	}
> +	dfs_root = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> +	if (IS_ERR(dfs_root))
> +		return PTR_ERR(dfs_root);
>   
>   	caam_debugfs_init(ctrlpriv, perfmon, dfs_root);
>   
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c5c9d8e0d88d..4b38514887a4 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -2,6 +2,7 @@
>   /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
>   #include <linux/debugfs.h>
>   #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
>   
> @@ -30,11 +31,6 @@ static void enable_suspend(void *data)
>   	cxl_mem_active_dec();
>   }
>   
> -static void remove_debugfs(void *dentry)
> -{
> -	debugfs_remove_recursive(dentry);
> -}
> -
>   static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>   {
>   	struct device *dev = file->private;
> @@ -138,7 +134,8 @@ static int cxl_mem_probe(struct device *dev)
>   		debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
>   				    &cxl_poison_clear_fops);
>   
> -	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
> +	rc = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
> +				      dentry);
>   	if (rc)
>   		return rc;
>   
> 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);
>   }
>   
>   static const struct of_device_id gpio_mockup_of_match[] = {
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 62cc3893dca5..ad0ed2459394 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -10,6 +10,7 @@
>   #include <linux/bits.h>
>   #include <linux/clk.h>
>   #include <linux/debugfs.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/gpio/driver.h>
>   #include <linux/i2c.h>
> @@ -427,18 +428,12 @@ static int status_show(struct seq_file *s, void *data)
>   
>   DEFINE_SHOW_ATTRIBUTE(status);
>   
> -static void ti_sn65dsi86_debugfs_remove(void *data)
> -{
> -	debugfs_remove_recursive(data);
> -}
> -
>   static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
>   {
>   	struct device *dev = pdata->dev;
>   	struct dentry *debugfs;
> -	int ret;
>   
> -	debugfs = debugfs_create_dir(dev_name(dev), NULL);
> +	debugfs = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
>   
>   	/*
>   	 * We might get an error back if debugfs wasn't enabled in the kernel
> @@ -447,10 +442,6 @@ static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
>   	if (IS_ERR_OR_NULL(debugfs))
>   		return;
>   
> -	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove, debugfs);
> -	if (ret)
> -		return;
> -
>   	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
>   }
>   
> diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
> index b5325d0e72b9..2a7c33763ce8 100644
> --- a/drivers/hwmon/hp-wmi-sensors.c
> +++ b/drivers/hwmon/hp-wmi-sensors.c
> @@ -23,6 +23,7 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/debugfs.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/hwmon.h>
>   #include <linux/jiffies.h>
>   #include <linux/mutex.h>
> @@ -1304,12 +1305,6 @@ static int current_reading_show(struct seq_file *seqf, void *ignored)
>   }
>   DEFINE_SHOW_ATTRIBUTE(current_reading);
>   
> -/* hp_wmi_devm_debugfs_remove - devm callback for debugfs cleanup */
> -static void hp_wmi_devm_debugfs_remove(void *res)
> -{
> -	debugfs_remove_recursive(res);
> -}
> -
>   /* hp_wmi_debugfs_init - create and populate debugfs directory tree */
>   static void hp_wmi_debugfs_init(struct device *dev, struct hp_wmi_info *info,
>   				struct hp_wmi_platform_events *pevents,
> @@ -1320,21 +1315,15 @@ static void hp_wmi_debugfs_init(struct device *dev, struct hp_wmi_info *info,
>   	struct dentry *debugfs;
>   	struct dentry *entries;
>   	struct dentry *dir;
> -	int err;
>   	u8 i;
>   
>   	/* dev_name() gives a not-very-friendly GUID for WMI devices. */
>   	scnprintf(buf, sizeof(buf), "hp-wmi-sensors-%u", dev->id);
>   
> -	debugfs = debugfs_create_dir(buf, NULL);
> +	debugfs = devm_debugfs_create_dir(dev, buf, NULL);
>   	if (IS_ERR(debugfs))
>   		return;
>   
> -	err = devm_add_action_or_reset(dev, hp_wmi_devm_debugfs_remove,
> -				       debugfs);
> -	if (err)
> -		return;
> -
>   	entries = debugfs_create_dir("sensor", debugfs);
>   
>   	for (i = 0; i < icount; i++, info++) {
> 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);
>   
>   	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;
>   }
>   
>   static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 1363d9f89181..e0f956a21125 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -7,6 +7,7 @@
>    */
>   
>   #include <linux/debugfs.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/kernel.h>
>   #include <linux/math64.h>
>   #include <linux/module.h>
> @@ -3336,13 +3337,6 @@ static const struct file_operations pmbus_debugfs_ops_mfr = {
>   	.open = simple_open,
>   };
>   
> -static void pmbus_remove_debugfs(void *data)
> -{
> -	struct dentry *entry = data;
> -
> -	debugfs_remove_recursive(entry);
> -}
> -
>   static int pmbus_init_debugfs(struct i2c_client *client,
>   			      struct pmbus_data *data)
>   {
> @@ -3357,8 +3351,9 @@ static int pmbus_init_debugfs(struct i2c_client *client,
>   	 * Create the debugfs directory for this device. Use the hwmon device
>   	 * name to avoid conflicts (hwmon numbers are globally unique).
>   	 */
> -	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
> -					   pmbus_debugfs_dir);
> +	data->debugfs = devm_debugfs_create_dir(data->dev,
> +						dev_name(data->hwmon_dev),
> +						pmbus_debugfs_dir);
>   	if (IS_ERR_OR_NULL(data->debugfs)) {
>   		data->debugfs = NULL;
>   		return -ENODEV;
> @@ -3542,8 +3537,7 @@ static int pmbus_init_debugfs(struct i2c_client *client,
>   		}
>   	}
>   
> -	return devm_add_action_or_reset(data->dev,
> -					pmbus_remove_debugfs, data->debugfs);
> +	return 0;
>   }
>   #else
>   static int pmbus_init_debugfs(struct i2c_client *client,
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 70640fb96117..39d743175ec4 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/mutex.h>
> @@ -108,4 +109,51 @@ static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>   		return 0;
>   }
>   
> +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;
> +
> +	dentry = debugfs_create_dir(name, parent);
> +	if (IS_ERR(dentry))
> +		return dentry;
> +
> +	/*
> +	 * debugfs_remove_recursive() is an empty function if CONFIG_DEBUG_FS is
> +	 * disabled. No need to register an action in that case.
> +	 */
> +	if (IS_ENABLED(CONFIG_DEBUG_FS)) {


This conditional seems unnecessary since in that case debugfs_create_dir()
would return -ENODEV, and the code below would never be executed to start with.

Guenter

> +		int err;
> +
> +		err = devm_add_action_or_reset(dev,
> +					       devm_debugfs_dir_recursive_drop,
> +					       dentry);
> +		if (err < 0)
> +			return ERR_PTR(err);
> +	}
> +
> +	return dentry;
> +}
> +
>   #endif


^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
  2024-02-22 14:58 [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Marek Behún
  2024-02-22 14:58 ` [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function Marek Behún
  2024-02-22 15:11 ` [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Bartosz Golaszewski
@ 2024-02-22 15:24 ` Guenter Roeck
  2024-02-22 16:03   ` Guenter Roeck
  2024-02-22 16:44 ` Matthew Auld
  2024-02-22 21:42 ` andy.shevchenko
  4 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2024-02-22 15:24 UTC (permalink / raw)
  To: Marek Behún, linux-kernel, Hans de Goede, Matti Vaittinen
  Cc: Linus Walleij, Bartosz Golaszewski, Lucas De Marchi, Oded Gabbay,
	Thomas Hellström, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Aleksandr Mezin,
	Jean Delvare, Pavel Machek, Lee Jones, Sebastian Reichel,
	Matthias Brugger, AngeloGioacchino Del Regno, linux-gpio,
	intel-xe, dri-devel, linux-hwmon, linux-leds, linux-pm,
	linux-arm-kernel, linux-mediatek

On 2/22/24 06:58, Marek Behún wrote:
> A few drivers are doing resource-managed mutex initialization by
> implementing ad-hoc one-liner mutex dropping functions and using them
> with devm_add_action_or_reset(). Help drivers avoid these repeated
> one-liners by adding managed version of mutex initialization.
> 
> Use the new function devm_mutex_init() in the following drivers:
>    drivers/gpio/gpio-pisosr.c
>    drivers/gpio/gpio-sim.c
>    drivers/gpu/drm/xe/xe_hwmon.c
>    drivers/hwmon/nzxt-smart2.c
>    drivers/leds/leds-is31fl319x.c
>    drivers/power/supply/mt6370-charger.c
>    drivers/power/supply/rt9467-charger.c
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>   drivers/gpio/gpio-pisosr.c            |  9 ++-----
>   drivers/gpio/gpio-sim.c               | 12 ++--------
>   drivers/gpu/drm/xe/xe_hwmon.c         | 11 ++-------
>   drivers/hwmon/nzxt-smart2.c           |  9 ++-----
>   drivers/leds/leds-is31fl319x.c        |  9 ++-----
>   drivers/power/supply/mt6370-charger.c | 11 +--------
>   drivers/power/supply/rt9467-charger.c | 34 ++++-----------------------
>   include/linux/devm-helpers.h          | 32 +++++++++++++++++++++++++
>   8 files changed, 47 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pisosr.c b/drivers/gpio/gpio-pisosr.c
> index e3013e778e15..dddbf37e855f 100644
> --- a/drivers/gpio/gpio-pisosr.c
> +++ b/drivers/gpio/gpio-pisosr.c
> @@ -7,6 +7,7 @@
>   #include <linux/bitmap.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/gpio/driver.h>
>   #include <linux/module.h>
> @@ -116,11 +117,6 @@ static const struct gpio_chip template_chip = {
>   	.can_sleep		= true,
>   };
>   
> -static void pisosr_mutex_destroy(void *lock)
> -{
> -	mutex_destroy(lock);
> -}
> -
>   static int pisosr_gpio_probe(struct spi_device *spi)
>   {
>   	struct device *dev = &spi->dev;
> @@ -147,8 +143,7 @@ static int pisosr_gpio_probe(struct spi_device *spi)
>   		return dev_err_probe(dev, PTR_ERR(gpio->load_gpio),
>   				     "Unable to allocate load GPIO\n");
>   
> -	mutex_init(&gpio->lock);
> -	ret = devm_add_action_or_reset(dev, pisosr_mutex_destroy, &gpio->lock);
> +	ret = devm_mutex_init(dev, &gpio->lock);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index c4106e37e6db..fcfcaa4efe70 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -12,6 +12,7 @@
>   #include <linux/completion.h>
>   #include <linux/configfs.h>
>   #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/err.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/gpio/driver.h>
> @@ -307,13 +308,6 @@ static ssize_t gpio_sim_sysfs_pull_store(struct device *dev,
>   	return len;
>   }
>   
> -static void gpio_sim_mutex_destroy(void *data)
> -{
> -	struct mutex *lock = data;
> -
> -	mutex_destroy(lock);
> -}
> -
>   static void gpio_sim_put_device(void *data)
>   {
>   	struct device *dev = data;
> @@ -457,9 +451,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
>   	if (ret)
>   		return ret;
>   
> -	mutex_init(&chip->lock);
> -	ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,
> -				       &chip->lock);
> +	ret = devm_mutex_init(dev, &chip->lock);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 174ed2185481..bb88ae1c196c 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -3,6 +3,7 @@
>    * Copyright © 2023 Intel Corporation
>    */
>   
> +#include <linux/devm-helpers.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/hwmon.h>
>   #include <linux/types.h>
> @@ -729,13 +730,6 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
>   		xe_hwmon_energy_get(hwmon, &energy);
>   }
>   
> -static void xe_hwmon_mutex_destroy(void *arg)
> -{
> -	struct xe_hwmon *hwmon = arg;
> -
> -	mutex_destroy(&hwmon->hwmon_lock);
> -}
> -
>   void xe_hwmon_register(struct xe_device *xe)
>   {
>   	struct device *dev = xe->drm.dev;
> @@ -751,8 +745,7 @@ void xe_hwmon_register(struct xe_device *xe)
>   
>   	xe->hwmon = hwmon;
>   
> -	mutex_init(&hwmon->hwmon_lock);
> -	if (devm_add_action_or_reset(dev, xe_hwmon_mutex_destroy, hwmon))
> +	if (devm_mutex_init(dev, &hwmon->hwmon_lock))
>   		return;
>   
>   	/* primary GT to access device level properties */
> diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
> index 7aa586eb74be..00bc89607673 100644
> --- a/drivers/hwmon/nzxt-smart2.c
> +++ b/drivers/hwmon/nzxt-smart2.c
> @@ -5,6 +5,7 @@
>    * Copyright (c) 2021 Aleksandr Mezin
>    */
>   
> +#include <linux/devm-helpers.h>
>   #include <linux/hid.h>
>   #include <linux/hwmon.h>
>   #include <linux/math.h>
> @@ -721,11 +722,6 @@ static int __maybe_unused nzxt_smart2_hid_reset_resume(struct hid_device *hdev)
>   	return init_device(drvdata, drvdata->update_interval);
>   }
>   
> -static void mutex_fini(void *lock)
> -{
> -	mutex_destroy(lock);
> -}
> -
>   static int nzxt_smart2_hid_probe(struct hid_device *hdev,
>   				 const struct hid_device_id *id)
>   {
> @@ -741,8 +737,7 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
>   
>   	init_waitqueue_head(&drvdata->wq);
>   
> -	mutex_init(&drvdata->mutex);
> -	ret = devm_add_action_or_reset(&hdev->dev, mutex_fini, &drvdata->mutex);
> +	ret = devm_mutex_init(&hdev->dev, &drvdata->mutex);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
> index 66c65741202e..e9d7cf6a386c 100644
> --- a/drivers/leds/leds-is31fl319x.c
> +++ b/drivers/leds/leds-is31fl319x.c
> @@ -8,6 +8,7 @@
>    * effect LEDs.
>    */
>   
> +#include <linux/devm-helpers.h>
>   #include <linux/err.h>
>   #include <linux/i2c.h>
>   #include <linux/leds.h>
> @@ -495,11 +496,6 @@ static inline int is31fl3196_db_to_gain(u32 dezibel)
>   	return dezibel / IS31FL3196_AUDIO_GAIN_DB_STEP;
>   }
>   
> -static void is31f1319x_mutex_destroy(void *lock)
> -{
> -	mutex_destroy(lock);
> -}
> -
>   static int is31fl319x_probe(struct i2c_client *client)
>   {
>   	struct is31fl319x_chip *is31;
> @@ -515,8 +511,7 @@ static int is31fl319x_probe(struct i2c_client *client)
>   	if (!is31)
>   		return -ENOMEM;
>   
> -	mutex_init(&is31->lock);
> -	err = devm_add_action_or_reset(dev, is31f1319x_mutex_destroy, &is31->lock);
> +	err = devm_mutex_init(dev, &is31->lock);
>   	if (err)
>   		return err;
>   
> diff --git a/drivers/power/supply/mt6370-charger.c b/drivers/power/supply/mt6370-charger.c
> index e24fce087d80..fa0517d0352d 100644
> --- a/drivers/power/supply/mt6370-charger.c
> +++ b/drivers/power/supply/mt6370-charger.c
> @@ -766,13 +766,6 @@ static int mt6370_chg_init_psy(struct mt6370_priv *priv)
>   	return PTR_ERR_OR_ZERO(priv->psy);
>   }
>   
> -static void mt6370_chg_destroy_attach_lock(void *data)
> -{
> -	struct mutex *attach_lock = data;
> -
> -	mutex_destroy(attach_lock);
> -}
> -
>   static void mt6370_chg_destroy_wq(void *data)
>   {
>   	struct workqueue_struct *wq = data;
> @@ -900,9 +893,7 @@ static int mt6370_chg_probe(struct platform_device *pdev)
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init psy\n");
>   
> -	mutex_init(&priv->attach_lock);
> -	ret = devm_add_action_or_reset(dev, mt6370_chg_destroy_attach_lock,
> -				       &priv->attach_lock);
> +	ret = devm_mutex_init(dev, &priv->attach_lock);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init attach lock\n");
>   
> diff --git a/drivers/power/supply/rt9467-charger.c b/drivers/power/supply/rt9467-charger.c
> index fdfdc83ab045..84f07c22077f 100644
> --- a/drivers/power/supply/rt9467-charger.c
> +++ b/drivers/power/supply/rt9467-charger.c
> @@ -10,6 +10,7 @@
>   #include <linux/bitfield.h>
>   #include <linux/completion.h>
>   #include <linux/delay.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
>   #include <linux/interrupt.h>
> @@ -1149,27 +1150,6 @@ static int rt9467_reset_chip(struct rt9467_chg_data *data)
>   	return regmap_field_write(data->rm_field[F_RST], 1);
>   }
>   
> -static void rt9467_chg_destroy_adc_lock(void *data)
> -{
> -	struct mutex *adc_lock = data;
> -
> -	mutex_destroy(adc_lock);
> -}
> -
> -static void rt9467_chg_destroy_attach_lock(void *data)
> -{
> -	struct mutex *attach_lock = data;
> -
> -	mutex_destroy(attach_lock);
> -}
> -
> -static void rt9467_chg_destroy_ichg_ieoc_lock(void *data)
> -{
> -	struct mutex *ichg_ieoc_lock = data;
> -
> -	mutex_destroy(ichg_ieoc_lock);
> -}
> -
>   static void rt9467_chg_complete_aicl_done(void *data)
>   {
>   	struct completion *aicl_done = data;
> @@ -1222,21 +1202,15 @@ static int rt9467_charger_probe(struct i2c_client *i2c)
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to add irq chip\n");
>   
> -	mutex_init(&data->adc_lock);
> -	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_adc_lock,
> -				       &data->adc_lock);
> +	ret = devm_mutex_init(dev, &data->adc_lock);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init ADC lock\n");
>   
> -	mutex_init(&data->attach_lock);
> -	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_attach_lock,
> -				       &data->attach_lock);
> +	ret = devm_mutex_init(dev, &data->attach_lock);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init attach lock\n");
>   
> -	mutex_init(&data->ichg_ieoc_lock);
> -	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_ichg_ieoc_lock,
> -				       &data->ichg_ieoc_lock);
> +	ret = devm_mutex_init(dev, &data->ichg_ieoc_lock);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init ICHG/IEOC lock\n");
>   
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..70640fb96117 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,8 @@
>    */
>   
>   #include <linux/device.h>
> +#include <linux/kconfig.h>
> +#include <linux/mutex.h>
>   #include <linux/workqueue.h>
>   
>   static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +78,34 @@ static inline int devm_work_autocancel(struct device *dev,
>   	return devm_add_action(dev, devm_work_drop, w);
>   }
>   
> +static inline void devm_mutex_drop(void *res)
> +{
> +	mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource managed mutex initialization
> + * @dev:	Device which lifetime mutex is bound to
> + * @lock:	Mutex to be initialized (and automatically destroyed)
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.
> + * A few drivers initialize mutexes which they want destroyed before driver is
> + * detached, for debugging purposes.
> + * devm_mutex_init() can be used to omit the explicit mutex_destroy() call when
> + * driver is detached.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +
> +	/*
> +	 * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
> +	 * disabled. No need to allocate an action in that case.
> +	 */
> +	if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
> +		return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
> +	else

else after return is unnecessary.

> +		return 0;
> +}
> +
>   #endif


^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
  2024-02-22 15:24 ` Guenter Roeck
@ 2024-02-22 16:03   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2024-02-22 16:03 UTC (permalink / raw)
  To: Marek Behún, linux-kernel, Hans de Goede, Matti Vaittinen
  Cc: Linus Walleij, Bartosz Golaszewski, Lucas De Marchi, Oded Gabbay,
	Thomas Hellström, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Aleksandr Mezin,
	Jean Delvare, Pavel Machek, Lee Jones, Sebastian Reichel,
	Matthias Brugger, AngeloGioacchino Del Regno, linux-gpio,
	intel-xe, dri-devel, linux-hwmon, linux-leds, linux-pm,
	linux-arm-kernel, linux-mediatek

Oops, sorry for the noise. Shortened reply below.

On 2/22/24 07:24, Guenter Roeck wrote:
> On 2/22/24 06:58, Marek Behún wrote:
>> A few drivers are doing resource-managed mutex initialization by
>> implementing ad-hoc one-liner mutex dropping functions and using them
>> with devm_add_action_or_reset(). Help drivers avoid these repeated
>> one-liners by adding managed version of mutex initialization.
>>

[ ... ]

>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +    mutex_init(lock);
>> +
>> +    /*
>> +     * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
>> +     * disabled. No need to allocate an action in that case.
>> +     */
>> +    if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
>> +        return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
>> +    else
> 
> else after return is unnecessary.
> 
>> +        return 0;
>> +}
>> +
>>   #endif
> 
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
  2024-02-22 14:58 [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Marek Behún
                   ` (2 preceding siblings ...)
  2024-02-22 15:24 ` Guenter Roeck
@ 2024-02-22 16:44 ` Matthew Auld
  2024-02-22 18:33   ` Hans de Goede
  2024-02-22 21:42 ` andy.shevchenko
  4 siblings, 1 reply; 14+ messages in thread
From: Matthew Auld @ 2024-02-22 16:44 UTC (permalink / raw)
  To: Marek Behún, linux-kernel, Hans de Goede, Matti Vaittinen
  Cc: Linus Walleij, Bartosz Golaszewski, Lucas De Marchi, Oded Gabbay,
	Thomas Hellström, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Aleksandr Mezin,
	Jean Delvare, Guenter Roeck, Pavel Machek, Lee Jones,
	Sebastian Reichel, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-gpio, intel-xe, dri-devel, linux-hwmon, linux-leds,
	linux-pm, linux-arm-kernel, linux-mediatek

On 22/02/2024 14:58, Marek Behún wrote:
> A few drivers are doing resource-managed mutex initialization by
> implementing ad-hoc one-liner mutex dropping functions and using them
> with devm_add_action_or_reset(). Help drivers avoid these repeated
> one-liners by adding managed version of mutex initialization.
> 
> Use the new function devm_mutex_init() in the following drivers:
>    drivers/gpio/gpio-pisosr.c
>    drivers/gpio/gpio-sim.c
>    drivers/gpu/drm/xe/xe_hwmon.c
>    drivers/hwmon/nzxt-smart2.c
>    drivers/leds/leds-is31fl319x.c
>    drivers/power/supply/mt6370-charger.c
>    drivers/power/supply/rt9467-charger.c
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>   drivers/gpio/gpio-pisosr.c            |  9 ++-----
>   drivers/gpio/gpio-sim.c               | 12 ++--------
>   drivers/gpu/drm/xe/xe_hwmon.c         | 11 ++-------
>   drivers/hwmon/nzxt-smart2.c           |  9 ++-----
>   drivers/leds/leds-is31fl319x.c        |  9 ++-----
>   drivers/power/supply/mt6370-charger.c | 11 +--------
>   drivers/power/supply/rt9467-charger.c | 34 ++++-----------------------
>   include/linux/devm-helpers.h          | 32 +++++++++++++++++++++++++
>   8 files changed, 47 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pisosr.c b/drivers/gpio/gpio-pisosr.c
> index e3013e778e15..dddbf37e855f 100644
> --- a/drivers/gpio/gpio-pisosr.c
> +++ b/drivers/gpio/gpio-pisosr.c
> @@ -7,6 +7,7 @@
>   #include <linux/bitmap.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/gpio/driver.h>
>   #include <linux/module.h>
> @@ -116,11 +117,6 @@ static const struct gpio_chip template_chip = {
>   	.can_sleep		= true,
>   };
>   
> -static void pisosr_mutex_destroy(void *lock)
> -{
> -	mutex_destroy(lock);
> -}
> -
>   static int pisosr_gpio_probe(struct spi_device *spi)
>   {
>   	struct device *dev = &spi->dev;
> @@ -147,8 +143,7 @@ static int pisosr_gpio_probe(struct spi_device *spi)
>   		return dev_err_probe(dev, PTR_ERR(gpio->load_gpio),
>   				     "Unable to allocate load GPIO\n");
>   
> -	mutex_init(&gpio->lock);
> -	ret = devm_add_action_or_reset(dev, pisosr_mutex_destroy, &gpio->lock);
> +	ret = devm_mutex_init(dev, &gpio->lock);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> index c4106e37e6db..fcfcaa4efe70 100644
> --- a/drivers/gpio/gpio-sim.c
> +++ b/drivers/gpio/gpio-sim.c
> @@ -12,6 +12,7 @@
>   #include <linux/completion.h>
>   #include <linux/configfs.h>
>   #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/err.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/gpio/driver.h>
> @@ -307,13 +308,6 @@ static ssize_t gpio_sim_sysfs_pull_store(struct device *dev,
>   	return len;
>   }
>   
> -static void gpio_sim_mutex_destroy(void *data)
> -{
> -	struct mutex *lock = data;
> -
> -	mutex_destroy(lock);
> -}
> -
>   static void gpio_sim_put_device(void *data)
>   {
>   	struct device *dev = data;
> @@ -457,9 +451,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
>   	if (ret)
>   		return ret;
>   
> -	mutex_init(&chip->lock);
> -	ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,
> -				       &chip->lock);
> +	ret = devm_mutex_init(dev, &chip->lock);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 174ed2185481..bb88ae1c196c 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -3,6 +3,7 @@
>    * Copyright © 2023 Intel Corporation
>    */
>   
> +#include <linux/devm-helpers.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/hwmon.h>
>   #include <linux/types.h>
> @@ -729,13 +730,6 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
>   		xe_hwmon_energy_get(hwmon, &energy);
>   }
>   
> -static void xe_hwmon_mutex_destroy(void *arg)
> -{
> -	struct xe_hwmon *hwmon = arg;
> -
> -	mutex_destroy(&hwmon->hwmon_lock);
> -}
> -
>   void xe_hwmon_register(struct xe_device *xe)
>   {
>   	struct device *dev = xe->drm.dev;
> @@ -751,8 +745,7 @@ void xe_hwmon_register(struct xe_device *xe)
>   
>   	xe->hwmon = hwmon;
>   
> -	mutex_init(&hwmon->hwmon_lock);
> -	if (devm_add_action_or_reset(dev, xe_hwmon_mutex_destroy, hwmon))
> +	if (devm_mutex_init(dev, &hwmon->hwmon_lock))
>   		return;
>   
>   	/* primary GT to access device level properties */
> diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
> index 7aa586eb74be..00bc89607673 100644
> --- a/drivers/hwmon/nzxt-smart2.c
> +++ b/drivers/hwmon/nzxt-smart2.c
> @@ -5,6 +5,7 @@
>    * Copyright (c) 2021 Aleksandr Mezin
>    */
>   
> +#include <linux/devm-helpers.h>
>   #include <linux/hid.h>
>   #include <linux/hwmon.h>
>   #include <linux/math.h>
> @@ -721,11 +722,6 @@ static int __maybe_unused nzxt_smart2_hid_reset_resume(struct hid_device *hdev)
>   	return init_device(drvdata, drvdata->update_interval);
>   }
>   
> -static void mutex_fini(void *lock)
> -{
> -	mutex_destroy(lock);
> -}
> -
>   static int nzxt_smart2_hid_probe(struct hid_device *hdev,
>   				 const struct hid_device_id *id)
>   {
> @@ -741,8 +737,7 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
>   
>   	init_waitqueue_head(&drvdata->wq);
>   
> -	mutex_init(&drvdata->mutex);
> -	ret = devm_add_action_or_reset(&hdev->dev, mutex_fini, &drvdata->mutex);
> +	ret = devm_mutex_init(&hdev->dev, &drvdata->mutex);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
> index 66c65741202e..e9d7cf6a386c 100644
> --- a/drivers/leds/leds-is31fl319x.c
> +++ b/drivers/leds/leds-is31fl319x.c
> @@ -8,6 +8,7 @@
>    * effect LEDs.
>    */
>   
> +#include <linux/devm-helpers.h>
>   #include <linux/err.h>
>   #include <linux/i2c.h>
>   #include <linux/leds.h>
> @@ -495,11 +496,6 @@ static inline int is31fl3196_db_to_gain(u32 dezibel)
>   	return dezibel / IS31FL3196_AUDIO_GAIN_DB_STEP;
>   }
>   
> -static void is31f1319x_mutex_destroy(void *lock)
> -{
> -	mutex_destroy(lock);
> -}
> -
>   static int is31fl319x_probe(struct i2c_client *client)
>   {
>   	struct is31fl319x_chip *is31;
> @@ -515,8 +511,7 @@ static int is31fl319x_probe(struct i2c_client *client)
>   	if (!is31)
>   		return -ENOMEM;
>   
> -	mutex_init(&is31->lock);
> -	err = devm_add_action_or_reset(dev, is31f1319x_mutex_destroy, &is31->lock);
> +	err = devm_mutex_init(dev, &is31->lock);
>   	if (err)
>   		return err;
>   
> diff --git a/drivers/power/supply/mt6370-charger.c b/drivers/power/supply/mt6370-charger.c
> index e24fce087d80..fa0517d0352d 100644
> --- a/drivers/power/supply/mt6370-charger.c
> +++ b/drivers/power/supply/mt6370-charger.c
> @@ -766,13 +766,6 @@ static int mt6370_chg_init_psy(struct mt6370_priv *priv)
>   	return PTR_ERR_OR_ZERO(priv->psy);
>   }
>   
> -static void mt6370_chg_destroy_attach_lock(void *data)
> -{
> -	struct mutex *attach_lock = data;
> -
> -	mutex_destroy(attach_lock);
> -}
> -
>   static void mt6370_chg_destroy_wq(void *data)
>   {
>   	struct workqueue_struct *wq = data;
> @@ -900,9 +893,7 @@ static int mt6370_chg_probe(struct platform_device *pdev)
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init psy\n");
>   
> -	mutex_init(&priv->attach_lock);
> -	ret = devm_add_action_or_reset(dev, mt6370_chg_destroy_attach_lock,
> -				       &priv->attach_lock);
> +	ret = devm_mutex_init(dev, &priv->attach_lock);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init attach lock\n");
>   
> diff --git a/drivers/power/supply/rt9467-charger.c b/drivers/power/supply/rt9467-charger.c
> index fdfdc83ab045..84f07c22077f 100644
> --- a/drivers/power/supply/rt9467-charger.c
> +++ b/drivers/power/supply/rt9467-charger.c
> @@ -10,6 +10,7 @@
>   #include <linux/bitfield.h>
>   #include <linux/completion.h>
>   #include <linux/delay.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
>   #include <linux/interrupt.h>
> @@ -1149,27 +1150,6 @@ static int rt9467_reset_chip(struct rt9467_chg_data *data)
>   	return regmap_field_write(data->rm_field[F_RST], 1);
>   }
>   
> -static void rt9467_chg_destroy_adc_lock(void *data)
> -{
> -	struct mutex *adc_lock = data;
> -
> -	mutex_destroy(adc_lock);
> -}
> -
> -static void rt9467_chg_destroy_attach_lock(void *data)
> -{
> -	struct mutex *attach_lock = data;
> -
> -	mutex_destroy(attach_lock);
> -}
> -
> -static void rt9467_chg_destroy_ichg_ieoc_lock(void *data)
> -{
> -	struct mutex *ichg_ieoc_lock = data;
> -
> -	mutex_destroy(ichg_ieoc_lock);
> -}
> -
>   static void rt9467_chg_complete_aicl_done(void *data)
>   {
>   	struct completion *aicl_done = data;
> @@ -1222,21 +1202,15 @@ static int rt9467_charger_probe(struct i2c_client *i2c)
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to add irq chip\n");
>   
> -	mutex_init(&data->adc_lock);
> -	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_adc_lock,
> -				       &data->adc_lock);
> +	ret = devm_mutex_init(dev, &data->adc_lock);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init ADC lock\n");
>   
> -	mutex_init(&data->attach_lock);
> -	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_attach_lock,
> -				       &data->attach_lock);
> +	ret = devm_mutex_init(dev, &data->attach_lock);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init attach lock\n");
>   
> -	mutex_init(&data->ichg_ieoc_lock);
> -	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_ichg_ieoc_lock,
> -				       &data->ichg_ieoc_lock);
> +	ret = devm_mutex_init(dev, &data->ichg_ieoc_lock);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to init ICHG/IEOC lock\n");
>   
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..70640fb96117 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,8 @@
>    */
>   
>   #include <linux/device.h>
> +#include <linux/kconfig.h>
> +#include <linux/mutex.h>
>   #include <linux/workqueue.h>
>   
>   static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +78,34 @@ static inline int devm_work_autocancel(struct device *dev,
>   	return devm_add_action(dev, devm_work_drop, w);
>   }
>   
> +static inline void devm_mutex_drop(void *res)
> +{
> +	mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource managed mutex initialization
> + * @dev:	Device which lifetime mutex is bound to
> + * @lock:	Mutex to be initialized (and automatically destroyed)
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.
> + * A few drivers initialize mutexes which they want destroyed before driver is
> + * detached, for debugging purposes.
> + * devm_mutex_init() can be used to omit the explicit mutex_destroy() call when
> + * driver is detached.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);

Do you know if this this needs __always_inline? The static lockdep key 
in mutex_init() should be different for each caller class. See 
c21f11d182c2 ("drm: fix drmm_mutex_init()").

> +
> +	/*
> +	 * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
> +	 * disabled. No need to allocate an action in that case.
> +	 */
> +	if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
> +		return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
> +	else
> +		return 0;
> +}
> +
>   #endif

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function
  2024-02-22 14:58 ` [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function Marek Behún
  2024-02-22 15:23   ` Guenter Roeck
@ 2024-02-22 17:23   ` Dave Jiang
  2024-02-22 18:11     ` Dan Williams
  1 sibling, 1 reply; 14+ messages in thread
From: Dave Jiang @ 2024-02-22 17:23 UTC (permalink / raw)
  To: Marek Behún, linux-kernel, Hans de Goede, Matti Vaittinen
  Cc: Horia Geantă,
	Pankaj Gupta, Gaurav Jain, Herbert Xu, David S. Miller,
	Davidlohr Bueso, Jonathan Cameron, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Bamvor Jian Zhang,
	Linus Walleij, Bartosz Golaszewski, Douglas Anderson,
	Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, James Seo,
	Jean Delvare, Guenter Roeck, linux-crypto, linux-cxl, linux-gpio,
	dri-devel, linux-hwmon



On 2/22/24 7:58 AM, 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.
> 
> 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
> drivers
>   drivers/cxl/mem.c
>   drivers/gpio/gpio-mockup.c
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/crypto/caam/ctrl.c            | 16 +++------
>  drivers/cxl/mem.c                     |  9 ++---
>  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          | 48 +++++++++++++++++++++++++++
>  8 files changed, 72 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index bdf367f3f679..ea3ed9a17f1a 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
> @@ -604,11 +605,6 @@ static int init_clocks(struct device *dev, const struct caam_imx_data *data)
>  	return devm_add_action_or_reset(dev, disable_clocks, ctrlpriv);
>  }
>  
> -static void caam_remove_debugfs(void *root)
> -{
> -	debugfs_remove_recursive(root);
> -}
> -
>  #ifdef CONFIG_FSL_MC_BUS
>  static bool check_version(struct fsl_mc_version *mc_version, u32 major,
>  			  u32 minor, u32 revision)
> @@ -1058,13 +1054,9 @@ static int caam_probe(struct platform_device *pdev)
>  	ctrlpriv->era = caam_get_era(perfmon);
>  	ctrlpriv->domain = iommu_get_domain_for_dev(dev);
>  
> -	dfs_root = debugfs_create_dir(dev_name(dev), NULL);
> -	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> -		ret = devm_add_action_or_reset(dev, caam_remove_debugfs,
> -					       dfs_root);
> -		if (ret)
> -			return ret;
> -	}
> +	dfs_root = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> +	if (IS_ERR(dfs_root))
> +		return PTR_ERR(dfs_root);
>  
>  	caam_debugfs_init(ctrlpriv, perfmon, dfs_root);
>  
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c5c9d8e0d88d..4b38514887a4 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -2,6 +2,7 @@
>  /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  
> @@ -30,11 +31,6 @@ static void enable_suspend(void *data)
>  	cxl_mem_active_dec();
>  }
>  
> -static void remove_debugfs(void *dentry)
> -{
> -	debugfs_remove_recursive(dentry);
> -}
> -
>  static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>  {
>  	struct device *dev = file->private;
> @@ -138,7 +134,8 @@ static int cxl_mem_probe(struct device *dev)
>  		debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
>  				    &cxl_poison_clear_fops);
>  
> -	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
> +	rc = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
> +				      dentry);

This is probably the better fix for cxl:

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 3b64fb1b9ed0..3258427af032 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -57,7 +57,6 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s);
 void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
 				   resource_size_t length);
 
-struct dentry *cxl_debugfs_create_dir(const char *dir);
 int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
 		     enum cxl_decoder_mode mode);
 int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size);
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 27166a411705..5c2db4791b8b 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1402,7 +1402,7 @@ void __init cxl_mbox_init(void)
 {
 	struct dentry *mbox_debugfs;
 
-	mbox_debugfs = cxl_debugfs_create_dir("mbox");
+	mbox_debugfs = debugfs_create_dir("mbox", NULL);
 	debugfs_create_bool("raw_allow_all", 0600, mbox_debugfs,
 			    &cxl_raw_allow_all);
 }
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index e59d9d37aa65..82c6a1c6aff4 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -10,6 +10,7 @@
 #include <linux/slab.h>
 #include <linux/idr.h>
 #include <linux/node.h>
+#include <linux/devm-helpers.h>
 #include <cxlmem.h>
 #include <cxlpci.h>
 #include <cxl.h>
@@ -2207,13 +2208,7 @@ struct bus_type cxl_bus_type = {
 };
 EXPORT_SYMBOL_NS_GPL(cxl_bus_type, CXL);
 
-static struct dentry *cxl_debugfs;
-
-struct dentry *cxl_debugfs_create_dir(const char *dir)
-{
-	return debugfs_create_dir(dir, cxl_debugfs);
-}
-EXPORT_SYMBOL_NS_GPL(cxl_debugfs_create_dir, CXL);
+struct dentry *cxl_debugfs;
 
 static __init int cxl_core_init(void)
 {
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b6017c0c57b4..ca8399b24955 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -880,6 +880,8 @@ void cxl_switch_parse_cdat(struct cxl_port *port);
 int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 				      struct access_coordinate *coord);
 
+extern struct dentry *cxl_debugfs;
+
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
  * of these symbols in tools/testing/cxl/.
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 5303d6942b88..b6f13ba87927 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -859,6 +859,5 @@ struct cxl_hdm {
 };
 
 struct seq_file;
-struct dentry *cxl_debugfs_create_dir(const char *dir);
 void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index c5c9d8e0d88d..494abe7a54c5 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -4,6 +4,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/devm-helpers.h>
 
 #include "cxlmem.h"
 #include "cxlpci.h"
@@ -30,11 +31,6 @@ static void enable_suspend(void *data)
 	cxl_mem_active_dec();
 }
 
-static void remove_debugfs(void *dentry)
-{
-	debugfs_remove_recursive(dentry);
-}
-
 static int cxl_mem_dpa_show(struct seq_file *file, void *data)
 {
 	struct device *dev = file->private;
@@ -128,7 +124,10 @@ static int cxl_mem_probe(struct device *dev)
 	if (work_pending(&cxlmd->detach_work))
 		return -EBUSY;
 
-	dentry = cxl_debugfs_create_dir(dev_name(dev));
+	dentry = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
+
 	debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show);
 
 	if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
@@ -138,10 +137,6 @@ static int cxl_mem_probe(struct device *dev)
 		debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
 				    &cxl_poison_clear_fops);
 
-	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
-	if (rc)
-		return rc;
-
 	rc = devm_cxl_enumerate_ports(cxlmd);
 	if (rc)
 		return rc;



>  	if (rc)
>  		return rc;
>  
> 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);
>  }
>  
>  static const struct of_device_id gpio_mockup_of_match[] = {
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 62cc3893dca5..ad0ed2459394 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -10,6 +10,7 @@
>  #include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/debugfs.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/i2c.h>
> @@ -427,18 +428,12 @@ static int status_show(struct seq_file *s, void *data)
>  
>  DEFINE_SHOW_ATTRIBUTE(status);
>  
> -static void ti_sn65dsi86_debugfs_remove(void *data)
> -{
> -	debugfs_remove_recursive(data);
> -}
> -
>  static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
>  {
>  	struct device *dev = pdata->dev;
>  	struct dentry *debugfs;
> -	int ret;
>  
> -	debugfs = debugfs_create_dir(dev_name(dev), NULL);
> +	debugfs = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
>  
>  	/*
>  	 * We might get an error back if debugfs wasn't enabled in the kernel
> @@ -447,10 +442,6 @@ static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
>  	if (IS_ERR_OR_NULL(debugfs))
>  		return;
>  
> -	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove, debugfs);
> -	if (ret)
> -		return;
> -
>  	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
>  }
>  
> diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
> index b5325d0e72b9..2a7c33763ce8 100644
> --- a/drivers/hwmon/hp-wmi-sensors.c
> +++ b/drivers/hwmon/hp-wmi-sensors.c
> @@ -23,6 +23,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/debugfs.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/hwmon.h>
>  #include <linux/jiffies.h>
>  #include <linux/mutex.h>
> @@ -1304,12 +1305,6 @@ static int current_reading_show(struct seq_file *seqf, void *ignored)
>  }
>  DEFINE_SHOW_ATTRIBUTE(current_reading);
>  
> -/* hp_wmi_devm_debugfs_remove - devm callback for debugfs cleanup */
> -static void hp_wmi_devm_debugfs_remove(void *res)
> -{
> -	debugfs_remove_recursive(res);
> -}
> -
>  /* hp_wmi_debugfs_init - create and populate debugfs directory tree */
>  static void hp_wmi_debugfs_init(struct device *dev, struct hp_wmi_info *info,
>  				struct hp_wmi_platform_events *pevents,
> @@ -1320,21 +1315,15 @@ static void hp_wmi_debugfs_init(struct device *dev, struct hp_wmi_info *info,
>  	struct dentry *debugfs;
>  	struct dentry *entries;
>  	struct dentry *dir;
> -	int err;
>  	u8 i;
>  
>  	/* dev_name() gives a not-very-friendly GUID for WMI devices. */
>  	scnprintf(buf, sizeof(buf), "hp-wmi-sensors-%u", dev->id);
>  
> -	debugfs = debugfs_create_dir(buf, NULL);
> +	debugfs = devm_debugfs_create_dir(dev, buf, NULL);
>  	if (IS_ERR(debugfs))
>  		return;
>  
> -	err = devm_add_action_or_reset(dev, hp_wmi_devm_debugfs_remove,
> -				       debugfs);
> -	if (err)
> -		return;
> -
>  	entries = debugfs_create_dir("sensor", debugfs);
>  
>  	for (i = 0; i < icount; i++, info++) {
> 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);
>  
>  	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;
>  }
>  
>  static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 1363d9f89181..e0f956a21125 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/debugfs.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/kernel.h>
>  #include <linux/math64.h>
>  #include <linux/module.h>
> @@ -3336,13 +3337,6 @@ static const struct file_operations pmbus_debugfs_ops_mfr = {
>  	.open = simple_open,
>  };
>  
> -static void pmbus_remove_debugfs(void *data)
> -{
> -	struct dentry *entry = data;
> -
> -	debugfs_remove_recursive(entry);
> -}
> -
>  static int pmbus_init_debugfs(struct i2c_client *client,
>  			      struct pmbus_data *data)
>  {
> @@ -3357,8 +3351,9 @@ static int pmbus_init_debugfs(struct i2c_client *client,
>  	 * Create the debugfs directory for this device. Use the hwmon device
>  	 * name to avoid conflicts (hwmon numbers are globally unique).
>  	 */
> -	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
> -					   pmbus_debugfs_dir);
> +	data->debugfs = devm_debugfs_create_dir(data->dev,
> +						dev_name(data->hwmon_dev),
> +						pmbus_debugfs_dir);
>  	if (IS_ERR_OR_NULL(data->debugfs)) {
>  		data->debugfs = NULL;
>  		return -ENODEV;
> @@ -3542,8 +3537,7 @@ static int pmbus_init_debugfs(struct i2c_client *client,
>  		}
>  	}
>  
> -	return devm_add_action_or_reset(data->dev,
> -					pmbus_remove_debugfs, data->debugfs);
> +	return 0;
>  }
>  #else
>  static int pmbus_init_debugfs(struct i2c_client *client,
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 70640fb96117..39d743175ec4 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/mutex.h>
> @@ -108,4 +109,51 @@ static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>  		return 0;
>  }
>  
> +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;
> +
> +	dentry = debugfs_create_dir(name, parent);
> +	if (IS_ERR(dentry))
> +		return dentry;
> +
> +	/*
> +	 * debugfs_remove_recursive() is an empty function if CONFIG_DEBUG_FS is
> +	 * disabled. No need to register an action in that case.
> +	 */
> +	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> +		int err;
> +
> +		err = devm_add_action_or_reset(dev,
> +					       devm_debugfs_dir_recursive_drop,
> +					       dentry);
> +		if (err < 0)
> +			return ERR_PTR(err);
> +	}
> +
> +	return dentry;
> +}
> +
>  #endif

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function
  2024-02-22 17:23   ` Dave Jiang
@ 2024-02-22 18:11     ` Dan Williams
  2024-02-22 18:25       ` Dan Williams
  0 siblings, 1 reply; 14+ messages in thread
From: Dan Williams @ 2024-02-22 18:11 UTC (permalink / raw)
  To: Dave Jiang, Marek Behún, linux-kernel, Hans de Goede,
	Matti Vaittinen
  Cc: Horia Geantă,
	Pankaj Gupta, Gaurav Jain, Herbert Xu, David S. Miller,
	Davidlohr Bueso, Jonathan Cameron, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Bamvor Jian Zhang,
	Linus Walleij, Bartosz Golaszewski, Douglas Anderson,
	Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, James Seo,
	Jean Delvare, Guenter Roeck, linux-crypto, linux-cxl, linux-gpio,
	dri-devel, linux-hwmon

Dave Jiang wrote:
> 
> 
> On 2/22/24 7:58 AM, 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.
> > 
> > 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
> > drivers
> >   drivers/cxl/mem.c
> >   drivers/gpio/gpio-mockup.c
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/crypto/caam/ctrl.c            | 16 +++------
> >  drivers/cxl/mem.c                     |  9 ++---
> >  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          | 48 +++++++++++++++++++++++++++
> >  8 files changed, 72 insertions(+), 71 deletions(-)
> > 
> > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > index bdf367f3f679..ea3ed9a17f1a 100644
> > --- a/drivers/crypto/caam/ctrl.c
> > +++ b/drivers/crypto/caam/ctrl.c
> > @@ -7,6 +7,7 @@
> >   */
> >  
> >  #include <linux/device.h>
> > +#include <linux/devm-helpers.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/platform_device.h>
> > @@ -604,11 +605,6 @@ static int init_clocks(struct device *dev, const struct caam_imx_data *data)
> >  	return devm_add_action_or_reset(dev, disable_clocks, ctrlpriv);
> >  }
> >  
> > -static void caam_remove_debugfs(void *root)
> > -{
> > -	debugfs_remove_recursive(root);
> > -}
> > -
> >  #ifdef CONFIG_FSL_MC_BUS
> >  static bool check_version(struct fsl_mc_version *mc_version, u32 major,
> >  			  u32 minor, u32 revision)
> > @@ -1058,13 +1054,9 @@ static int caam_probe(struct platform_device *pdev)
> >  	ctrlpriv->era = caam_get_era(perfmon);
> >  	ctrlpriv->domain = iommu_get_domain_for_dev(dev);
> >  
> > -	dfs_root = debugfs_create_dir(dev_name(dev), NULL);
> > -	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> > -		ret = devm_add_action_or_reset(dev, caam_remove_debugfs,
> > -					       dfs_root);
> > -		if (ret)
> > -			return ret;
> > -	}
> > +	dfs_root = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> > +	if (IS_ERR(dfs_root))
> > +		return PTR_ERR(dfs_root);
> >  
> >  	caam_debugfs_init(ctrlpriv, perfmon, dfs_root);
> >  
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index c5c9d8e0d88d..4b38514887a4 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -2,6 +2,7 @@
> >  /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> >  #include <linux/debugfs.h>
> >  #include <linux/device.h>
> > +#include <linux/devm-helpers.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> >  
> > @@ -30,11 +31,6 @@ static void enable_suspend(void *data)
> >  	cxl_mem_active_dec();
> >  }
> >  
> > -static void remove_debugfs(void *dentry)
> > -{
> > -	debugfs_remove_recursive(dentry);
> > -}
> > -
> >  static int cxl_mem_dpa_show(struct seq_file *file, void *data)
> >  {
> >  	struct device *dev = file->private;
> > @@ -138,7 +134,8 @@ static int cxl_mem_probe(struct device *dev)
> >  		debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
> >  				    &cxl_poison_clear_fops);
> >  
> > -	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
> > +	rc = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
> > +				      dentry);
> 
> This is probably the better fix for cxl:
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 3b64fb1b9ed0..3258427af032 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -57,7 +57,6 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s);
>  void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
>  				   resource_size_t length);
>  
> -struct dentry *cxl_debugfs_create_dir(const char *dir);
>  int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
>  		     enum cxl_decoder_mode mode);
>  int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size);
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 27166a411705..5c2db4791b8b 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1402,7 +1402,7 @@ void __init cxl_mbox_init(void)
>  {
>  	struct dentry *mbox_debugfs;
>  
> -	mbox_debugfs = cxl_debugfs_create_dir("mbox");
> +	mbox_debugfs = debugfs_create_dir("mbox", NULL);
>  	debugfs_create_bool("raw_allow_all", 0600, mbox_debugfs,
>  			    &cxl_raw_allow_all);
>  }
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e59d9d37aa65..82c6a1c6aff4 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -10,6 +10,7 @@
>  #include <linux/slab.h>
>  #include <linux/idr.h>
>  #include <linux/node.h>
> +#include <linux/devm-helpers.h>
>  #include <cxlmem.h>
>  #include <cxlpci.h>
>  #include <cxl.h>
> @@ -2207,13 +2208,7 @@ struct bus_type cxl_bus_type = {
>  };
>  EXPORT_SYMBOL_NS_GPL(cxl_bus_type, CXL);
>  
> -static struct dentry *cxl_debugfs;
> -
> -struct dentry *cxl_debugfs_create_dir(const char *dir)
> -{
> -	return debugfs_create_dir(dir, cxl_debugfs);
> -}
> -EXPORT_SYMBOL_NS_GPL(cxl_debugfs_create_dir, CXL);
> +struct dentry *cxl_debugfs;
>  
>  static __init int cxl_core_init(void)
>  {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index b6017c0c57b4..ca8399b24955 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -880,6 +880,8 @@ void cxl_switch_parse_cdat(struct cxl_port *port);
>  int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  				      struct access_coordinate *coord);
>  
> +extern struct dentry *cxl_debugfs;
> +
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
>   * of these symbols in tools/testing/cxl/.
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 5303d6942b88..b6f13ba87927 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -859,6 +859,5 @@ struct cxl_hdm {
>  };
>  
>  struct seq_file;
> -struct dentry *cxl_debugfs_create_dir(const char *dir);
>  void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
>  #endif /* __CXL_MEM_H__ */
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c5c9d8e0d88d..494abe7a54c5 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -4,6 +4,7 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/devm-helpers.h>
>  
>  #include "cxlmem.h"
>  #include "cxlpci.h"
> @@ -30,11 +31,6 @@ static void enable_suspend(void *data)
>  	cxl_mem_active_dec();
>  }
>  
> -static void remove_debugfs(void *dentry)
> -{
> -	debugfs_remove_recursive(dentry);
> -}
> -
>  static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>  {
>  	struct device *dev = file->private;
> @@ -128,7 +124,10 @@ static int cxl_mem_probe(struct device *dev)
>  	if (work_pending(&cxlmd->detach_work))
>  		return -EBUSY;
>  
> -	dentry = cxl_debugfs_create_dir(dev_name(dev));
> +	dentry = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
> +

No that loses the "cxl" prefix.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function
  2024-02-22 18:11     ` Dan Williams
@ 2024-02-22 18:25       ` Dan Williams
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Williams @ 2024-02-22 18:25 UTC (permalink / raw)
  To: Dan Williams, Dave Jiang, Marek Behún, linux-kernel,
	Hans de Goede, Matti Vaittinen
  Cc: Horia Geantă,
	Pankaj Gupta, Gaurav Jain, Herbert Xu, David S. Miller,
	Davidlohr Bueso, Jonathan Cameron, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Bamvor Jian Zhang,
	Linus Walleij, Bartosz Golaszewski, Douglas Anderson,
	Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, James Seo,
	Jean Delvare, Guenter Roeck, linux-crypto, linux-cxl, linux-gpio,
	dri-devel, linux-hwmon

Dan Williams wrote:
> Dave Jiang wrote:
> > 
> > 
> > On 2/22/24 7:58 AM, 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.
> > > 
> > > 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
> > > drivers
> > >   drivers/cxl/mem.c
> > >   drivers/gpio/gpio-mockup.c
> > > 
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > ---
> > >  drivers/crypto/caam/ctrl.c            | 16 +++------
> > >  drivers/cxl/mem.c                     |  9 ++---
> > >  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          | 48 +++++++++++++++++++++++++++
> > >  8 files changed, 72 insertions(+), 71 deletions(-)
> > > 
[..]
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 5303d6942b88..b6f13ba87927 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -859,6 +859,5 @@ struct cxl_hdm {
> >  };
> >  
> >  struct seq_file;
> > -struct dentry *cxl_debugfs_create_dir(const char *dir);
> >  void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
> >  #endif /* __CXL_MEM_H__ */
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index c5c9d8e0d88d..494abe7a54c5 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -4,6 +4,7 @@
> >  #include <linux/device.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > +#include <linux/devm-helpers.h>
> >  
> >  #include "cxlmem.h"
> >  #include "cxlpci.h"
> > @@ -30,11 +31,6 @@ static void enable_suspend(void *data)
> >  	cxl_mem_active_dec();
> >  }
> >  
> > -static void remove_debugfs(void *dentry)
> > -{
> > -	debugfs_remove_recursive(dentry);
> > -}
> > -
> >  static int cxl_mem_dpa_show(struct seq_file *file, void *data)
> >  {
> >  	struct device *dev = file->private;
> > @@ -128,7 +124,10 @@ static int cxl_mem_probe(struct device *dev)
> >  	if (work_pending(&cxlmd->detach_work))
> >  		return -EBUSY;
> >  
> > -	dentry = cxl_debugfs_create_dir(dev_name(dev));
> > +	dentry = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> > +	if (IS_ERR(dentry))
> > +		return PTR_ERR(dentry);
> > +
> 
> No that loses the "cxl" prefix.

So, to be clear, I do see the benefit of removing the
devm_add_action_or_reset() call altogether, but that work is a bit
deeper and should not be tied with all these other cleanups. So I think
from the CXL perspective, please drop these CXL changes out of this
patch. Follow up with a standalone patch, or leave it to drivers/cxl/
folks to create that deeper cleanup.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
  2024-02-22 16:44 ` Matthew Auld
@ 2024-02-22 18:33   ` Hans de Goede
  0 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2024-02-22 18:33 UTC (permalink / raw)
  To: Matthew Auld, Marek Behún, linux-kernel, Matti Vaittinen
  Cc: Linus Walleij, Bartosz Golaszewski, Lucas De Marchi, Oded Gabbay,
	Thomas Hellström, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Aleksandr Mezin,
	Jean Delvare, Guenter Roeck, Pavel Machek, Lee Jones,
	Sebastian Reichel, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-gpio, intel-xe, dri-devel, linux-hwmon, linux-leds,
	linux-pm, linux-arm-kernel, linux-mediatek

Hi,

On 2/22/24 17:44, Matthew Auld wrote:
> On 22/02/2024 14:58, Marek Behún wrote:
>> A few drivers are doing resource-managed mutex initialization by
>> implementing ad-hoc one-liner mutex dropping functions and using them
>> with devm_add_action_or_reset(). Help drivers avoid these repeated
>> one-liners by adding managed version of mutex initialization.

<snip>

>> index 74891802200d..70640fb96117 100644
>> --- a/include/linux/devm-helpers.h
>> +++ b/include/linux/devm-helpers.h
>> @@ -24,6 +24,8 @@
>>    */
>>     #include <linux/device.h>
>> +#include <linux/kconfig.h>
>> +#include <linux/mutex.h>
>>   #include <linux/workqueue.h>
>>     static inline void devm_delayed_work_drop(void *res)
>> @@ -76,4 +78,34 @@ static inline int devm_work_autocancel(struct device *dev,
>>       return devm_add_action(dev, devm_work_drop, w);
>>   }
>>   +static inline void devm_mutex_drop(void *res)
>> +{
>> +    mutex_destroy(res);
>> +}
>> +
>> +/**
>> + * devm_mutex_init - Resource managed mutex initialization
>> + * @dev:    Device which lifetime mutex is bound to
>> + * @lock:    Mutex to be initialized (and automatically destroyed)
>> + *
>> + * Initialize mutex which is automatically destroyed when driver is detached.
>> + * A few drivers initialize mutexes which they want destroyed before driver is
>> + * detached, for debugging purposes.
>> + * devm_mutex_init() can be used to omit the explicit mutex_destroy() call when
>> + * driver is detached.
>> + */
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +    mutex_init(lock);
> 
> Do you know if this this needs __always_inline? The static lockdep key in mutex_init() should be
> different for each caller class. See c21f11d182c2 ("drm: fix drmm_mutex_init()").

That is a very good point. I believe that this should mirror mutex_init() and
the actual static inline function should be __devm_mutex_init() which takes
the key as extra argument (and calls __mutex_init()) and then make
devm_mutex_init() itself a macro mirroring the mutex_init() macro.

Regards,

Hans






> 
>> +
>> +    /*
>> +     * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
>> +     * disabled. No need to allocate an action in that case.
>> +     */
>> +    if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
>> +        return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
>> +    else
>> +        return 0;
>> +}
>> +
>>   #endif
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
  2024-02-22 14:58 [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Marek Behún
                   ` (3 preceding siblings ...)
  2024-02-22 16:44 ` Matthew Auld
@ 2024-02-22 21:42 ` andy.shevchenko
  2024-02-23 12:26   ` Marek Behún
  4 siblings, 1 reply; 14+ messages in thread
From: andy.shevchenko @ 2024-02-22 21:42 UTC (permalink / raw)
  To: Marek Behún
  Cc: linux-kernel, Hans de Goede, Matti Vaittinen, Linus Walleij,
	Bartosz Golaszewski, Lucas De Marchi, Oded Gabbay,
	Thomas Hellström, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Aleksandr Mezin,
	Jean Delvare, Guenter Roeck, Pavel Machek, Lee Jones,
	Sebastian Reichel, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-gpio, intel-xe, dri-devel, linux-hwmon, linux-leds,
	linux-pm, linux-arm-kernel, linux-mediatek, George Stark

Thu, Feb 22, 2024 at 03:58:37PM +0100, Marek Behún kirjoitti:
> A few drivers are doing resource-managed mutex initialization by
> implementing ad-hoc one-liner mutex dropping functions and using them
> with devm_add_action_or_reset(). Help drivers avoid these repeated
> one-liners by adding managed version of mutex initialization.
> 
> Use the new function devm_mutex_init() in the following drivers:
>   drivers/gpio/gpio-pisosr.c
>   drivers/gpio/gpio-sim.c
>   drivers/gpu/drm/xe/xe_hwmon.c
>   drivers/hwmon/nzxt-smart2.c
>   drivers/leds/leds-is31fl319x.c
>   drivers/power/supply/mt6370-charger.c
>   drivers/power/supply/rt9467-charger.c

Pardon me, but why?

https://lore.kernel.org/linux-leds/20231214173614.2820929-1-gnstark@salutedevices.com/

Can you cooperate, folks, instead of doing something independently?


> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,8 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/kconfig.h>
> +#include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  
>  static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +78,34 @@ static inline int devm_work_autocancel(struct device *dev,
>  	return devm_add_action(dev, devm_work_drop, w);
>  }
>  
> +static inline void devm_mutex_drop(void *res)
> +{
> +	mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource managed mutex initialization
> + * @dev:	Device which lifetime mutex is bound to
> + * @lock:	Mutex to be initialized (and automatically destroyed)
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.
> + * A few drivers initialize mutexes which they want destroyed before driver is
> + * detached, for debugging purposes.
> + * devm_mutex_init() can be used to omit the explicit mutex_destroy() call when
> + * driver is detached.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +
> +	/*
> +	 * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
> +	 * disabled. No need to allocate an action in that case.
> +	 */
> +	if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
> +		return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
> +	else
> +		return 0;
> +}

Cc: George Stark <gnstark@salutedevices.com>

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
  2024-02-22 21:42 ` andy.shevchenko
@ 2024-02-23 12:26   ` Marek Behún
  2024-02-28  0:27     ` George Stark
  0 siblings, 1 reply; 14+ messages in thread
From: Marek Behún @ 2024-02-23 12:26 UTC (permalink / raw)
  To: andy.shevchenko, George Stark
  Cc: linux-kernel, Hans de Goede, Matti Vaittinen, Linus Walleij,
	Bartosz Golaszewski, Lucas De Marchi, Oded Gabbay,
	Thomas Hellström, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Aleksandr Mezin,
	Jean Delvare, Guenter Roeck, Pavel Machek, Lee Jones,
	Sebastian Reichel, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-gpio, intel-xe, dri-devel, linux-hwmon, linux-leds,
	linux-pm, linux-arm-kernel, linux-mediatek

On Thu, 22 Feb 2024 23:42:11 +0200
andy.shevchenko@gmail.com wrote:

> Thu, Feb 22, 2024 at 03:58:37PM +0100, Marek Behún kirjoitti:
> > A few drivers are doing resource-managed mutex initialization by
> > implementing ad-hoc one-liner mutex dropping functions and using them
> > with devm_add_action_or_reset(). Help drivers avoid these repeated
> > one-liners by adding managed version of mutex initialization.
> > 
> > Use the new function devm_mutex_init() in the following drivers:
> >   drivers/gpio/gpio-pisosr.c
> >   drivers/gpio/gpio-sim.c
> >   drivers/gpu/drm/xe/xe_hwmon.c
> >   drivers/hwmon/nzxt-smart2.c
> >   drivers/leds/leds-is31fl319x.c
> >   drivers/power/supply/mt6370-charger.c
> >   drivers/power/supply/rt9467-charger.c  
> 
> Pardon me, but why?
> 
> https://lore.kernel.org/linux-leds/20231214173614.2820929-1-gnstark@salutedevices.com/
> 
> Can you cooperate, folks, instead of doing something independently?

Thanks Andy for pointing to George's patch series.

I can drop the mutex_init() part and add just the debugfs part.

Marek

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
  2024-02-23 12:26   ` Marek Behún
@ 2024-02-28  0:27     ` George Stark
  0 siblings, 0 replies; 14+ messages in thread
From: George Stark @ 2024-02-28  0:27 UTC (permalink / raw)
  To: andy.shevchenko, Marek Behún
  Cc: linux-kernel, Hans de Goede, Matti Vaittinen, Linus Walleij,
	Bartosz Golaszewski, Lucas De Marchi, Oded Gabbay,
	Thomas Hellström, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Aleksandr Mezin,
	Jean Delvare, Guenter Roeck, Pavel Machek, Lee Jones,
	Sebastian Reichel, Matthias Brugger, AngeloGioacchino Del Regno,
	linux-gpio, intel-xe, dri-devel, linux-hwmon, linux-leds,
	linux-pm, linux-arm-kernel, linux-mediatek, kernel


On 2/23/24 15:26, Marek Behún wrote:
> On Thu, 22 Feb 2024 23:42:11 +0200
> andy.shevchenko@gmail.com wrote:
> 
>> Thu, Feb 22, 2024 at 03:58:37PM +0100, Marek Behún kirjoitti:
>>> A few drivers are doing resource-managed mutex initialization by
>>> implementing ad-hoc one-liner mutex dropping functions and using them
>>> with devm_add_action_or_reset(). Help drivers avoid these repeated
>>> one-liners by adding managed version of mutex initialization.
>>>
>>> Use the new function devm_mutex_init() in the following drivers:
>>>    drivers/gpio/gpio-pisosr.c
>>>    drivers/gpio/gpio-sim.c
>>>    drivers/gpu/drm/xe/xe_hwmon.c
>>>    drivers/hwmon/nzxt-smart2.c
>>>    drivers/leds/leds-is31fl319x.c
>>>    drivers/power/supply/mt6370-charger.c
>>>    drivers/power/supply/rt9467-charger.c
>>
>> Pardon me, but why?
>>
>> https://lore.kernel.org/linux-leds/20231214173614.2820929-1-gnstark@salutedevices.com/
>>
>> Can you cooperate, folks, instead of doing something independently?

Hello Andy

Thanks for pointing to my patch series
> 
> Thanks Andy for pointing to George's patch series.
> 
> I can drop the mutex_init() part and add just the debugfs part.

Hello Marek

I started to propose devm_mutex_init in December 2023. We tried to put 
it in devm-helpers.h firstly then we came to conclusion that 
linux/mutex.h would be a better place for it. Now I'm waiting for this 
series [1] to be merged because my patch depends on it. I'll let you 
know when I have an update.

[1] 
https://lore.kernel.org/lkml/20240222150540.79981-2-longman@redhat.com/T/
> 
> Marek

-- 
Best regards
George

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-02-28  0:27 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-22 14:58 [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Marek Behún
2024-02-22 14:58 ` [PATCH 2/2] devm-helpers: Add resource managed version of debugfs directory create function Marek Behún
2024-02-22 15:23   ` Guenter Roeck
2024-02-22 17:23   ` Dave Jiang
2024-02-22 18:11     ` Dan Williams
2024-02-22 18:25       ` Dan Williams
2024-02-22 15:11 ` [PATCH 1/2] devm-helpers: Add resource managed version of mutex init Bartosz Golaszewski
2024-02-22 15:24 ` Guenter Roeck
2024-02-22 16:03   ` Guenter Roeck
2024-02-22 16:44 ` Matthew Auld
2024-02-22 18:33   ` Hans de Goede
2024-02-22 21:42 ` andy.shevchenko
2024-02-23 12:26   ` Marek Behún
2024-02-28  0:27     ` George Stark

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).