| Message ID | 20231213223020.2713164-4-gnstark@salutedevices.com |
|---|---|
| State | New |
| Headers | show |
| Series | devm_led_classdev_register() usage problem | expand |
On Thu, Dec 14, 2023 at 12:30 AM George Stark <gnstark@salutedevices.com> 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
On Thu, Dec 14, 2023 at 12:36 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Dec 14, 2023 at 12:30 AM George Stark <gnstark@salutedevices.com> 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
Hi, On 12/13/23 23:38, Andy Shevchenko wrote: > On Thu, Dec 14, 2023 at 12:36 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Thu, Dec 14, 2023 at 12:30 AM George Stark <gnstark@salutedevices.com> 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 <hdegoede@redhat.com> 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 gnstark@salutedevices.com. 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 <gnstark@salutedevices.com> > --- > 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 gnstark@salutedevices.com. 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 <gnstark@salutedevices.com> >> --- >> 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/377e4437-7051-4d88-ae68-1460bcd692e1@redhat.com/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 <gnstark@salutedevices.com> >> --- >> 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 >>
Le 14/12/2023 à 13:48, George Stark a écrit : > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. > 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/377e4437-7051-4d88-ae68-1460bcd692e1@redhat.com/T/#m3f6df30ffccaccb1df4669a327f349164f572931 > Christophe
On Thu, Dec 14, 2023 at 3:00 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > Le 14/12/2023 à 13:48, George Stark a écrit : > > [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. > > 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.
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
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 <gnstark@salutedevices.com> --- include/linux/devm-helpers.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)