Linux LED subsystem development
 help / color / mirror / Atom feed
From: Lee Jones <lee@kernel.org>
To: sashiko-reviews@lists.linux.dev
Cc: Paul Sherman <shermanpauldylan@gmail.com>, linux-leds@vger.kernel.org
Subject: Re: [PATCH] leds: trigger: cpu: use cpuhp_setup_state_nocalls() for large CPU counts
Date: Thu, 6 Aug 2026 15:15:13 +0100	[thread overview]
Message-ID: <20260806141513.GJ2869284@google.com> (raw)
In-Reply-To: <20260727222518.B1ECA1F000E9@smtp.kernel.org>

On Mon, 27 Jul 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Skipping hotplug startup callbacks during initialization leaves the CPU LED triggers in an incorrect inactive state for all already-online CPUs.
> - [Low] The commit message description is not capitalized, violating LED subsystem conventions.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> --
> 
> --- Patch [1]: [PATCH] leds: trigger: cpu: use cpuhp_setup_state_nocalls() for large CPU counts ---
> commit 2aa3e021f1cce38c0cc7487063d486ab550d6162
> Author: Paul Sherman <shermanpauldylan@gmail.com>
> 
> leds: trigger: cpu: use cpuhp_setup_state_nocalls() for large CPU counts
> 
> This commit changes ledtrig_cpu_init() to use cpuhp_setup_state_nocalls()
> instead of cpuhp_setup_state(). This avoids invoking the hotplug callbacks for
> already-online CPUs at registration time to prevent boot delays on systems
> with large CPU counts.
> 
> > leds: trigger: cpu: use cpuhp_setup_state_nocalls() for large CPU counts
> 
> [Severity: Low]
> This isn't a bug, but the commit message description starts with a lowercase
> letter. The LED subsystem guidelines mandate: "Always capitalize the description
> after the subsystem prefix for MFD, LED, and Backlight subsystems."
> 
> > diff --git a/drivers/leds/trigger/ledtrig-cpu.c b/drivers/leds/trigger/ledtrig-cpu.c
> > index 679323c2ccda0..ce75d5f54684c 100644
> > --- a/drivers/leds/trigger/ledtrig-cpu.c
> > +++ b/drivers/leds/trigger/ledtrig-cpu.c
> > @@ -163,7 +163,15 @@ static int __init ledtrig_cpu_init(void)
> >  
> >  	register_syscore(&ledtrig_cpu_syscore);
> >  
> > -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "leds/trigger:starting",
> > +	/*
> > +	 * Use cpuhp_setup_state_nocalls() to avoid invoking ledtrig_online_cpu()
> > +	 * on every already-online CPU at registration time. On systems with large
> > +	 * CPU counts (e.g. 64-hart RISC-V) cpuhp_setup_state() would serialize
> > +	 * callbacks across all online CPUs, causing multi-hundred-second delays
> > +	 * during boot. The trigger operates correctly for future hotplug events
> > +	 * regardless.
> > + 	 */
> > +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "leds/trigger:starting",
> > +				ledtrig_online_cpu, ledtrig_prepare_down_cpu);
> 
> [Severity: High]
> Does skipping the hotplug startup callbacks during initialization leave the CPU
> LED triggers in an incorrect inactive state for all already-online CPUs?
> 
> By replacing cpuhp_setup_state() with cpuhp_setup_state_nocalls(), the driver
> skips invoking ledtrig_online_cpu() for already-online CPUs during registration.
> Because the cpu_trig per-CPU variables and num_active_cpus are zero-initialized,
> they remain in the inactive state.
> 
> On architectures that do not emit ledtrig_cpu events during idle transitions,
> these CPUs will never call ledtrig_cpu(CPU_LED_START), so their LEDs will never
> turn on unless they undergo a hotplug offline/online cycle.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260727221607.5216-1-shermanpauldylan@gmail.com?part=1

-- 
Lee Jones

      reply	other threads:[~2026-08-06 14:15 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-27 22:16 [PATCH] leds: trigger: cpu: use cpuhp_setup_state_nocalls() for large CPU counts Paul Sherman
2026-07-27 22:25 ` sashiko-bot
2026-08-06 14:15   ` Lee Jones [this message]

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=20260806141513.GJ2869284@google.com \
    --to=lee@kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=shermanpauldylan@gmail.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox