linux-kernel - Re: [PATCH v2] drivers: irqchip: add irq-type-changer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a179c80c-7ce8-da1a-f344-5d72b65c3da4@cogentembedded.com>
Date:   Tue, 25 Jan 2022 08:47:37 +0300
From:   Nikita Yushchenko <nikita.yoush@...entembedded.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Eugeniu Rosca <erosca@...adit-jv.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drivers: irqchip: add irq-type-changer

Hi

 > I also don't see why you model this as the actual device that triggers
 > the interrupt.

Well, that somehow matches the physical reality. In the case of wl18xx on KF, physically the interrupt 
signal indeed originates from the intermediate chip - the inverting level-shifter.

I remember your suggestion to configure interrupt source's node with interrupt-parent set to inverter 
and interrupt details for inverter's parent. But this looks hacky and inconsistent for me.

In contrary, an abstraction of intermediate entity that does a static conversion of the trigger type and 
does not need any software control, looks sane. And, hardware designers do strange things sometimes, I 
won't be surprised observing a change from level to edge one day. Thus it looked a good idea not to 
limit the conversion to inversion, but support arbitrary one. Then, irqspec inside the node for the 
intermediate entity obtains a meaning, making the entire model consistent.

I don't strongly insist on this approach, it just looks cleaner for me.

 > I'm also pretty sure that with the current code,
 > you end-up with *two* interrupts (one for the inverter and one for the
 > end-point).

In driver's init, I only call of_irq_parse_one() for interrupt defined in the changer's node. This does 
not create a mapping for it. The mapping is only created when changer's "interrupt-child" creates a 
mapping for their interrupt - then changer's alloc() routine calls irq_domain_alloc_irqs_parent() in the 
same way as all other hierarchical irqchips do.

I don't see where double mapping can appear here. Please explain.

 > You need a proper DT binding...
 > Geert commented on why this is wrong...
 > Use struct_size()...

Will fix all that after we negotiate the basic approach.

Thanks,

Nikita