2024-02-22 14:59:21

by Marek Behún

[permalink] [raw]
Subject: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init

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 <[email protected]>
---
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



2024-02-22 15:25:07

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init

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 <[email protected]>
> ---
> 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


2024-02-22 16:03:24

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init

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
>
>


2024-02-22 16:44:27

by Matthew Auld

[permalink] [raw]
Subject: Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init

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 <[email protected]>
> ---
> 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

2024-02-22 18:41:51

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init

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
>


2024-02-23 12:27:04

by Marek Behún

[permalink] [raw]
Subject: Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init

On Thu, 22 Feb 2024 23:42:11 +0200
[email protected] 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/[email protected]/
>
> 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

2024-02-28 00:36:38

by George Stark

[permalink] [raw]
Subject: Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init


On 2/23/24 15:26, Marek Behún wrote:
> On Thu, 22 Feb 2024 23:42:11 +0200
> [email protected] 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/[email protected]/
>>
>> 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/[email protected]/T/
>
> Marek

--
Best regards
George

2024-02-22 21:42:33

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init

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/[email protected]/

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 <[email protected]>

--
With Best Regards,
Andy Shevchenko



2024-02-22 15:11:55

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init

On Thu, Feb 22, 2024 at 3:58 PM Marek Behún <[email protected]> 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 <[email protected]>