Re: [PATCH v2] of/irq: Make sure to update out_irq->np to the new parent in of_irq_parse_raw

["WHR" <whr@rivoreo.one>]

> On Wed, Jul 31, 2024 at 10:22 PM WHR <whr@rivoreo.one> wrote:
>>
>> > On Mon, Jul 29, 2024 at 11:54 PM WHR <whr@rivoreo.one> wrote:
>> >>
>> >> Commit 935df1bd40d43c4ee91838c42a20e9af751885cc has removed an
>> >> assignment statement for 'out_irq->np' right after label 'skiplevel',
>> >> causing the new parent acquired from function of_irq_find_parent didn't
>> >> being stored to 'out_irq->np' as it supposed to. Under some conditions
>> >> this can resuit in multiple corruptions and leakages to device nodes.
>> >
>> > Under what conditions? Please provide a specific platform and DT.
>>
>> I have a previous email sent to you before I came up with the fix. The
>> kernel
>> log for debugging and the device tree blob are attached again.
> 
> Thanks. The patch needs to stand on its own with this detail, not
> require that I've read (and remember) some other email among the
> 1000s.
> 
> "multiple corruptions and leakages to device nodes" is meaningless. Be
> exact, it's device_node refcounts we're talking about. The issue is
> out_irq->np is not updated from 'usbdrd' node to the real interrupt
> parent, the 'plic' node. In the next iteration of the loop, we find
> 'interrupt-controller' in the plic node and return, but out_irq is not
> pointing to the plic. Then of_irq_get() fails to get the irq host and
> does a put on out_irq->np which is usbdrd, not plic node.
> 
> So please update the commit msg and provide your name, not initials.

Since the fix for this regression is really trivial, I think you'll be able to
commit it by yourself instead.


>> > Honestly, I think the DT is wrong if you get to this point. We'd have
>> > to have the initial interrupt parent with #interrupt-cells, but not an
>> > interrupt-controller nor interrupt-map property to get here. Maybe
>> > that happens in some ancient platform, but if so, I want to know which
>> > one and what exactly we need to handle.
>>
>> So you suggest the #interrupt-cells is erroneous in that node, and should
>> be
>> removed?
> 
> Yes. dtc warns about this. dtschema would too if there was a schema
> (there is, but not since you use a downstream binding).
> 
> The clint node has the same issue.
> 
>> This is a device vendor-provided DT, so any issue in it will have to be
>> fixed
>> locally.
> 
> Complain to your vendor...

Thanks for help diagnosing the issues.