Re: [PATCH] soc: samsung: exynos-pmu: Fix for CONFIG_DEBUG_PREEMPT

[Krzysztof Kozlowski <krzk@kernel.org>]

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