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
