linux-kernel - Re: [PATCH v6 0/7] HID: asus: Fix ASUS ROG Laptop's Keyboard backlight handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: 
 <CAGwozwEvQ69yCa3dXk7BOUaB_-XmpKcBdK-xjNYq_5hMGDmvrQ@mail.gmail.com>
Date: Mon, 20 Oct 2025 20:54:22 +0200
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Denis Benato <benato.denis96@...il.com>,
 platform-driver-x86@...r.kernel.org,
	linux-input@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires <bentiss@...nel.org>,
	Corentin Chary <corentin.chary@...il.com>,
 "Luke D . Jones" <luke@...nes.dev>,
	Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH v6 0/7] HID: asus: Fix ASUS ROG Laptop's Keyboard
 backlight handling

On Mon, 20 Oct 2025 at 19:13, Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> On Fri, 17 Oct 2025, Antheas Kapenekakis wrote:
> > On Thu, 16 Oct 2025 at 18:16, Antheas Kapenekakis <lkml@...heas.dev> wrote:
> > >
> > > On Thu, 16 Oct 2025 at 17:09, Ilpo Järvinen
> > > <ilpo.jarvinen@...ux.intel.com> wrote:
> > > >
> > > > On Thu, 16 Oct 2025, Antheas Kapenekakis wrote:
> > > > > On Thu, 16 Oct 2025 at 13:57, Denis Benato <benato.denis96@...il.com> wrote:
> > > > > > On 10/13/25 22:15, Antheas Kapenekakis wrote:
> > > > > > > This is a two part series which does the following:
> > > > > > >   - Clean-up init sequence
> > > > > > >   - Unify backlight handling to happen under asus-wmi so that all Aura
> > > > > > >     devices have synced brightness controls and the backlight button works
> > > > > > >     properly when it is on a USB laptop keyboard instead of one w/ WMI.
> > > > > > >
> > > > > > > For more context, see cover letter of V1. Since V5, I removed some patches
> > > > > > > to make this easier to merge.
> > > > > > >
> > > > > > > All comments with these patches had been addressed since V4.
> > > > > > I have loaded this patchset for users of asus-linux project to try out.
> > > > > >
> > > > > > One of them opened a bug report about a kernel bug that happens
> > > > > > consistently when closing the lid of his laptop [1].
> > > > > >
> > > > > > He also sent another piece of kernel log, but didn't specify anything more
> > > > > > about this [2].
> > > > > >
> > > > > > [1] https://pastebin.com/akZx1w10
> > > > > > [2] https://pastebin.com/sKdczPgf
> > > > >
> > > > > Can you provide a link to the bug report? [2] seems unrelated.
> > > > >
> > > > > As for [1], it looks like a trace that stems from a sysfs write to
> > > > > brightness stemming from userspace that follows the same chain it
> > > > > would on a stock kernel and times out. Is it present on a stock
> > > > > kernel?
> > > > >
> > > > > Ilpo should know more about this, could the spinlock be interfering?
> > > >
> > > > [1] certainly seems to do schedule() from do_kbd_led_set() so it's not
> > > > possible to use spinlock there.
> > > >
> > > > So we're back to what requires the spinlock? And what the spinlock
> > > > protects?
> > >
> > > For that invocation, since it is coming from the cdev device owned by
> > > asus_wmi, it protects asus_ref.listeners under do_kbd_led_set.
> > > asus_wmi is protected by the fact it is owned by that device. Spinlock
> > > is not required in this invocation due to not being an IRQ.
> > >
> > > Under asus_hid_event (second to last patch), which is called from an
> > > IRQ, a spinlock is required for protecting both listeners and the
> > > asus_ref.asus, and I suspect that scheduling from an IRQ is not
> > > allowed either. Is that correct?
> >
> > So it is a bit tricky here. When the IRQ fires, it needs to know
> > whether asus-wmi will handle the keyboard brightness event so that it
> > falls back to emitting it.
> >
> > If we want it to know for sure, it needs to access asus_wmi, so it
> > needs a spinlock or an IRQ friendly lock. This way, currently,
> > asus_hid_event will return -EBUSY if there is no led device so the
> > event propagates through hid.
> >
> > If we say that it is good enough to know that it was compiled with
> > IS_REACHABLE(CONFIG_ASUS_WMI), ie the actual implementation of
> > asus_hid_event in asus-wmi will never return an error, then,
> > asus_hid_event can schedule a task to fire the event without a lock,
> > and that task can use a normal locking primitive.
> >
> > If the task needs to be assigned to a device or have a handle,
> > asus_hid_listener can be provided to asus_hid_event, so that it is
> > owned by the calling device.
> >
> > What would the appropriate locking primitive be in this case?
>
> If you can move the non-check content out of asus_hid_event(), then you
> can nest mutex & spinlock for updating asus_ref. On reader side,
> asus_hid_event() only takes the spinlock and the rest of the readers
> (non-irq ones) can take just the mutex.

I think I found a good compromise on the V7. I also found that in
kbd_get I was missing a lock for the brightness value which
complicated things further, as get was called when creating the led
device.

On V7 I use a workqueue to do the bulk of initialization and setting
brightness so I manage to limit the lock to asus, the led wk value and
a new notify value that does hw_changed

>
> --
>  i.