Hello Christophe


On 12/17/23 12:31, Christophe Leroy wrote:

...

quoted

quoted

quoted

quoted

quoted

quoted

---
     include/linux/mutex.h        | 23 +++++++++++++++++++++++
     kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
     2 files changed, 45 insertions(+)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index a33aa9eb9fc3..ebd03ff1ef66 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h

@@ -21,6 +21,8 @@
     #include <linux/debug_locks.h>
     #include <linux/cleanup.h>
+struct device;
+
     #ifdef CONFIG_DEBUG_LOCK_ALLOC
     # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
             , .dep_map = {                    \

@@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock,

const char *name,
      */
     extern bool mutex_is_locked(struct mutex *lock);
+#ifdef CONFIG_DEBUG_MUTEXES
+
+int devm_mutex_init(struct device *dev, struct mutex *lock);

Please add "extern" to the function declaration to be consistent with
other functional declarations in mutex.h.

'extern' is pointless and deprecated on function prototypes. Already
having some is not a good reason to add new ones, errors from the past
should be avoided nowadays. With time they should all disappear so
don't
add new ones.

Yes, "extern" is optional. It is just a suggestion and I am going to
argue about that.

FWIW, note that when you perform a strict check with checkpatch.pl, you
get a warning for that:

$ ./scripts/checkpatch.pl --strict -g HEAD
CHECK: extern prototypes should be avoided in .h files
#56: FILE: include/linux/mutex.h:131:
+extern int devm_mutex_init(struct device *dev, struct mutex *lock);

total: 0 errors, 0 warnings, 1 checks, 99 lines checked

This is ambiguous situation about extern. It's deprecated and useless on
one hand but harmless. And those externs will not disappear by themself
- it'll be one patch that clean them all at once (in one header at
least) so one more extern will not alter the overall picture.

That kind of cleanup patch bomb is a nightmare for backporting, so if it
happens one day it should be as light as possible, hence the importance
to not add new ones and remove existing one everytime you modify or move
a line including it for whatever reason.

quoted

On the other hand if we manage to place devm_mutex_init near
mutex_destroy then we'll have:

int devm_mutex_init(struct device *dev, struct mutex *lock);
extern void mutex_destroy(struct mutex *lock);

I sent you an alternative proposal that avoids duplication of the static
inline version of devm_mutex_init(). If you agree with it just take it
into your series and that question will vanish.

Thanks for that patch by the way. The only comment is that moving 
mutex_destroy
should be done in a separate patch IMO.
Waiman Long proposed such a refactoring here:
https://lore.kernel.org/lkml/20231216013656.1382213-2-longman@redhat.com/T/ (local)

With this patch adding devm_mutex_init would be straightforward.

quoted

and it raises questions and does not look very nice.

If you look at linux/mm.h there are plenty of them anyway, so why do
different ? For an exemple look at
https://elixir.bootlin.com/linux/v6.7-rc4/source/include/linux/mm.h#L2372

Oh, I see. Ok, I don't have any more arguments against removing extern.
We'll see what mutex.h maintainers decide.

Christophe

-- 
Best regards
George