All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lee Jones <lee@kernel.org>
To: Hans de Goede <johannes.goede@oss.qualcomm.com>
Cc: Sebastian Reichel <sre@kernel.org>,
	Pavel Machek <pavel@kernel.org>,
	linux-leds@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] leds: led-class: Only Add LED to leds_list when it is fully ready
Date: Thu, 22 Jan 2026 11:49:25 +0000	[thread overview]
Message-ID: <20260122114925.GF3831112@google.com> (raw)
In-Reply-To: <20260109093504.GA1118061@google.com>

On Fri, 09 Jan 2026, Lee Jones wrote:

> On Thu, 08 Jan 2026, Hans de Goede wrote:
> 
> > Hi Lee,
> > 
> > On 8-Jan-26 13:11, Lee Jones wrote:
> > > On Fri, 12 Dec 2025, Hans de Goede wrote:
> > > 
> > >> Hi,
> > >>
> > >> On 12-Dec-25 07:49, Sebastian Reichel wrote:
> > >>> Hi,
> > >>>
> > >>> On Thu, Dec 11, 2025 at 05:37:27PM +0100, Hans de Goede wrote:
> > >>>> Before this change the LED was added to leds_list before led_init_core()
> > >>>> gets called adding it the list before led_classdev.set_brightness_work gets
> > >>>> initialized.
> > >>>>
> > >>>> This leaves a window where led_trigger_register() of a LED's default
> > >>>> trigger will call led_trigger_set() which calls led_set_brightness()
> > >>>> which in turn will end up queueing the *uninitialized*
> > >>>> led_classdev.set_brightness_work.
> > >>>>
> > >>>> This race gets hit by the lenovo-thinkpad-t14s EC driver which registers
> > >>>> 2 LEDs with a default trigger provided by snd_ctl_led.ko in quick
> > >>>> succession. The first led_classdev_register() causes an async modprobe of
> > >>>> snd_ctl_led to run and that async modprobe manages to exactly hit
> > >>>> the window where the second LED is on the leds_list without led_init_core()
> > >>>> being called for it, resulting in:
> > >>>>
> > >>>>  ------------[ cut here ]------------
> > >>>>  WARNING: CPU: 11 PID: 5608 at kernel/workqueue.c:4234 __flush_work+0x344/0x390
> > >>>>  Hardware name: LENOVO 21N2S01F0B/21N2S01F0B, BIOS N42ET93W (2.23 ) 09/01/2025
> > >>>>  ...
> > >>>>  Call trace:
> > >>>>   __flush_work+0x344/0x390 (P)
> > >>>>   flush_work+0x2c/0x50
> > >>>>   led_trigger_set+0x1c8/0x340
> > >>>>   led_trigger_register+0x17c/0x1c0
> > >>>>   led_trigger_register_simple+0x84/0xe8
> > >>>>   snd_ctl_led_init+0x40/0xf88 [snd_ctl_led]
> > >>>>   do_one_initcall+0x5c/0x318
> > >>>>   do_init_module+0x9c/0x2b8
> > >>>>   load_module+0x7e0/0x998
> > >>>>
> > >>>> Close the race window by moving the adding of the LED to leds_list to
> > >>>> after the led_init_core() call.
> > >>>>
> > >>>> Cc: Sebastian Reichel <sre@kernel.org>
> > >>>> Cc: stable@vger.kernel.org
> > >>>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> > >>>> ---
> > >>>
> > >>> heh, I've never hit this. But I guess that is not too surprising
> > >>> considering it is a race condition. The change looks good to me:
> > >>>
> > >>> Reviewed-by: Sebastian Reichel <sre@kernel.org>
> > >>
> > >> Thx.
> > >>  
> > >>>> Note no Fixes tag as this problem has been around for a long long time,
> > >>>> so I could not really find a good commit for the Fixes tag.
> > >>>
> > >>> My suggestion would be:
> > >>>
> > >>> Fixes: d23a22a74fde ("leds: delay led_set_brightness if stopping soft-blink")
> > >>
> > >> Ack, that works for me.
> > >>
> > >> Lee can you add this Fixes tag while merging ?
> > >>
> > >> Also (in case it is not obvious) this is a bugfix so it would be
> > >> nice if this could go in a fixes pull-request for 6.19.
> > > 
> > > Yes, I can add the Fixes: tag and no, I have no plans to send this for
> > > -fixes.  As you rightly mentioned, this issue has been around for a long
> > > time already.  I tend to only send -fixes pull-requests for things that
> > > broke in -rc1 of the same release.
> > 
> > Even though this has been around for a long time, it would be good
> > to get this in as a fix for 6.19-rc# because as described in the commit
> > msg the lenovo-thinkpad-t14s embedded-controller driver, which is new in
> > 6.19-rc1 manages to reliably trigger the race (for me, with a Fedora
> > kernel distconfig).
> > 
> > I was surprised I could hit the race pretty reliably, but it did make
> > debugging this easier.
> > 
> > Hitting the race also leads to a crash due to a NULL ptr deref after
> > the WARN(). I did not elaborate on this in the commit msg, because
> > the WARN() is the first sign of trying to use uninitialized mem.
> > 
> > IMHO having a reproducable race which causes a crash is
> > a good reason to submit this as a fix for 6.19 .
> 
> Noted.  Leave it with me.

https://lore.kernel.org/all/20260122114749.GE3831112@google.com/T/#u

-- 
Lee Jones [李琼斯]

  reply	other threads:[~2026-01-22 11:49 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-12-11 16:37 [PATCH] leds: led-class: Only Add LED to leds_list when it is fully ready Hans de Goede
2025-12-12  6:49 ` Sebastian Reichel
2025-12-12  9:05   ` Hans de Goede
2026-01-08 12:11     ` Lee Jones
2026-01-08 12:17       ` Hans de Goede
2026-01-09  9:35         ` Lee Jones
2026-01-22 11:49           ` Lee Jones [this message]
2026-01-08 12:14 ` (subset) " Lee Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260122114925.GF3831112@google.com \
    --to=lee@kernel.org \
    --cc=johannes.goede@oss.qualcomm.com \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@kernel.org \
    --cc=sre@kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.