Le 06/12/2023 à 23:14, Christophe Leroy a écrit : > > > Le 06/12/2023 à 19:58, George Stark a écrit : >> [Vous ne recevez pas souvent de courriers de >> gnstark@xxxxxxxxxxxxxxxxx. Découvrez pourquoi ceci est important à >> https://aka.ms/LearnAboutSenderIdentification ] >> >> Hello Hans >> >> Thanks for the review. >> >> On 12/6/23 18:01, Hans de Goede wrote: >>> Hi George, >>> >>> On 12/4/23 19:05, George Stark wrote: >>>> Using of devm API leads to 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 wrapping. >>>> 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@xxxxxxxxxxxxxxxxx> >>>> --- >>>> include/linux/devm-helpers.h | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/include/linux/devm-helpers.h >>>> b/include/linux/devm-helpers.h >>>> index 74891802200d..2f56e476776f 100644 >>>> --- a/include/linux/devm-helpers.h >>>> +++ b/include/linux/devm-helpers.h >>>> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct >>>> device *dev, >>>> return devm_add_action(dev, devm_work_drop, w); >>>> } >>>> >>>> +static inline void devm_mutex_release(void *res) >>>> +{ >>>> + mutex_destroy(res); >>>> +} >>>> + >>>> +/** >>>> + * devm_mutex_init - Resource-managed mutex initialization >>>> + * @dev: Device which lifetime work is bound to >>>> + * @lock: Pointer to a mutex >>>> + * >>>> + * Initialize mutex which is automatically destroyed when driver is >>>> detached. >>>> + */ >>>> +static inline int devm_mutex_init(struct device *dev, struct mutex >>>> *lock) >>>> +{ >>>> + mutex_init(lock); >>>> + return devm_add_action_or_reset(dev, devm_mutex_release, lock); >>>> +} >>>> + >>>> #endif >>> >>> mutex_destroy() only actually does anything if CONFIG_DEBUG_MUTEXES >>> is set, otherwise it is an empty inline-stub. >>> >>> Adding a devres resource to the device just to call an empty inline >>> stub which is a no-op seems like a waste of resources. IMHO it >>> would be better to change this to: >>> >>> static inline int devm_mutex_init(struct device *dev, struct mutex >>> *lock) >>> { >>> mutex_init(lock); >>> #ifdef CONFIG_DEBUG_MUTEXES >>> return devm_add_action_or_reset(dev, devm_mutex_release, lock); >>> #else >>> return 0; >>> #endif >>> } >>> >>> To avoid the unnecessary devres allocation when >>> CONFIG_DEBUG_MUTEXES is not set. >> >> Honestly saying I don't like unnecessary devres allocation either but >> the proposed approach has its own price: >> >> 1) we'll have more than one place with branching if mutex_destroy is >> empty or not using indirect condition. If suddenly mutex_destroy is >> extended for non-debug code (in upstream branch or e.g. by someone for >> local debug) than there'll be a problem. >> >> 2) If mutex_destroy is empty or not depends on CONFIG_PREEMPT_RT option >> too. When CONFIG_PREEMPT_RT is on mutex_destroy is always empty. >> >> As I see it only the mutex interface (mutex.h) has to say definitely if >> mutex_destroy must be called. Probably we could add some define to >> include/linux/mutex.h,like IS_MUTEX_DESTROY_REQUIRED and declare it near >> mutex_destroy definition itself. >> >> I tried to put devm_mutex_init itself in mutex.h and it could've helped >> too but it's not the place for devm API. >> > > What do you mean by "it's not the place for devm API" ? > > If you do a 'grep devm_ include/linux/' you'll find devm_ functions in > almost 100 .h files. Why wouldn't mutex.h be the place for > devm_mutex_init() ? Looking at it closer, I have the feeling that you want to do similar to devm_gpio_request() in linux/gpio.h : In linux/mutex.h, add a prototype for devm_mutex_init() when CONFIG_DEBUG_MUTEXES is defined and an empty static inline otherwise. Then define devm_mutex_init() in kernel/locking/mutex-debug.c Wouldn't that work ? Christophe
