Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 19 Nov 2019 22:29:56 +0000
From:   Marc Zyngier <maz@kernel.org>
To:     Andreas =?UTF-8?Q?F=C3=A4rber?= <afaerber@suse.de>
Cc:     linux-realtek-soc@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Aleix Roca Nonell <kernelrocks@gmail.com>,
        James Tai <james.tai@realtek.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>
Subject: Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver
Message-ID: <20191119222956.23665e5d@why>
In-Reply-To: <e98364c5-a859-7981-8ccf-f8e5b5069379@suse.de>
References: <20191119021917.15917-1-afaerber@suse.de>
        <20191119021917.15917-3-afaerber@suse.de>
        <a34e00cac16899b53d0b6445f0e81f4c@www.loen.fr>
        <e98364c5-a859-7981-8ccf-f8e5b5069379@suse.de>
Organization: Approximate
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 19 Nov 2019 21:56:48 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 19.11.19 um 13:01 schrieb Marc Zyngier:
> > On 2019-11-19 02:19, Andreas Färber wrote:  
> >> +static void rtd1195_mux_enable_irq(struct irq_data *data)
> >> +{
> >> +    struct rtd1195_irq_mux_data *mux_data =
> >> irq_data_get_irq_chip_data(data);
> >> +    unsigned long flags;
> >> +    u32 mask;
> >> +
> >> +    mask = mux_data->info->isr_to_int_en_mask[data->hwirq];
> >> +    if (!mask)
> >> +        return;  
> > 
> > How can this happen? You've mapped the interrupt, so it exists.
> > I can't see how you can decide to fail such enable.  
> 
> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same.
> 
> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but
> are sparsely populated. So there are circumstances such as WDOG_NMI as
> well as reserved bits that we cannot enable.

But the you should have failed the map. The moment you allow the
mapping to occur, you have accepted the contract that this interrupt is
usable.

> This check should be
> identical to v3; the equivalent mask check inside the interrupt handler
> was extended with "mask &&" to do the same in this v4.

Spurious interrupts are a different matter. What I'm objecting to here
is a simple question of logic, whether or not you are allowed to fail
enabling an interrupt that you've otherwise allowed to be populated.
 
	M.
-- 
Jazz is not dead. It just smells funny...