drivers/soc/samsung/exynos-pmu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Booting the kernel on Pixel-6 with `CONFIG_DEBUG_PREEMPT` prints the
following WARN:
[ 0.784187][ T1] BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
[ 0.784328][ T1] caller is debug_smp_processor_id+0x20/0x30
[ 0.784433][ T1] CPU: 6 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc4-gd69eb204c255 #1 PREEMPT
[ 0.784439][ T1] Hardware name: Oriole (DT)
[ 0.784441][ T1] Call trace:
[ 0.784443][ T1] show_stack+0x34/0xa0 (C)
[ 0.784453][ T1] dump_stack_lvl+0x7c/0xb0
[ 0.784460][ T1] dump_stack+0x18/0x24
[ 0.784464][ T1] check_preemption_disabled+0xf8/0x100
[ 0.784470][ T1] debug_smp_processor_id+0x20/0x30
[ 0.784476][ T1] gs101_cpuhp_pmu_online+0x40/0x108
[ 0.784483][ T1] cpuhp_invoke_callback+0x188/0x2d8
[ 0.784490][ T1] cpuhp_issue_call+0xec/0x240
[ 0.784494][ T1] __cpuhp_setup_state_cpuslocked+0x140/0x2c0
[ 0.784499][ T1] __cpuhp_setup_state+0x58/0x88
[ 0.784504][ T1] exynos_pmu_probe+0x2a4/0x380
[ 0.784508][ T1] platform_probe+0x64/0xd0
[ 0.784516][ T1] really_probe+0xd0/0x3b0
[ 0.784520][ T1] __driver_probe_device+0x8c/0x170
[ 0.784524][ T1] driver_probe_device+0x44/0x140
[ 0.784528][ T1] __device_attach_driver+0xd8/0x180
[ 0.784532][ T1] bus_for_each_drv+0x90/0xf8
[ 0.784536][ T1] __device_attach+0xa8/0x1d0
[ 0.784540][ T1] device_initial_probe+0x1c/0x30
[ 0.784544][ T1] bus_probe_device+0xb4/0xc0
[ 0.784547][ T1] device_add+0x4d0/0x700
[ 0.784550][ T1] of_device_add+0x4c/0x78
[ 0.784556][ T1] of_platform_device_create_pdata+0x9c/0x148
[ 0.784560][ T1] of_platform_bus_create+0x1d0/0x370
[ 0.784563][ T1] of_platform_bus_create+0x234/0x370
[ 0.784567][ T1] of_platform_populate+0x84/0x178
[ 0.784571][ T1] of_platform_default_populate_init+0xf0/0x120
[ 0.784579][ T1] do_one_initcall+0x68/0x2d0
[ 0.784585][ T1] kernel_init_freeable+0x2d8/0x358
[ 0.784589][ T1] kernel_init+0x28/0x168
[ 0.784595][ T1] ret_from_fork+0x10/0x20
As this value is only read once, it doesn't require to be stable, so
just use "raw_smp_processor_id" instead.
Cc: Peter Griffin <peter.griffin@linaro.org>
Cc: André Draszik <andre.draszik@linaro.org>
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
drivers/soc/samsung/exynos-pmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index a77288f49d24..338f4758a089 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
static int gs101_cpuhp_pmu_online(unsigned int cpu)
{
- unsigned int cpuhint = smp_processor_id();
+ unsigned int cpuhint = raw_smp_processor_id();
u32 reg, mask;
/* clear cpu inform hint */
@@ -361,7 +361,7 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
static int gs101_cpuhp_pmu_offline(unsigned int cpu)
{
u32 reg, mask;
- unsigned int cpuhint = smp_processor_id();
+ unsigned int cpuhint = raw_smp_processor_id();
/* set cpu inform hint */
regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
--
2.51.0.355.g5224444f11-googHi Mostafa,
Thanks for your patch and bug report, it's great to see more folks
running upstream Pixel 6 :)
On Fri, 5 Sept 2025 at 17:25, Mostafa Saleh <smostafa@google.com> wrote:
>
> Booting the kernel on Pixel-6 with `CONFIG_DEBUG_PREEMPT` prints the
> following WARN:
>
> [ 0.784187][ T1] BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
> [ 0.784328][ T1] caller is debug_smp_processor_id+0x20/0x30
> [ 0.784433][ T1] CPU: 6 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc4-gd69eb204c255 #1 PREEMPT
> [ 0.784439][ T1] Hardware name: Oriole (DT)
> [ 0.784441][ T1] Call trace:
> [ 0.784443][ T1] show_stack+0x34/0xa0 (C)
> [ 0.784453][ T1] dump_stack_lvl+0x7c/0xb0
> [ 0.784460][ T1] dump_stack+0x18/0x24
> [ 0.784464][ T1] check_preemption_disabled+0xf8/0x100
> [ 0.784470][ T1] debug_smp_processor_id+0x20/0x30
> [ 0.784476][ T1] gs101_cpuhp_pmu_online+0x40/0x108
> [ 0.784483][ T1] cpuhp_invoke_callback+0x188/0x2d8
> [ 0.784490][ T1] cpuhp_issue_call+0xec/0x240
> [ 0.784494][ T1] __cpuhp_setup_state_cpuslocked+0x140/0x2c0
> [ 0.784499][ T1] __cpuhp_setup_state+0x58/0x88
> [ 0.784504][ T1] exynos_pmu_probe+0x2a4/0x380
> [ 0.784508][ T1] platform_probe+0x64/0xd0
> [ 0.784516][ T1] really_probe+0xd0/0x3b0
> [ 0.784520][ T1] __driver_probe_device+0x8c/0x170
> [ 0.784524][ T1] driver_probe_device+0x44/0x140
> [ 0.784528][ T1] __device_attach_driver+0xd8/0x180
> [ 0.784532][ T1] bus_for_each_drv+0x90/0xf8
> [ 0.784536][ T1] __device_attach+0xa8/0x1d0
> [ 0.784540][ T1] device_initial_probe+0x1c/0x30
> [ 0.784544][ T1] bus_probe_device+0xb4/0xc0
> [ 0.784547][ T1] device_add+0x4d0/0x700
> [ 0.784550][ T1] of_device_add+0x4c/0x78
> [ 0.784556][ T1] of_platform_device_create_pdata+0x9c/0x148
> [ 0.784560][ T1] of_platform_bus_create+0x1d0/0x370
> [ 0.784563][ T1] of_platform_bus_create+0x234/0x370
> [ 0.784567][ T1] of_platform_populate+0x84/0x178
> [ 0.784571][ T1] of_platform_default_populate_init+0xf0/0x120
> [ 0.784579][ T1] do_one_initcall+0x68/0x2d0
> [ 0.784585][ T1] kernel_init_freeable+0x2d8/0x358
> [ 0.784589][ T1] kernel_init+0x28/0x168
> [ 0.784595][ T1] ret_from_fork+0x10/0x20
>
> As this value is only read once, it doesn't require to be stable, so
> just use "raw_smp_processor_id" instead.
Can I ask what baseline you are running when you see this warning?
As this code got refactored recently in commit 78b72897a5c8 ("soc:
samsung: exynos-pmu: Enable CPU Idle for gs101") which is present in
linux-next but hasn't made its way to a proper release yet. After this
patch smp_processor_id() is always called with a raw_spin_lock() held
(so this warning shouldn't fire).
I just built next-20250909 locally to confirm, and with the above
patch reverted I see the warning you mention. So in summary I think
the issue has already been fixed by the above commit.
Thanks,
Peter
Hi Peter,
On Tue, Sep 09, 2025 at 12:14:51PM +0100, Peter Griffin wrote:
> Hi Mostafa,
>
> Thanks for your patch and bug report, it's great to see more folks
> running upstream Pixel 6 :)
>
> On Fri, 5 Sept 2025 at 17:25, Mostafa Saleh <smostafa@google.com> wrote:
> >
> > Booting the kernel on Pixel-6 with `CONFIG_DEBUG_PREEMPT` prints the
> > following WARN:
> >
> > [ 0.784187][ T1] BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
> > [ 0.784328][ T1] caller is debug_smp_processor_id+0x20/0x30
> > [ 0.784433][ T1] CPU: 6 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc4-gd69eb204c255 #1 PREEMPT
> > [ 0.784439][ T1] Hardware name: Oriole (DT)
> > [ 0.784441][ T1] Call trace:
> > [ 0.784443][ T1] show_stack+0x34/0xa0 (C)
> > [ 0.784453][ T1] dump_stack_lvl+0x7c/0xb0
> > [ 0.784460][ T1] dump_stack+0x18/0x24
> > [ 0.784464][ T1] check_preemption_disabled+0xf8/0x100
> > [ 0.784470][ T1] debug_smp_processor_id+0x20/0x30
> > [ 0.784476][ T1] gs101_cpuhp_pmu_online+0x40/0x108
> > [ 0.784483][ T1] cpuhp_invoke_callback+0x188/0x2d8
> > [ 0.784490][ T1] cpuhp_issue_call+0xec/0x240
> > [ 0.784494][ T1] __cpuhp_setup_state_cpuslocked+0x140/0x2c0
> > [ 0.784499][ T1] __cpuhp_setup_state+0x58/0x88
> > [ 0.784504][ T1] exynos_pmu_probe+0x2a4/0x380
> > [ 0.784508][ T1] platform_probe+0x64/0xd0
> > [ 0.784516][ T1] really_probe+0xd0/0x3b0
> > [ 0.784520][ T1] __driver_probe_device+0x8c/0x170
> > [ 0.784524][ T1] driver_probe_device+0x44/0x140
> > [ 0.784528][ T1] __device_attach_driver+0xd8/0x180
> > [ 0.784532][ T1] bus_for_each_drv+0x90/0xf8
> > [ 0.784536][ T1] __device_attach+0xa8/0x1d0
> > [ 0.784540][ T1] device_initial_probe+0x1c/0x30
> > [ 0.784544][ T1] bus_probe_device+0xb4/0xc0
> > [ 0.784547][ T1] device_add+0x4d0/0x700
> > [ 0.784550][ T1] of_device_add+0x4c/0x78
> > [ 0.784556][ T1] of_platform_device_create_pdata+0x9c/0x148
> > [ 0.784560][ T1] of_platform_bus_create+0x1d0/0x370
> > [ 0.784563][ T1] of_platform_bus_create+0x234/0x370
> > [ 0.784567][ T1] of_platform_populate+0x84/0x178
> > [ 0.784571][ T1] of_platform_default_populate_init+0xf0/0x120
> > [ 0.784579][ T1] do_one_initcall+0x68/0x2d0
> > [ 0.784585][ T1] kernel_init_freeable+0x2d8/0x358
> > [ 0.784589][ T1] kernel_init+0x28/0x168
> > [ 0.784595][ T1] ret_from_fork+0x10/0x20
> >
> > As this value is only read once, it doesn't require to be stable, so
> > just use "raw_smp_processor_id" instead.
>
> Can I ask what baseline you are running when you see this warning?
>
> As this code got refactored recently in commit 78b72897a5c8 ("soc:
> samsung: exynos-pmu: Enable CPU Idle for gs101") which is present in
> linux-next but hasn't made its way to a proper release yet. After this
> patch smp_processor_id() is always called with a raw_spin_lock() held
> (so this warning shouldn't fire).
>
> I just built next-20250909 locally to confirm, and with the above
> patch reverted I see the warning you mention. So in summary I think
> the issue has already been fixed by the above commit.
I am on commit
commit d69eb204c255c35abd9e8cb621484e8074c75eaa
Merge: 68f285e26478 9b2bfdbf43ad
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu Sep 4 09:59:15 2025 -0700
Merge tag 'net-6.17-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
Which doesn't have the commit you mentioned as it is in the Maintainer's
tree, thanks for pointing that.
Thanks,
Mostafa
>
> Thanks,
>
> Peter
On 05/09/2025 18:24, Mostafa Saleh wrote: > Booting the kernel on Pixel-6 with `CONFIG_DEBUG_PREEMPT` prints the > following WARN: > > [ 0.784187][ T1] BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1 > [ 0.784328][ T1] caller is debug_smp_processor_id+0x20/0x30 > [ 0.784433][ T1] CPU: 6 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc4-gd69eb204c255 #1 PREEMPT > [ 0.784439][ T1] Hardware name: Oriole (DT) > [ 0.784441][ T1] Call trace: > [ 0.784443][ T1] show_stack+0x34/0xa0 (C) > [ 0.784453][ T1] dump_stack_lvl+0x7c/0xb0 > [ 0.784460][ T1] dump_stack+0x18/0x24 > [ 0.784464][ T1] check_preemption_disabled+0xf8/0x100 > [ 0.784470][ T1] debug_smp_processor_id+0x20/0x30 > [ 0.784476][ T1] gs101_cpuhp_pmu_online+0x40/0x108 > [ 0.784483][ T1] cpuhp_invoke_callback+0x188/0x2d8 > [ 0.784490][ T1] cpuhp_issue_call+0xec/0x240 > [ 0.784494][ T1] __cpuhp_setup_state_cpuslocked+0x140/0x2c0 > [ 0.784499][ T1] __cpuhp_setup_state+0x58/0x88 > [ 0.784504][ T1] exynos_pmu_probe+0x2a4/0x380 > [ 0.784508][ T1] platform_probe+0x64/0xd0 > [ 0.784516][ T1] really_probe+0xd0/0x3b0 > [ 0.784520][ T1] __driver_probe_device+0x8c/0x170 > [ 0.784524][ T1] driver_probe_device+0x44/0x140 > [ 0.784528][ T1] __device_attach_driver+0xd8/0x180 > [ 0.784532][ T1] bus_for_each_drv+0x90/0xf8 > [ 0.784536][ T1] __device_attach+0xa8/0x1d0 > [ 0.784540][ T1] device_initial_probe+0x1c/0x30 > [ 0.784544][ T1] bus_probe_device+0xb4/0xc0 > [ 0.784547][ T1] device_add+0x4d0/0x700 > [ 0.784550][ T1] of_device_add+0x4c/0x78 > [ 0.784556][ T1] of_platform_device_create_pdata+0x9c/0x148 > [ 0.784560][ T1] of_platform_bus_create+0x1d0/0x370 > [ 0.784563][ T1] of_platform_bus_create+0x234/0x370 > [ 0.784567][ T1] of_platform_populate+0x84/0x178 > [ 0.784571][ T1] of_platform_default_populate_init+0xf0/0x120 > [ 0.784579][ T1] do_one_initcall+0x68/0x2d0 > [ 0.784585][ T1] kernel_init_freeable+0x2d8/0x358 > [ 0.784589][ T1] kernel_init+0x28/0x168 > [ 0.784595][ T1] ret_from_fork+0x10/0x20 Please trim from unnecessary parts - [time]. > > As this value is only read once, it doesn't require to be stable, so Why it does not need to be stable? Onlining wrong CPU number is not desired... > just use "raw_smp_processor_id" instead. You might be just hiding some other real issue, because above stacktrace is from gs101_cpuhp_pmu_online() which IRQs disabled and preemption disabled. Provide analysis of the warning, instead of just making it disappear. > > Cc: Peter Griffin <peter.griffin@linaro.org> > Cc: André Draszik <andre.draszik@linaro.org> > No blank lines between tags. Missing Fixes tag... and then Cc is not necessary. Please follow standard kernel process. > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > drivers/soc/samsung/exynos-pmu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c > index a77288f49d24..338f4758a089 100644 > --- a/drivers/soc/samsung/exynos-pmu.c > +++ b/drivers/soc/samsung/exynos-pmu.c > @@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle); > Best regards, Krzysztof
On Fri, Sep 05, 2025 at 07:09:09PM +0200, Krzysztof Kozlowski wrote: > On 05/09/2025 18:24, Mostafa Saleh wrote: > > Booting the kernel on Pixel-6 with `CONFIG_DEBUG_PREEMPT` prints the > > following WARN: > > > > [ 0.784187][ T1] BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1 > > [ 0.784328][ T1] caller is debug_smp_processor_id+0x20/0x30 > > [ 0.784433][ T1] CPU: 6 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc4-gd69eb204c255 #1 PREEMPT > > [ 0.784439][ T1] Hardware name: Oriole (DT) > > [ 0.784441][ T1] Call trace: > > [ 0.784443][ T1] show_stack+0x34/0xa0 (C) > > [ 0.784453][ T1] dump_stack_lvl+0x7c/0xb0 > > [ 0.784460][ T1] dump_stack+0x18/0x24 > > [ 0.784464][ T1] check_preemption_disabled+0xf8/0x100 > > [ 0.784470][ T1] debug_smp_processor_id+0x20/0x30 > > [ 0.784476][ T1] gs101_cpuhp_pmu_online+0x40/0x108 > > [ 0.784483][ T1] cpuhp_invoke_callback+0x188/0x2d8 > > [ 0.784490][ T1] cpuhp_issue_call+0xec/0x240 > > [ 0.784494][ T1] __cpuhp_setup_state_cpuslocked+0x140/0x2c0 > > [ 0.784499][ T1] __cpuhp_setup_state+0x58/0x88 > > [ 0.784504][ T1] exynos_pmu_probe+0x2a4/0x380 > > [ 0.784508][ T1] platform_probe+0x64/0xd0 > > [ 0.784516][ T1] really_probe+0xd0/0x3b0 > > [ 0.784520][ T1] __driver_probe_device+0x8c/0x170 > > [ 0.784524][ T1] driver_probe_device+0x44/0x140 > > [ 0.784528][ T1] __device_attach_driver+0xd8/0x180 > > [ 0.784532][ T1] bus_for_each_drv+0x90/0xf8 > > [ 0.784536][ T1] __device_attach+0xa8/0x1d0 > > [ 0.784540][ T1] device_initial_probe+0x1c/0x30 > > [ 0.784544][ T1] bus_probe_device+0xb4/0xc0 > > [ 0.784547][ T1] device_add+0x4d0/0x700 > > [ 0.784550][ T1] of_device_add+0x4c/0x78 > > [ 0.784556][ T1] of_platform_device_create_pdata+0x9c/0x148 > > [ 0.784560][ T1] of_platform_bus_create+0x1d0/0x370 > > [ 0.784563][ T1] of_platform_bus_create+0x234/0x370 > > [ 0.784567][ T1] of_platform_populate+0x84/0x178 > > [ 0.784571][ T1] of_platform_default_populate_init+0xf0/0x120 > > [ 0.784579][ T1] do_one_initcall+0x68/0x2d0 > > [ 0.784585][ T1] kernel_init_freeable+0x2d8/0x358 > > [ 0.784589][ T1] kernel_init+0x28/0x168 > > [ 0.784595][ T1] ret_from_fork+0x10/0x20 > > Please trim from unnecessary parts - [time]. Will do. > > > > > As this value is only read once, it doesn't require to be stable, so > > Why it does not need to be stable? Onlining wrong CPU number is not > desired... > > > just use "raw_smp_processor_id" instead. > > You might be just hiding some other real issue, because above stacktrace > is from gs101_cpuhp_pmu_online() which IRQs disabled and preemption > disabled. Provide analysis of the warning, instead of just making it > disappear. Not sure I understand, how is preemption disabled? that wouldn't fire in that case. I am not familiar with this driver, but as I see the value is used only once, it would be stable, for example using get/put_cpu won't really matter, because next access doesn't depend on the current CPU. Otherwise, if you imply that this might not be enough, that means the driver is already broken even without CONFIG_DEBUG_PREEMP (which is beyond my knowledge at this point) > > > > > Cc: Peter Griffin <peter.griffin@linaro.org> > > Cc: André Draszik <andre.draszik@linaro.org> > > > > No blank lines between tags. > > Missing Fixes tag... and then Cc is not necessary. Please follow > standard kernel process. I will add the Fixes. I am working with Peter and André, so I thought Cc is fine (according to the process) in: https://docs.kernel.org/process/5.Posting.html Thanks, Mostafa > > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > > drivers/soc/samsung/exynos-pmu.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c > > index a77288f49d24..338f4758a089 100644 > > --- a/drivers/soc/samsung/exynos-pmu.c > > +++ b/drivers/soc/samsung/exynos-pmu.c > > @@ -338,7 +338,7 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle); > > > > > Best regards, > Krzysztof
On 05/09/2025 19:43, Mostafa Saleh wrote: >>> >>> As this value is only read once, it doesn't require to be stable, so >> >> Why it does not need to be stable? Onlining wrong CPU number is not >> desired... >> >>> just use "raw_smp_processor_id" instead. >> >> You might be just hiding some other real issue, because above stacktrace >> is from gs101_cpuhp_pmu_online() which IRQs disabled and preemption >> disabled. Provide analysis of the warning, instead of just making it >> disappear. > > Not sure I understand, how is preemption disabled? that wouldn't fire > in that case. Because there is explicit preempt_disable(). So far you did not present any code analysis, any actual arguments, so deflecting reviewer's comment like above will get you nowhere. Instead of replying with a question, bring arguments that the code path does not disable the preemption. My call is that you run all this on some other kernel, just like usually happens in Google. > > I am not familiar with this driver, but as I see the value is used > only once, it would be stable, for example using get/put_cpu won't > really matter, because next access doesn't depend on the current CPU. > Otherwise, if you imply that this might not be enough, that means > the driver is already broken even without CONFIG_DEBUG_PREEMP > (which is beyond my knowledge at this point) > >> >>> >>> Cc: Peter Griffin <peter.griffin@linaro.org> >>> Cc: André Draszik <andre.draszik@linaro.org> >>> >> >> No blank lines between tags. >> >> Missing Fixes tag... and then Cc is not necessary. Please follow >> standard kernel process. > > I will add the Fixes. > > I am working with Peter and André, so I thought Cc is fine > (according to the process) in: > https://docs.kernel.org/process/5.Posting.html Please learn how Fixes and get_maintainers.pl work. Best regards, Krzysztof
Krzysztof, On Sat, Sep 06, 2025 at 09:07:02AM +0200, Krzysztof Kozlowski wrote: > On 05/09/2025 19:43, Mostafa Saleh wrote: > >>> > >>> As this value is only read once, it doesn't require to be stable, so > >> > >> Why it does not need to be stable? Onlining wrong CPU number is not > >> desired... > >> > >>> just use "raw_smp_processor_id" instead. > >> > >> You might be just hiding some other real issue, because above stacktrace > >> is from gs101_cpuhp_pmu_online() which IRQs disabled and preemption > >> disabled. Provide analysis of the warning, instead of just making it > >> disappear. > > > > Not sure I understand, how is preemption disabled? that wouldn't fire > > in that case. > > Because there is explicit preempt_disable(). Where do you see that? From what I can tell, the driver (somewhat bizarrely) registers its online callback much earlier (at CPUHP_BP_PREPARE_DYN) than the offline callback so that gs101_cpuhp_pmu_online() will be run in the context of the caller/onliner rather than the actual CPU coming up. I don't see anything which disables preemption on that path. > So far you did not present any code analysis, any actual arguments, so > deflecting reviewer's comment like above will get you nowhere. Instead > of replying with a question, bring arguments that the code path does not > disable the preemption. Mostafa's reported a bug and had a go at fixing it, for goodness sake. Would you rather not hear about bugs in the code you maintain? As somebody who should understand this code better than most, perhaps you could try a bit harder to fill in the blanks on the technical side rather than pointing out extraneous blank lines and making questionable judgements about CC lines. > My call is that you run all this on some other kernel, just like usually > happens in Google. Whilst that inevitably happens with some of the more product-facing teams, I think it's foolish to assume that nobody works directly with upstream at Google. We're also not going to waste time fabricating bug reports which is why we bother to reproduce issues on Pixel 6, as that can boot upstream without any additional patches. Will
On 09/09/2025 12:15, Will Deacon wrote: > Krzysztof, > > On Sat, Sep 06, 2025 at 09:07:02AM +0200, Krzysztof Kozlowski wrote: >> On 05/09/2025 19:43, Mostafa Saleh wrote: >>>>> >>>>> As this value is only read once, it doesn't require to be stable, so >>>> >>>> Why it does not need to be stable? Onlining wrong CPU number is not >>>> desired... >>>> >>>>> just use "raw_smp_processor_id" instead. >>>> >>>> You might be just hiding some other real issue, because above stacktrace >>>> is from gs101_cpuhp_pmu_online() which IRQs disabled and preemption >>>> disabled. Provide analysis of the warning, instead of just making it >>>> disappear. >>> >>> Not sure I understand, how is preemption disabled? that wouldn't fire >>> in that case. >> >> Because there is explicit preempt_disable(). > > Where do you see that? I did look at the code. All the calls I saw (including calltrace from commit msg) were under raw spinlock and raw spinlock does: preempt_disable(); > > From what I can tell, the driver (somewhat bizarrely) registers its > online callback much earlier (at CPUHP_BP_PREPARE_DYN) than the offline > callback so that gs101_cpuhp_pmu_online() will be run in the context of > the caller/onliner rather than the actual CPU coming up. I don't see > anything which disables preemption on that path. > >> So far you did not present any code analysis, any actual arguments, so >> deflecting reviewer's comment like above will get you nowhere. Instead >> of replying with a question, bring arguments that the code path does not >> disable the preemption. > > Mostafa's reported a bug and had a go at fixing it, for goodness sake. > Would you rather not hear about bugs in the code you maintain? I am happy to see such patch, but Mostafa instead of replying with anything useful just shoved back my comment back to me. > > As somebody who should understand this code better than most, perhaps > you could try a bit harder to fill in the blanks on the technical side I did and I investigated the code and I cannot find the issue leading to it, thus I asked. And then contributor just replied whatever so I will go away and stop bothering them? > rather than pointing out extraneous blank lines and making questionable > judgements about CC lines. > >> My call is that you run all this on some other kernel, just like usually >> happens in Google. > > Whilst that inevitably happens with some of the more product-facing > teams, I think it's foolish to assume that nobody works directly with > upstream at Google. We're also not going to waste time fabricating bug I saw many, many bug reports from Google and Linaro done on Android kernel. Really, too many. > reports which is why we bother to reproduce issues on Pixel 6, as that > can boot upstream without any additional patches. Best regards, Krzysztof
On Tue, Sep 09, 2025 at 05:43:16PM +0200, Krzysztof Kozlowski wrote: > On 09/09/2025 12:15, Will Deacon wrote: > > Krzysztof, > > > > On Sat, Sep 06, 2025 at 09:07:02AM +0200, Krzysztof Kozlowski wrote: > >> On 05/09/2025 19:43, Mostafa Saleh wrote: > >>>>> > >>>>> As this value is only read once, it doesn't require to be stable, so > >>>> > >>>> Why it does not need to be stable? Onlining wrong CPU number is not > >>>> desired... > >>>> > >>>>> just use "raw_smp_processor_id" instead. > >>>> > >>>> You might be just hiding some other real issue, because above stacktrace > >>>> is from gs101_cpuhp_pmu_online() which IRQs disabled and preemption > >>>> disabled. Provide analysis of the warning, instead of just making it > >>>> disappear. > >>> > >>> Not sure I understand, how is preemption disabled? that wouldn't fire > >>> in that case. > >> > >> Because there is explicit preempt_disable(). > > > > Where do you see that? > > I did look at the code. > > All the calls I saw (including calltrace from commit msg) were under raw > spinlock and raw spinlock does: > > preempt_disable(); The backtrace doesn't contain a raw spinlock. As Peter subsequently pointed out, the reported issue has been fixed in linux-next and there's a raw spinlock there but since the report is from vanilla -rc4, it doesn't have that fix. Will
On 09/09/2025 19:03, Will Deacon wrote: > On Tue, Sep 09, 2025 at 05:43:16PM +0200, Krzysztof Kozlowski wrote: >> On 09/09/2025 12:15, Will Deacon wrote: >>> Krzysztof, >>> >>> On Sat, Sep 06, 2025 at 09:07:02AM +0200, Krzysztof Kozlowski wrote: >>>> On 05/09/2025 19:43, Mostafa Saleh wrote: >>>>>>> >>>>>>> As this value is only read once, it doesn't require to be stable, so >>>>>> >>>>>> Why it does not need to be stable? Onlining wrong CPU number is not >>>>>> desired... >>>>>> >>>>>>> just use "raw_smp_processor_id" instead. >>>>>> >>>>>> You might be just hiding some other real issue, because above stacktrace >>>>>> is from gs101_cpuhp_pmu_online() which IRQs disabled and preemption >>>>>> disabled. Provide analysis of the warning, instead of just making it >>>>>> disappear. >>>>> >>>>> Not sure I understand, how is preemption disabled? that wouldn't fire >>>>> in that case. >>>> >>>> Because there is explicit preempt_disable(). >>> >>> Where do you see that? >> >> I did look at the code. >> >> All the calls I saw (including calltrace from commit msg) were under raw >> spinlock and raw spinlock does: >> >> preempt_disable(); > > The backtrace doesn't contain a raw spinlock. As Peter subsequently How backtrace could contain spinlock? Backtrace points you the calls and clearly in these calls in the source code there is raw spinlock. > pointed out, the reported issue has been fixed in linux-next and there's Not in Linux next, but in maintainer tree. Who is going to apply the patch? Maintainer. On what tree? Maintainer's tree. > a raw spinlock there but since the report is from vanilla -rc4, it > doesn't have that fix. So that's the argument - instead of trying maintainer's tree or linux-next, or even trying to put some effort and investigate why maintainer claims that "gs101_cpuhp_pmu_online() which IRQs disabled and preemption disabled." you just respond "there is no fix in rc release". I gave the technical objection based on reading code. Contributor instead of checking this or doing basic work - developing on tree which has fixes, just bounced back. And you complain that I "could try a bit harder to fill". Keep such comments to yourself. Best regards, Krzysztof
On 09/09/2025 17:43, Krzysztof Kozlowski wrote: > On 09/09/2025 12:15, Will Deacon wrote: >> Krzysztof, >> >> On Sat, Sep 06, 2025 at 09:07:02AM +0200, Krzysztof Kozlowski wrote: >>> On 05/09/2025 19:43, Mostafa Saleh wrote: >>>>>> >>>>>> As this value is only read once, it doesn't require to be stable, so >>>>> >>>>> Why it does not need to be stable? Onlining wrong CPU number is not >>>>> desired... >>>>> >>>>>> just use "raw_smp_processor_id" instead. >>>>> >>>>> You might be just hiding some other real issue, because above stacktrace >>>>> is from gs101_cpuhp_pmu_online() which IRQs disabled and preemption >>>>> disabled. Provide analysis of the warning, instead of just making it >>>>> disappear. >>>> >>>> Not sure I understand, how is preemption disabled? that wouldn't fire >>>> in that case. >>> >>> Because there is explicit preempt_disable(). >> >> Where do you see that? > > I did look at the code. > > All the calls I saw (including calltrace from commit msg) were under raw > spinlock and raw spinlock does: > > preempt_disable(); > >> >> From what I can tell, the driver (somewhat bizarrely) registers its >> online callback much earlier (at CPUHP_BP_PREPARE_DYN) than the offline >> callback so that gs101_cpuhp_pmu_online() will be run in the context of >> the caller/onliner rather than the actual CPU coming up. I don't see >> anything which disables preemption on that path. >> >>> So far you did not present any code analysis, any actual arguments, so >>> deflecting reviewer's comment like above will get you nowhere. Instead >>> of replying with a question, bring arguments that the code path does not >>> disable the preemption. >> >> Mostafa's reported a bug and had a go at fixing it, for goodness sake. >> Would you rather not hear about bugs in the code you maintain? > > I am happy to see such patch, but Mostafa instead of replying with > anything useful just shoved back my comment back to me. > > >> >> As somebody who should understand this code better than most, perhaps >> you could try a bit harder to fill in the blanks on the technical side > > I did and I investigated the code and I cannot find the issue leading to > it, thus I asked. > > And then contributor just replied whatever so I will go away and stop > bothering them? > >> rather than pointing out extraneous blank lines and making questionable >> judgements about CC lines. >> >>> My call is that you run all this on some other kernel, just like usually >>> happens in Google. >> >> Whilst that inevitably happens with some of the more product-facing >> teams, I think it's foolish to assume that nobody works directly with >> upstream at Google. We're also not going to waste time fabricating bug > > I saw many, many bug reports from Google and Linaro done on Android > kernel. Really, too many. And I read now reply from Peter which nicely points that this bug report was not done on something older. Again my pure guess - some Android kernel. @Will, I pointed out - except new lines - real issue with this patch and explain in gs101_cpuhp_pmu_online() why I think that. Contributor did not even bother to check that logic I pointed out, so your scoffing is inappropriate. Best regards, Krzysztof
© 2016 - 2025 Red Hat, Inc.