Re: [RFC PATCH v7] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Guenter Roeck]

On 7/21/26 16:22, Alex Yeo wrote:
> On 2026/07/20 10:41 PM, Guenter Roeck wrote:
> > On 7/20/26 06:46, Ilpo Järvinen wrote:
> > > On Sun, 19 Jul 2026, Alex Yeo wrote:
> >
> > ...
> >
> > > > diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/ platform/x86/panasonic-laptop.c
> > > > index b83113c26f88..2d67188de2a9 100644
> > > > --- a/drivers/platform/x86/panasonic-laptop.c
> > > > +++ b/drivers/platform/x86/panasonic-laptop.c
> > > > @@ -119,6 +119,9 @@
> > > >    *        - v0.1  start from toshiba_acpi driver written by John Belmonte
> > > >    */
> > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > +
> > > > +#include <linux/printk.h>
> > > >   #include <linux/acpi.h>
> > > >   #include <linux/backlight.h>
> > > >   #include <linux/bits.h>
> > > > @@ -136,6 +139,14 @@
> > > >   #include <linux/types.h>
> > > >   #include <linux/uaccess.h>
> > > >   #include <acpi/video.h>
> > > > +#include <linux/sysfs.h>
> > > > +#include <linux/hwmon.h>
> > > > +#include <linux/hwmon-sysfs.h>
> >
> > FWIW, I don't see why this is needed.
>
> If you mean linux/hwmon-sysfs.h, I agree.
> I will remove that in v8.
>
> For the other includes:
>
>  >>> +#include <linux/sysfs.h>
> this was previously missing
>
>  >>> +#include <linux/hwmon.h>
> used this for registering the fan as a PWM fan on hwmon.
>
> > > >   struct pcc_acpi {
> > ...
> > > > +    /*
> > > > +     * This mutex ensures that the hwmon and thermal functions
> > > > +     * for fan operations do not conflict as the PWM fan is
> > > > +     * exposed to both.
> > > > +     */
> > > > +    struct mutex            pwm_fan_lock;
> >
> > Maybe ask why this additional lock is used instead of relying on
> > (for hwmon attributes) and using (for thermal) the hwmon subsystem
> > lock.
>
> Both the hwmon and thermal devices share the same control path
> (pcc_pwm_fan_speed_read and pcc_pwm_fan_speed_set). To prevent them from
> executing concurrently and interleaving, I added pwm_fan_lock to serialize access.
>
> Reading and writing fan speed requires a multi-step ACPI transaction (e.g., checking/setting manual mode first before accessing or setting fan speed). This is because I rely on querying ACPI for the current state instead of keeping state cached in the driver. Because of this, the mutex is placed in the outer hwmon and thermal callbacks rather than inside the low-level ACPI helper functions.
>
> If there's a more appropriate way to synchronize this across both interfaces (e.g. use hwmon_lock in place of a driver level mutex), I'd be interested to hear it.

I for my part would like to understand why hwmon_lock() / hwmon_unlock()
(or the hwmon_lock guard) are not sufficient. You suggest above that they
are not sufficient and that you need the additional pwm_fan_lock, but you
don't really explain the reason. You do explain that you need to lock the
thermal access functions, and I agree, but you do not explain why the hwmon
subsystem lock functions can not be used. It would be important to
understand why that is the case because those functions exist for exactly
that purpose. If they are insufficient, it is important to understand why
that is the case.

Thanks,
Guenter