Re: [patch V2 26/45] genirq/chip: Rework irq_set_handler() variants

[Jiri Slaby <jirislaby@kernel.org>]

On 12. 05. 25, 9:25, Jiri Slaby wrote:
> On 11. 05. 25, 19:49, Nathan Chancellor wrote:
>> On Sun, May 11, 2025 at 07:29:11PM +0200, Miguel Ojeda wrote:
>>> On Fri, 09 May 2025 14:22:11 +0100 Nathan Chancellor 
>>> <nathan@kernel.org> wrote:
>>>>
>>>> I am investigating some cases where
>>>>
>>>> WARN(!irqs_disabled(), "Interrupts were enabled early\n");
>>>>
>>>> in start_kernel() in init/main.c is triggered in certain builds with
>>>> clang after patch 23 of this series (very bizarre since the conversion
>>>> seems to be correct) and I happened to notice that this conversion 
>>>> seems
>>>> to be incorrect? Should this be scoped_irqdesc_get_and_buslock() like
>>>> below?
>>>
>>> Yeah, I am also seeing this in next-20250509 in a LLVM=1 arm64 
>>> defconfig + Rust
>>> build under QEMU.
>>
>> I noticed that the warning was reproducible with just the first patch of
>> the series that adds the lock guards and patch 23 but also several other
>> individual patches within the series, as I could not just revert patch
>> 23 on next-20250509 to fix it. I have no idea why yet because I have not
>> had the chance to actually sit down and dig into it but this diff fixes
>> every instance of the warning that I saw in my tests... :/ could be a
>> compiler bug or just some difference in behavior between compilers.
>>
>> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
>> index bd2db6ebb98e..94f463de8f26 100644
>> --- a/kernel/irq/internals.h
>> +++ b/kernel/irq/internals.h
>> @@ -176,10 +176,9 @@ __DEFINE_UNLOCK_GUARD(irqdesc_lock, struct irq_desc,
>>   static inline class_irqdesc_lock_t 
>> class_irqdesc_lock_constructor(unsigned int irq, bool bus,
>>                                     unsigned int check)
>>   {
>> -    class_irqdesc_lock_t _t = {
>> -        .bus    = bus,
>> -        .lock    = __irq_get_desc_lock(irq, &_t.flags, bus, check),
> 
> I assume the value stored by __irq_get_desc_lock() to &_t.flags is 
> overwritten by 0 by the initializer. class_irqdesc_lock_t::flags is 
> later than ::lock in the structure, so __irq_get_desc_lock() should be 
> called, setting ::flags, then the initializer should set flags to 0.
> 
>> -    };
>> +    class_irqdesc_lock_t _t = {};
>> +    _t.bus    = bus;
>> +    _t.lock    = __irq_get_desc_lock(irq, &_t.flags, bus, check);
> 
> That's why this works ^^.

In fact, this should work around the issue too:
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -178,6 +178,7 @@ static inline class_irqdesc_lock_t 
class_irqdesc_lock_constructor(unsigned int i
  {
         class_irqdesc_lock_t _t = {
                 .bus    = bus,
+               .flags  = 0,
                 .lock   = __irq_get_desc_lock(irq, &_t.flags, bus, check),
         };
         return _t;


-- 
js
suse labs