Re: [PATCH] backlight: pwm_bl: apply the initial backlight state with sane defaults

[Lee Jones <lee@kernel.org>]

On Tue, 25 Nov 2025, Daniel Thompson wrote:

> On Tue, Nov 25, 2025 at 09:24:26AM +0100, Michael Grzeschik wrote:
> > On Tue, Nov 18, 2025 at 12:52:14PM +0000, Daniel Thompson wrote:
> > > On Fri, Nov 14, 2025 at 02:09:56PM +0000, Mark Brown wrote:
> > > > On Thu, Jul 31, 2025 at 10:47:18AM +0200, Michael Grzeschik wrote:
> > > > > Currently when calling pwm_apply_might_sleep in the probe routine
> > > > > the pwm will be configured with an not fully defined state.
> > > > >
> > > > > The duty_cycle is not yet set in that moment. There is a final
> > > > > backlight_update_status call that will have a properly setup state.
> > > > > However this change in the backlight can create a short flicker if the
> > > > > backlight was already preinitialised.
> > > >
> > > > I'm seeing the libre.computer Renegade Elite producing warnings during
> > > > boot in -next which bisect to this patch.  The warnings are:
> > > >
> > > > [   24.175095] input: adc-keys as /devices/platform/adc-keys/input/input1
> > > > [   24.176612] ------------[ cut here ]------------
> > > > [   24.177048] WARNING: CPU: 0 PID: 0 at kernel/context_tracking.c:127 ct_kernel_exit.constprop.0+0x98/0xa0
> > > >
> > > > ...
> > > >
> > > > [   24.190106] Call trace:
> > > > [   24.190325]  ct_kernel_exit.constprop.0+0x98/0xa0 (P)
> > > > [   24.190775]  ct_idle_enter+0x10/0x20
> > > > [   24.191096]  cpuidle_enter_state+0x1fc/0x320
> > > > [   24.191476]  cpuidle_enter+0x38/0x50
> > > > [   24.191802]  do_idle+0x1e4/0x260
> > > > [   24.192094]  cpu_startup_entry+0x34/0x3c
> > > > [   24.192444]  rest_init+0xdc/0xe0
> > > > [   24.192734]  console_on_rootfs+0x0/0x6c
> > > > [   24.193082]  __primary_switched+0x88/0x90
> > > > [   24.193445] ---[ end trace 0000000000000000 ]---
> > > >
> > > > which seems a little surprising but there is some console stuff there
> > > > that looks relevant.
> > > >
> > > > Full log:
> > > >
> > > >     https://lava.sirena.org.uk/scheduler/job/2086528#L897
> > >
> > > Michael, reading these logs it looks to me like the underlying oops
> > > is this backtrace (which makes a lot more sense given the code you
> > > altered):
> > >
> > > [   24.133631] Call trace:
> > > [   24.133853]  pwm_backlight_probe+0x830/0x868 [pwm_bl] (P)
> > > [   24.134341]  platform_probe+0x5c/0xa4
> > > [   24.134679]  really_probe+0xbc/0x2c0
> > > [   24.135001]  __driver_probe_device+0x78/0x120
> > > [   24.135391]  driver_probe_device+0x3c/0x154
> > > [   24.135765]  __driver_attach+0x90/0x1a0
> > > [   24.136111]  bus_for_each_dev+0x7c/0xdc
> > > [   24.136462]  driver_attach+0x24/0x38
> > > [   24.136785]  bus_add_driver+0xe4/0x208
> > > [   24.137124]  driver_register+0x68/0x130
> > > [   24.137468]  __platform_driver_register+0x24/0x30
> > > [   24.137888]  pwm_backlight_driver_init+0x20/0x1000 [pwm_bl]
> > > [   24.138389]  do_one_initcall+0x60/0x1d4
> > > [   24.138735]  do_init_module+0x54/0x23c
> > > [   24.139073]  load_module+0x1760/0x1cf0
> > > [   24.139407]  init_module_from_file+0x88/0xcc
> > > [   24.139787]  __arm64_sys_finit_module+0x1bc/0x338
> > > [   24.140207]  invoke_syscall+0x48/0x104
> > > [   24.140549]  el0_svc_common.constprop.0+0x40/0xe0
> > > [   24.140970]  do_el0_svc+0x1c/0x28
> > > [   24.141268]  el0_svc+0x34/0xec
> > > [   24.141548]  el0t_64_sync_handler+0xa0/0xf0
> > > [   24.141920]  el0t_64_sync+0x198/0x19c
> > >
> > > Should we back out the patch for now?
> >
> > I would be fine with that. But actually I would like to see the
> > proof that without the patch, this backtrace will not trigger.
> > Looking through the codepath, I could not directly find a case
> > where this should happen.
> 
> I took a look at the logs Mark provided and I think the problem
> is a divide-by-zero caused by calling pwm_backlight_brightness_default()
> when state.period is zero.
> 
> It emerges as a BRK because the compiler recognised there is undefined
> behaviour. The zero that we divide by comes from a ternary condition in
> fls(). The compiler recognises one of the conditional code paths will
> result in undefined behaviour so, it doesn't need to generating code for
> the bad code path, it just injects a brk instruction.
> 
> 
> > Mark, is there a way to rerun this without my patch?
> 
> I have to admit I thought this was why Mark provided a bisect log!
> 
> Anyhow, unless someone can refute the analysis above I do think we need
> to pull the patch.

Un-applied now, thanks.

-- 
Lee Jones [李琼斯]