This patch series fixes the problem of devm_led_classdev_register misusing.
The basic problem is described in [1]. Shortly when devm_led_classdev_register()
is used then led_classdev_unregister() called after driver's remove() callback.
led_classdev_unregister() calls driver's brightness_set callback and that callback
may use resources which were destroyed already in driver's remove().
After discussion with maintainers [2] [3] we decided:
1) don't touch led subsytem core code and don't remove led_set_brightness() from it
but fix drivers
2) don't use devm_led_classdev_unregister
So the solution is to use devm wrappers for all resources
driver's brightness_set() depends on. And introduce dedicated devm wrapper
for mutex as it's often used resource.
[1] https://lore.kernel.org/lkml/[email protected]/T/
[2] https://lore.kernel.org/lkml/[email protected]/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
[3] https://lore.kernel.org/lkml/[email protected]/T/#mdbf572a85c33f869a553caf986b6228bb65c8383
Changelog:
v1->v2:
revise patch series completely
v2->v3:
locking: add define if mutex_destroy() is not an empty function
new patch, discussed here [8]
devm-helpers: introduce devm_mutex_init
previous version [4]
- revise code based on mutex_destroy define
- update commit message
- update devm_mutex_init()'s description
leds: aw2013: unlock mutex before destroying it
previous version [5]
- make this patch first in the series
- add tags Fixes and RvB by Andy
leds: aw2013: use devm API to cleanup module's resources
previous version [6]
- make aw2013_chip_disable_action()'s body oneline
- don't shadow devm_mutex_init() return code
leds: aw200xx: use devm API to cleanup module's resources
previous version [7]
- make aw200xx_*_action()'s bodies oneline
- don't shadow devm_mutex_init() return code
leds: lm3532: use devm API to cleanup module's resources
leds: nic78bx: use devm API to cleanup module's resources
leds: mlxreg: use devm_mutex_init for mutex initializtion
leds: an30259a: use devm_mutext_init for mutext initialization
leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
- those pathes were planned but not sent in the series #2 due to mail server
problem on my side. I revised them according to the comments.
[4] https://lore.kernel.org/lkml/[email protected]/T/#mf500af0eda2a9ffc95594607dbe4cb64f2e3c9a8
[5] https://lore.kernel.org/lkml/[email protected]/T/#mc92df4fb4f7d4187fb01cc1144acfa5fb5230dd2
[6] https://lore.kernel.org/lkml/[email protected]/T/#m300df89710c43cc2ab598baa16c68dd0a0d7d681
[7] https://lore.kernel.org/lkml/[email protected]/T/#m8e5c65e0c6b137c91fa00bb9320ad581164d1d0b
[8] https://lore.kernel.org/lkml/[email protected]/T/#m5f84a4a2f387d49678783e652b9e658e02c27450
George Stark (11):
leds: aw2013: unlock mutex before destroying it
locking: add define if mutex_destroy() is not an empty function
devm-helpers: introduce devm_mutex_init
leds: aw2013: use devm API to cleanup module's resources
leds: aw200xx: use devm API to cleanup module's resources
leds: lp3952: use devm API to cleanup module's resources
leds: lm3532: use devm API to cleanup module's resources
leds: nic78bx: use devm API to cleanup module's resources
leds: mlxreg: use devm_mutex_init for mutex initializtion
leds: an30259a: use devm_mutext_init for mutext initialization
leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds
drivers/leds/leds-an30259a.c | 15 +++++----------
drivers/leds/leds-aw200xx.c | 33 ++++++++++++++++++++++-----------
drivers/leds/leds-aw2013.c | 27 +++++++++++++++------------
drivers/leds/leds-lm3532.c | 30 ++++++++++++++++++------------
drivers/leds/leds-lp3952.c | 21 +++++++++++----------
drivers/leds/leds-mlxreg.c | 17 ++++++-----------
drivers/leds/leds-nic78bx.c | 25 +++++++++++++------------
drivers/leds/leds-powernv.c | 23 ++++++++---------------
include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
include/linux/mutex.h | 3 +++
10 files changed, 128 insertions(+), 93 deletions(-)
--
2.25.1
In the probe() callback in case of error mutex is destroyed being locked
which is not allowed so unlock the mutex before destroying.
Fixes: 59ea3c9faf32 ("leds: add aw2013 driver")
Signed-off-by: George Stark <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/leds/leds-aw2013.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 59765640b70f..c2bc0782c0cd 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -397,6 +397,7 @@ static int aw2013_probe(struct i2c_client *client)
regulator_disable(chip->vcc_regulator);
error:
+ mutex_unlock(&chip->mutex);
mutex_destroy(&chip->mutex);
return ret;
}
--
2.25.1
Using of devm API leads to a certain order of releasing resources.
So all dependent resources which are not devm-wrapped should be deleted
with respect to devm-release order. Mutex is one of such objects that
often is bound to other resources and has no own devm wrapper.
Since mutex_destroy() actually does nothing in non-debug builds
frequently calling mutex_destroy() is just ignored which is safe for now
but wrong formally and can lead to a problem if mutex_destroy() is
extended so introduce devm_mutex_init().
Signed-off-by: George Stark <[email protected]>
---
include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
index 74891802200d..4043c3481d2e 100644
--- a/include/linux/devm-helpers.h
+++ b/include/linux/devm-helpers.h
@@ -24,6 +24,7 @@
*/
#include <linux/device.h>
+#include <linux/mutex.h>
#include <linux/workqueue.h>
static inline void devm_delayed_work_drop(void *res)
@@ -76,4 +77,30 @@ static inline int devm_work_autocancel(struct device *dev,
return devm_add_action(dev, devm_work_drop, w);
}
+#ifdef mutex_destroy
+static inline void devm_mutex_release(void *res)
+{
+ mutex_destroy(res);
+}
+#endif
+
+/**
+ * devm_mutex_init - Resource-managed mutex initialization
+ * @dev: Device which lifetime mutex is bound to
+ * @lock: Pointer to a mutex
+ *
+ * Initialize mutex which is automatically destroyed when the driver is detached.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+ mutex_init(lock);
+#ifdef mutex_destroy
+ return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+#else
+ return 0;
+#endif
+}
+
#endif
--
2.25.1
In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().
Signed-off-by: George Stark <[email protected]>
---
drivers/leds/leds-aw2013.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index c2bc0782c0cd..863aeb02f278 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0+
// Driver for Awinic AW2013 3-channel LED driver
+#include <linux/devm-helpers.h>
#include <linux/i2c.h>
#include <linux/leds.h>
#include <linux/module.h>
@@ -318,6 +319,11 @@ static int aw2013_probe_dt(struct aw2013 *chip)
return 0;
}
+static void aw2013_chip_disable_action(void *data)
+{
+ aw2013_chip_disable(data);
+}
+
static const struct regmap_config aw2013_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
@@ -334,7 +340,10 @@ static int aw2013_probe(struct i2c_client *client)
if (!chip)
return -ENOMEM;
- mutex_init(&chip->mutex);
+ ret = devm_mutex_init(&client->dev, &chip->mutex);
+ if (ret)
+ return ret;
+
mutex_lock(&chip->mutex);
chip->client = client;
@@ -378,6 +387,10 @@ static int aw2013_probe(struct i2c_client *client)
goto error_reg;
}
+ ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
+ if (ret)
+ goto error_reg;
+
ret = aw2013_probe_dt(chip);
if (ret < 0)
goto error_reg;
@@ -398,19 +411,9 @@ static int aw2013_probe(struct i2c_client *client)
error:
mutex_unlock(&chip->mutex);
- mutex_destroy(&chip->mutex);
return ret;
}
-static void aw2013_remove(struct i2c_client *client)
-{
- struct aw2013 *chip = i2c_get_clientdata(client);
-
- aw2013_chip_disable(chip);
-
- mutex_destroy(&chip->mutex);
-}
-
static const struct of_device_id aw2013_match_table[] = {
{ .compatible = "awinic,aw2013", },
{ /* sentinel */ },
@@ -424,7 +427,6 @@ static struct i2c_driver aw2013_driver = {
.of_match_table = of_match_ptr(aw2013_match_table),
},
.probe = aw2013_probe,
- .remove = aw2013_remove,
};
module_i2c_driver(aw2013_driver);
--
2.25.1
mutex_destroy() is only a debug helper and an empty function on non-debug
configurations still we can't legally ignore it because it's the
established API call and it can be extended theoretically in the future.
Sometimes it could be useful to know e.g. in the higher-level API if
mutex_destroy() really does something in the current configuration
and it's should be called or skipped otherwise for the sake of
optimization so add dedicated define to recognize these cases.
Signed-off-by: George Stark <[email protected]>
---
include/linux/mutex.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index a33aa9eb9fc3..2395ce4fcaf6 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -83,6 +83,9 @@ struct mutex {
extern void mutex_destroy(struct mutex *lock);
+/* mutex_destroy() is a real function, not a NOP */
+#define mutex_destroy mutex_destroy
+
#else
# define __DEBUG_MUTEX_INITIALIZER(lockname)
--
2.25.1
On Thu, Dec 14, 2023 at 12:30 AM George Stark <[email protected]> wrote:
>
> Using of devm API leads to a certain order of releasing resources.
> So all dependent resources which are not devm-wrapped should be deleted
> with respect to devm-release order. Mutex is one of such objects that
> often is bound to other resources and has no own devm wrapper.
> Since mutex_destroy() actually does nothing in non-debug builds
> frequently calling mutex_destroy() is just ignored which is safe for now
> but wrong formally and can lead to a problem if mutex_destroy() is
> extended so introduce devm_mutex_init().
...
> +#ifdef mutex_destroy
> +static inline void devm_mutex_release(void *res)
> +{
> + mutex_destroy(res);
> +}
> +#endif
> +
> +/**
> + * devm_mutex_init - Resource-managed mutex initialization
> + * @dev: Device which lifetime mutex is bound to
> + * @lock: Pointer to a mutex
> + *
> + * Initialize mutex which is automatically destroyed when the driver is detached.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> + mutex_init(lock);
> +#ifdef mutex_destroy
> + return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +#else
> + return 0;
> +#endif
> +}
If this is going to be accepted, you may decrease the amount of ifdeffery.
#ifdef ...
#else
#define devm_mutex_init(dev, lock) mutex_init(lock)
#endif
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 14, 2023 at 12:36 AM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Dec 14, 2023 at 12:30 AM George Stark <[email protected]> wrote:
> >
> > Using of devm API leads to a certain order of releasing resources.
> > So all dependent resources which are not devm-wrapped should be deleted
> > with respect to devm-release order. Mutex is one of such objects that
> > often is bound to other resources and has no own devm wrapper.
> > Since mutex_destroy() actually does nothing in non-debug builds
> > frequently calling mutex_destroy() is just ignored which is safe for now
> > but wrong formally and can lead to a problem if mutex_destroy() is
> > extended so introduce devm_mutex_init().
...
> > +#ifdef mutex_destroy
> > +static inline void devm_mutex_release(void *res)
> > +{
> > + mutex_destroy(res);
> > +}
> > +#endif
> > +
> > +/**
> > + * devm_mutex_init - Resource-managed mutex initialization
> > + * @dev: Device which lifetime mutex is bound to
> > + * @lock: Pointer to a mutex
> > + *
> > + * Initialize mutex which is automatically destroyed when the driver is detached.
> > + *
> > + * Returns: 0 on success or a negative error code on failure.
> > + */
> > +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> > +{
> > + mutex_init(lock);
> > +#ifdef mutex_destroy
> > + return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> > +#else
> > + return 0;
> > +#endif
> > +}
>
> If this is going to be accepted, you may decrease the amount of ifdeffery.
>
> #ifdef ...
> #else
> #define devm_mutex_init(dev, lock) mutex_init(lock)
More precisely ({ mutex_init(lock); 0; }) or as a static inline...
> #endif
--
With Best Regards,
Andy Shevchenko
On 12/13/23 17:30, George Stark wrote:
> mutex_destroy() is only a debug helper and an empty function on non-debug
> configurations still we can't legally ignore it because it's the
> established API call and it can be extended theoretically in the future.
> Sometimes it could be useful to know e.g. in the higher-level API if
> mutex_destroy() really does something in the current configuration
> and it's should be called or skipped otherwise for the sake of
> optimization so add dedicated define to recognize these cases.
>
> Signed-off-by: George Stark <[email protected]>
> ---
> include/linux/mutex.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index a33aa9eb9fc3..2395ce4fcaf6 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -83,6 +83,9 @@ struct mutex {
>
> extern void mutex_destroy(struct mutex *lock);
>
> +/* mutex_destroy() is a real function, not a NOP */
> +#define mutex_destroy mutex_destroy
> +
> #else
>
> # define __DEBUG_MUTEX_INITIALIZER(lockname)
Acked-by: Waiman Long <[email protected]>
George Stark писал(а) 14.12.2023 03:30:
> In this driver LEDs are registered using devm_led_classdev_register()
> so they are automatically unregistered after module's remove() is done.
> led_classdev_unregister() calls module's led_set_brightness() to turn off
> the LEDs and that callback uses resources which were destroyed already
> in module's remove() so use devm API instead of remove().
>
> Signed-off-by: George Stark <[email protected]>
Thanks for noticing and fixing this!
Perhaps this patch needs a Fixes tag too, like 1/11?
Tested-by: Nikita Travkin <[email protected]>
Btw, seems like (5..11)/11 never arrived to the lists...
Nikita
> ---
> drivers/leds/leds-aw2013.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index c2bc0782c0cd..863aeb02f278 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0+
> // Driver for Awinic AW2013 3-channel LED driver
>
> +#include <linux/devm-helpers.h>
> #include <linux/i2c.h>
> #include <linux/leds.h>
> #include <linux/module.h>
> @@ -318,6 +319,11 @@ static int aw2013_probe_dt(struct aw2013 *chip)
> return 0;
> }
>
> +static void aw2013_chip_disable_action(void *data)
> +{
> + aw2013_chip_disable(data);
> +}
> +
> static const struct regmap_config aw2013_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> @@ -334,7 +340,10 @@ static int aw2013_probe(struct i2c_client *client)
> if (!chip)
> return -ENOMEM;
>
> - mutex_init(&chip->mutex);
> + ret = devm_mutex_init(&client->dev, &chip->mutex);
> + if (ret)
> + return ret;
> +
> mutex_lock(&chip->mutex);
>
> chip->client = client;
> @@ -378,6 +387,10 @@ static int aw2013_probe(struct i2c_client *client)
> goto error_reg;
> }
>
> + ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
> + if (ret)
> + goto error_reg;
> +
> ret = aw2013_probe_dt(chip);
> if (ret < 0)
> goto error_reg;
> @@ -398,19 +411,9 @@ static int aw2013_probe(struct i2c_client *client)
>
> error:
> mutex_unlock(&chip->mutex);
> - mutex_destroy(&chip->mutex);
> return ret;
> }
>
> -static void aw2013_remove(struct i2c_client *client)
> -{
> - struct aw2013 *chip = i2c_get_clientdata(client);
> -
> - aw2013_chip_disable(chip);
> -
> - mutex_destroy(&chip->mutex);
> -}
> -
> static const struct of_device_id aw2013_match_table[] = {
> { .compatible = "awinic,aw2013", },
> { /* sentinel */ },
> @@ -424,7 +427,6 @@ static struct i2c_driver aw2013_driver = {
> .of_match_table = of_match_ptr(aw2013_match_table),
> },
> .probe = aw2013_probe,
> - .remove = aw2013_remove,
> };
>
> module_i2c_driver(aw2013_driver);
Hi,
On 12/13/23 23:38, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 12:36 AM Andy Shevchenko
> <[email protected]> wrote:
>> On Thu, Dec 14, 2023 at 12:30 AM George Stark <[email protected]> wrote:
>>>
>>> Using of devm API leads to a certain order of releasing resources.
>>> So all dependent resources which are not devm-wrapped should be deleted
>>> with respect to devm-release order. Mutex is one of such objects that
>>> often is bound to other resources and has no own devm wrapper.
>>> Since mutex_destroy() actually does nothing in non-debug builds
>>> frequently calling mutex_destroy() is just ignored which is safe for now
>>> but wrong formally and can lead to a problem if mutex_destroy() is
>>> extended so introduce devm_mutex_init().
>
> ...
>
>>> +#ifdef mutex_destroy
>>> +static inline void devm_mutex_release(void *res)
>>> +{
>>> + mutex_destroy(res);
>>> +}
>>> +#endif
>>> +
>>> +/**
>>> + * devm_mutex_init - Resource-managed mutex initialization
>>> + * @dev: Device which lifetime mutex is bound to
>>> + * @lock: Pointer to a mutex
>>> + *
>>> + * Initialize mutex which is automatically destroyed when the driver is detached.
>>> + *
>>> + * Returns: 0 on success or a negative error code on failure.
>>> + */
>>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>>> +{
>>> + mutex_init(lock);
>>> +#ifdef mutex_destroy
>>> + return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>> +#else
>>> + return 0;
>>> +#endif
>>> +}
>>
>> If this is going to be accepted, you may decrease the amount of ifdeffery.
>>
>> #ifdef ...
>> #else
>> #define devm_mutex_init(dev, lock) mutex_init(lock)
>
> More precisely ({ mutex_init(lock); 0; }) or as a static inline...
With a static inline we are pretty much back to the original
v3 patch.
I believe the best way to reduce the ifdef-ery is to remove
the #ifdef around devm_mutex_release() having unused
static inline ... functions in .h files is quite common,
so this one does not need a #ifdef around it and with
that removed we are down to just one #ifdef so just
removing the #ifdef around devm_mutex_release() seems
the best fix.
With that fixed you may add my:
Reviewed-by: Hans de Goede <[email protected]>
to the patch and I'm fine with this being routed
upstream through whatever tree is convenient.
Regards,
Hans
Le 13/12/2023 à 23:30, George Stark a écrit :
> [Vous ne recevez pas souvent de courriers de [email protected]. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>
> Using of devm API leads to a certain order of releasing resources.
> So all dependent resources which are not devm-wrapped should be deleted
> with respect to devm-release order. Mutex is one of such objects that
> often is bound to other resources and has no own devm wrapper.
> Since mutex_destroy() actually does nothing in non-debug builds
> frequently calling mutex_destroy() is just ignored which is safe for now
> but wrong formally and can lead to a problem if mutex_destroy() is
> extended so introduce devm_mutex_init().
So you abandonned the idea of using mutex.h ?
I can't see the point to spread mutex functions into devm-helpers.h
Adding a mutex_destroy macro for this purpose looks odd. And if someone
defines a new version of mutex_destroy() and forget the macro, it will
go undetected.
Usually macros of that type serve the purpose of defining a fallback
when the macro is not defined. In that case, when someone adds a new
version without defining the macro, it gets detected because if
conflicts with the fallback.
But in your case it works the other way round, so I will just go undetected.
For me the best solution remains to use mutex.h and have
devm_mutex_init() defined or declared at the same place as mutex_destroy().
>
> Signed-off-by: George Stark <[email protected]>
> ---
> include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..4043c3481d2e 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,7 @@
> */
>
> #include <linux/device.h>
> +#include <linux/mutex.h>
> #include <linux/workqueue.h>
>
> static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +77,30 @@ static inline int devm_work_autocancel(struct device *dev,
> return devm_add_action(dev, devm_work_drop, w);
> }
>
> +#ifdef mutex_destroy
> +static inline void devm_mutex_release(void *res)
> +{
> + mutex_destroy(res);
> +}
> +#endif
> +
> +/**
> + * devm_mutex_init - Resource-managed mutex initialization
> + * @dev: Device which lifetime mutex is bound to
> + * @lock: Pointer to a mutex
> + *
> + * Initialize mutex which is automatically destroyed when the driver is detached.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> + mutex_init(lock);
> +#ifdef mutex_destroy
> + return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +#else
> + return 0;
> +#endif
> +}
> +
> #endif
> --
> 2.25.1
>
Hi,
On 12/14/23 11:06, Christophe Leroy wrote:
>
>
> Le 13/12/2023 à 23:30, George Stark a écrit :
>> [Vous ne recevez pas souvent de courriers de [email protected]. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Using of devm API leads to a certain order of releasing resources.
>> So all dependent resources which are not devm-wrapped should be deleted
>> with respect to devm-release order. Mutex is one of such objects that
>> often is bound to other resources and has no own devm wrapper.
>> Since mutex_destroy() actually does nothing in non-debug builds
>> frequently calling mutex_destroy() is just ignored which is safe for now
>> but wrong formally and can lead to a problem if mutex_destroy() is
>> extended so introduce devm_mutex_init().
>
> So you abandonned the idea of using mutex.h ?
>
> I can't see the point to spread mutex functions into devm-helpers.h
>
> Adding a mutex_destroy macro for this purpose looks odd. And if someone
> defines a new version of mutex_destroy() and forget the macro, it will
> go undetected.
>
> Usually macros of that type serve the purpose of defining a fallback
> when the macro is not defined. In that case, when someone adds a new
> version without defining the macro, it gets detected because if
> conflicts with the fallback.
> But in your case it works the other way round, so I will just go undetected.
>
> For me the best solution remains to use mutex.h and have
> devm_mutex_init() defined or declared at the same place as mutex_destroy().
FWIW defining devm_mutex_init() in mutex.h is fine
with me and makes sense to me. I also agree that putting
it there would be better if that is acceptable for
the mutex maintainers.
devm-helpers.h is there for helpers which don't fit
in another place.
Regards,
Hans
>
>
>>
>> Signed-off-by: George Stark <[email protected]>
>> ---
>> include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
>> index 74891802200d..4043c3481d2e 100644
>> --- a/include/linux/devm-helpers.h
>> +++ b/include/linux/devm-helpers.h
>> @@ -24,6 +24,7 @@
>> */
>>
>> #include <linux/device.h>
>> +#include <linux/mutex.h>
>> #include <linux/workqueue.h>
>>
>> static inline void devm_delayed_work_drop(void *res)
>> @@ -76,4 +77,30 @@ static inline int devm_work_autocancel(struct device *dev,
>> return devm_add_action(dev, devm_work_drop, w);
>> }
>>
>> +#ifdef mutex_destroy
>> +static inline void devm_mutex_release(void *res)
>> +{
>> + mutex_destroy(res);
>> +}
>> +#endif
>> +
>> +/**
>> + * devm_mutex_init - Resource-managed mutex initialization
>> + * @dev: Device which lifetime mutex is bound to
>> + * @lock: Pointer to a mutex
>> + *
>> + * Initialize mutex which is automatically destroyed when the driver is detached.
>> + *
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> + mutex_init(lock);
>> +#ifdef mutex_destroy
>> + return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>> +#else
>> + return 0;
>> +#endif
>> +}
>> +
>> #endif
>> --
>> 2.25.1
>>
Hello Christophe
On 12/14/23 13:06, Christophe Leroy wrote:
>
>
...
>
> So you abandonned the idea of using mutex.h ?
I'm not the one who make a choice here. The patch [1] you're talking
about was seen by everyone but it seems like no one had shown interest.
For me personally approach with #define mutex_destroy is not very usual
but if even slight mixing device with mutex.h is unacceptable what else
can we do? Avoiding the need to allocate devm slot for empty
mutex_destroy is more important.
Should I make series #4 with the patch [1] to give it a last chance?
[1]
https://lore.kernel.org/lkml/[email protected]/T/#m3f6df30ffccaccb1df4669a327f349164f572931
> I can't see the point to spread mutex functions into devm-helpers.h
>
> Adding a mutex_destroy macro for this purpose looks odd. And if someone
> defines a new version of mutex_destroy() and forget the macro, it will
> go undetected.
>
> Usually macros of that type serve the purpose of defining a fallback
> when the macro is not defined. In that case, when someone adds a new
> version without defining the macro, it gets detected because if
> conflicts with the fallback.
> But in your case it works the other way round, so I will just go undetected.
>
> For me the best solution remains to use mutex.h and have
> devm_mutex_init() defined or declared at the same place as mutex_destroy().
>
>
>>
>> Signed-off-by: George Stark <[email protected]>
>> ---
>> include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
>> index 74891802200d..4043c3481d2e 100644
>> --- a/include/linux/devm-helpers.h
>> +++ b/include/linux/devm-helpers.h
>> @@ -24,6 +24,7 @@
>> */
>>
>> #include <linux/device.h>
>> +#include <linux/mutex.h>
>> #include <linux/workqueue.h>
>>
>> static inline void devm_delayed_work_drop(void *res)
>> @@ -76,4 +77,30 @@ static inline int devm_work_autocancel(struct device *dev,
>> return devm_add_action(dev, devm_work_drop, w);
>> }
>>
>> +#ifdef mutex_destroy
>> +static inline void devm_mutex_release(void *res)
>> +{
>> + mutex_destroy(res);
>> +}
>> +#endif
>> +
>> +/**
>> + * devm_mutex_init - Resource-managed mutex initialization
>> + * @dev: Device which lifetime mutex is bound to
>> + * @lock: Pointer to a mutex
>> + *
>> + * Initialize mutex which is automatically destroyed when the driver is detached.
>> + *
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> + mutex_init(lock);
>> +#ifdef mutex_destroy
>> + return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>> +#else
>> + return 0;
>> +#endif
>> +}
>> +
>> #endif
>> --
>> 2.25.1
>>
--
Best regards
George
Hello Nikita
Thanks for the review and testing.
On 12/14/23 08:42, Nikita Travkin wrote:
> George Stark писал(а) 14.12.2023 03:30:
>> In this driver LEDs are registered using devm_led_classdev_register()
>> so they are automatically unregistered after module's remove() is done.
>> led_classdev_unregister() calls module's led_set_brightness() to turn off
>> the LEDs and that callback uses resources which were destroyed already
>> in module's remove() so use devm API instead of remove().
>>
>> Signed-off-by: George Stark <[email protected]>
>
> Thanks for noticing and fixing this!
> Perhaps this patch needs a Fixes tag too, like 1/11?
This patch and the rest of the series depend on new feature
devm_mutex_init and if I add Fixes tag this feature will need to be
backported too along with fixes. I'm not sure it's desirable.
>
> Tested-by: Nikita Travkin <[email protected]>
>
> Btw, seems like (5..11)/11 never arrived to the lists...
Yeah there was a delay, but now all patches from series #3 are online.
>
> Nikita
>
>> ---
...
--
Best regards
George
Le 14/12/2023 à 13:48, George Stark a écrit :
> [Vous ne recevez pas souvent de courriers de [email protected].
> Découvrez pourquoi ceci est important à
> https://aka.ms/LearnAboutSenderIdentification ]
>
> Hello Christophe
>
> On 12/14/23 13:06, Christophe Leroy wrote:
>>
>>
> ...
>>
>> So you abandonned the idea of using mutex.h ?
>
> I'm not the one who make a choice here. The patch [1] you're talking
> about was seen by everyone but it seems like no one had shown interest.
> For me personally approach with #define mutex_destroy is not very usual
> but if even slight mixing device with mutex.h is unacceptable what else
> can we do? Avoiding the need to allocate devm slot for empty
> mutex_destroy is more important.
>
Why would a forward declaration of struct device in mutex.h be
unacceptable when it is done in so many headers ?
$ git grep "struct device;" include/ | wc -l
164
> Should I make series #4 with the patch [1] to give it a last chance?
Yes, lets give it a try
>
> [1]
> https://lore.kernel.org/lkml/[email protected]/T/#m3f6df30ffccaccb1df4669a327f349164f572931
>
Christophe
On Thu, Dec 14, 2023 at 3:00 PM Christophe Leroy
<[email protected]> wrote:
> Le 14/12/2023 à 13:48, George Stark a écrit :
> > [Vous ne recevez pas souvent de courriers de [email protected].
> > Découvrez pourquoi ceci est important à
> > https://aka.ms/LearnAboutSenderIdentification ]
> > On 12/14/23 13:06, Christophe Leroy wrote:
...
> >> So you abandonned the idea of using mutex.h ?
> >
> > I'm not the one who make a choice here. The patch [1] you're talking
> > about was seen by everyone but it seems like no one had shown interest.
> > For me personally approach with #define mutex_destroy is not very usual
> > but if even slight mixing device with mutex.h is unacceptable what else
> > can we do? Avoiding the need to allocate devm slot for empty
> > mutex_destroy is more important.
> >
>
> Why would a forward declaration of struct device in mutex.h be
> unacceptable when it is done in so many headers ?
>
> $ git grep "struct device;" include/ | wc -l
> 164
I believe the main misunderstanding here is where to put the
implementation. AFAIU Christophe wants the implementation to be in the
very same _C_-file where mutex_destroy() is defined. mutex.h in this
case indeed requires the only forward declaration and hence doesn't
need to include device.h.
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 14, 2023 at 03:58:56PM +0300, George Stark wrote:
> On 12/14/23 08:42, Nikita Travkin wrote:
> > George Stark писал(а) 14.12.2023 03:30:
...
> > Btw, seems like (5..11)/11 never arrived to the lists...
>
> Yeah there was a delay, but now all patches from series #3 are online.
Nikita is right. This patch was the last in the mailing lists. Fix your mail
gateways, it quite likely the mail server in your organisation filters out
some mails as spam or so. I highly recommend to escalate this with your IT
department.
--
With Best Regards,
Andy Shevchenko