linux-kernel - Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZqQcps0s5ilLmV1W@google.com>
Date: Fri, 26 Jul 2024 15:01:10 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Qiang Ma <maqianga@...ontech.com>
Cc: hdegoede@...hat.com, inux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

On Fri, Jul 26, 2024 at 02:57:03PM -0700, Dmitry Torokhov wrote:
> Hi Qiang,
> 
> On Fri, Jul 26, 2024 at 06:27:30PM +0800, Qiang Ma wrote:
> > After we turn on the keyboard CAPSL LED and let the system suspend,
> > the keyboard LED is not off, and the kernel log is as follows:
> > 
> > [  185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> > [  185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > [  185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> > [  185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > 
> > The log shows that after the command 0xed is sent, the data
> > sent is 0x04 instead of 0x00.
> > 
> > Solution:
> > Add a bitmap variable ledon in the atkbd structure, and then set ledon
> > according to code-value in the event, in the atkbd_set_leds function,
> > first look at the value of lenon, if it is 0, there is no need to
> > look at the value of dev->led, otherwise, Need to look at dev->led
> > to determine the keyboard LED on/off.
> 
> I am not sure why duplicating input_dev->led which is supposed to record
> which LEDs are currently active on an input device would solve the
> issue. Could you please explain?

Ah, OK, I see it now. We do not actually toggle input_dev->led when
suspending, so atkbd uses wrong data to determine the LED state.

> 
> The input core is supposed to turn off all LEDs on suspend. This happens
> in input_dev_toggle() which is called from input_dev_suspend(). It
> iterates over all LEDs on a device and turns off active ones one by
> one.
> 
> I think what happens here is we are running afoul of the throttling done
> in atkbd (see atkbd_schedule_event_work), and it does not actually turn
> off all LEDs in time. But on the other hand atkbd_cleanup() (which is
> called to suspend the keyboard) calls
> 
> 	ps2_command(&atkbd->ps2dev, NULL, ATKBD_CMD_RESET_DEF);
> 
> which should turn off everything anyways.

But still, why ATKBD_CMD_RESET_DEF does not shut off the LEDs for you?

Thanks.

-- 
Dmitry