Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Wilken Gottwalt <wilken.gottwalt@posteo.net>]

On Wed, 13 May 2026 11:10:26 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 5/13/26 10:50, Wilken Gottwalt wrote:
> > On Wed, 13 May 2026 09:42:08 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> On Wed, May 13, 2026 at 03:53:51PM +0000, Wilken Gottwalt wrote:
> >>> On Wed, 13 May 2026 07:58:14 -0700
> >>> Guenter Roeck <linux@roeck-us.net> wrote:
> >>>
> >> ...
> >>> Okay, that will get a bit complex now, because I added my hack and I see
> >>> exactly what I assumed is happening.
> >>>
> >> ...
> >>>
> >>> If this does not explain the obvious issue, I have not idea how explain
> >>> it further. My English is limited. This is a HID driver with data gathering
> >>> functions running in the context of the USB-HID context. Callbacks from the
> >>> hwmon and the debugfs subsystem call these data gathering functions, and the
> >>> first function in that context, corsairpsu_request(), which can run several
> >>> instances in paralellel, needs the mutex.
> >>>
> >>
> >> You don't explain why the patches below are insufficient.
> >>
> >> I used guard() to keep the changes simple, but hwmon_lock() / hwmon_unlock()
> >> would be similar. Please provide evidence that this does not work.
> >>
> >> Thanks,
> >> Guenter
> >> --
> >>  From aa3ec1484bdd619e8fa2ce569ec653d35fbf3615 Mon Sep 17 00:00:00 2001
> >> From: Guenter Roeck <linux@roeck-us.net>
> >> Date: Wed, 13 May 2026 07:14:33 -0700
> >> Subject: [PATCH 1/4] hwmon: Support guard() and scoped_guard for subsystem
> >>   locks
> >>
> >> Add support for guard() and scoped_guard() for the hwmon subsystem lock
> >> to simplify its use.
> >>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >>   Documentation/hwmon/hwmon-kernel-api.rst | 7 ++++---
> >>   include/linux/hwmon.h                    | 2 ++
> >>   2 files changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst
> >> b/Documentation/hwmon/hwmon-kernel-api.rst index 1d7f1397a827..9fcde32a140d 100644
> >> --- a/Documentation/hwmon/hwmon-kernel-api.rst
> >> +++ b/Documentation/hwmon/hwmon-kernel-api.rst
> >> @@ -85,9 +85,10 @@ removal.
> >>   When using ``[devm_]hwmon_device_register_with_info()`` to register the
> >>   hardware monitoring device, accesses using the associated access functions
> >>   are serialised by the hardware monitoring core. If a driver needs locking
> >> -for other functions such as interrupt handlers or for attributes which are
> >> -fully implemented in the driver, hwmon_lock() and hwmon_unlock() can be used
> >> -to ensure that calls to those functions are serialized.
> >> +for other functions such as interrupt handlers, attributes which are fully
> >> +implemented in the driver, or debugfs functions, hwmon_lock() and hwmon_unlock()
> >> +can be used to ensure that calls to those functions are serialized. Those
> >> +functions also support guard() and scoped_guard() variants.
> >>   
> >>   Using devm_hwmon_device_register_with_info()
> >>   --------------------------------------------
> >> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> >> index 301a83afbd66..04959e044fd0 100644
> >> --- a/include/linux/hwmon.h
> >> +++ b/include/linux/hwmon.h
> >> @@ -495,6 +495,8 @@ char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
> >>   void hwmon_lock(struct device *dev);
> >>   void hwmon_unlock(struct device *dev);
> >>   
> >> +DEFINE_GUARD(hwmon_lock, struct device *, hwmon_lock(_T), hwmon_unlock(_T))
> >> +
> >>   /**
> >>    * hwmon_is_bad_char - Is the char invalid in a hwmon name
> >>    * @ch: the char to be considered
> > 
> > Now I am completely confused. What is that guard() and scoped_guard() patch?
> 
> Why do you think I attached it ? Why would I do that if it was already upstream ?
> I wrote it this morning, that is why. You'd find it on the mailing list if
> you looked.
> 
> Ok, fine, I'll send another patch without it if you don't want to apply it
> even for testing.

No no, don't get me wrong. I just was confused about what you wanted.
And what me really confused, was the change in include/linux/hwmon.h. I
just couldn't imagine, that you proposed to change a subsystem header.
I really try to avoid to touch code, that may affect other drivers
outside of mine. Though, I will try it, but I still need to read into
this and understand it completely. I also may need to rethink how to
better deal with the with the RAW HID interface, which currently shares
the cmd_buffer and the completion with the normal HID operation.

greetings,
Wilken