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] [day] [month] [year] [list]

Message-ID: <8b5681a0-a9ab-8ad7-b293-ce1342e43099@cogentembedded.com>
Date:   Tue, 25 Jan 2022 22:33:47 +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

 > Such HW can stay in the bin.

Huh? Industry is full of ugly hardware (that is still surprisingly useful to cover practical needs at 
low cost - which ensures it will persist). Sure I'd prefer to spend time for something better than 
negotiating workarounds to keep ugly hardware working. But after 20+ years in the industry, I can only 
agree with the statement Linus wrote back in 2006 in [1]:

| For a driver writer, there is one rule above _all_ other rules:
|
|	"Reality sucks, deal with it"
|
| That rule is inviolate, and no amount of "I wish", and "it _should_
| work this way" or "..but the documentation says" matters at all.

[1] https://lore.kernel.org/lkml/Pine.LNX.4.64.0604241156340.3701@g5.osdl.org/

Can we finally stop blaming and propose a practical way to solve the original issue?

If the only solution you are ready to accept is the code that supports inversion only, and uses node's 
"interrupts" property to provide info for interrupt-grand-parent, please state that explicitly.

<skipping all the statements I don't agree with>

 > (I really should put a WARN_ON() in the irqdomain
 > code to catch this sort of things).

I do support this idea.

Is a WARN_ON() in irq_domain_set_mapping() checking that nothing is already assigned to this hwirq in 
this domain good enough to serve this purpose?

I.e. for a linear domain (which shall be enough to catch the wl18xx-on-kf case where both parent domain 
and irq-type-changer's domain are linear):

--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -521,9 +521,10 @@ static void irq_domain_set_mapping(struct irq_domain *domain,
                 return;

         mutex_lock(&domain->revmap_mutex);
-       if (hwirq < domain->revmap_size)
+       if (hwirq < domain->revmap_size) {
+               WARN_ON(domain->revmap[hwirq] != NULL);
                 rcu_assign_pointer(domain->revmap[hwirq], irq_data);
-       else
+       } else
                 radix_tree_insert(&domain->revmap_tree, hwirq, irq_data);
         mutex_unlock(&domain->revmap_mutex);
  }

This does not hit on my code.

Could you please suggest a better location for WARN_ON() that will hit?

Nikita