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]

Date:   Tue, 25 Jan 2022 10:08:00 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Nikita Yushchenko <nikita.yoush@...entembedded.com>
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

On Tue, 25 Jan 2022 09:35:22 +0000,
Nikita Yushchenko <nikita.yoush@...entembedded.com> wrote:
> 
> >>> 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.
> > 
> > Reality? By allowing something like an edge-to-level conversion? How
> > can that work?
> 
> Edge to level can be problematic, but level to edge does not cause any
> difficulties, nor in generating nor in handling.

Hmmm. A whole industry seems to disagree with you.

> 
> >> 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.
> > 
> > If you think that it can happen without a HW register that latches the
> > edge and requires an ack, you need to question your understanding of
> > an interrupt life cycle, and of the properties of the various trigger
> > types.
> 
> There are plenty of devices capable to signal both level and edge
> interrupts, configurable by a register. Basic handling is always the
> same, and involves masking the interrupt at interrupt controller while
> IRQs disabled, then enabling IRQs, then programming the device to
> clear the interrupt condition, then unmasking the interrupt at
> interrupt controller. If the trigger type is level or edge, is only
> interesting to interrupt controller driver, but not to a wider scope.
> 
> Nothing stops hw designers from doing all sort of strange things with
> interrupt signals. Right now I have a board on my desk where interrupt
> signals from several chips are wired to inputs of a logical AND
> element and the output of that is wired to SoC's gpio. I don't see
> what stops them tomorrow from setting up their CPLDs to issue a short
> impulse at output in return to a level change on input. And that will
> be a level to edge converter.

And a broken one, as you'd have nothing to regenerate an edge if the
input stays high. Such HW can stay in the bin.

> 
> >> 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.
> > 
> > Just look at your code. You start probing a device that has an
> > 'interrupts' property. This triggers the allocation of an interrupt.
> 
> Where does it trigger it?

I'll let you follow the path from of_device_alloc() all the way to the
actual DT parsing of the interrupt specifier, allocation of the
interrupt descriptor and mapping in the parent domain. You then
override the mapping (I really should put a WARN_ON() in the irqdomain
code to catch this sort of things).

> And what is this "allocation", after all? Is it allocation of virq
> number?

And descriptor, and mapping in the domain.

> That allocation happens when irq_create_fwspec_mapping() calls
> irq_domain_alloc_irqs(). But this path does not necessary gets
> executed in probing. In the current irq_tyoe_changer driver, it is
> not.

I don't think you see the full extent of the problem.

	M.

-- 
Without deviation from the norm, progress is not possible.