linux-kernel - Re: [PATCH v4 03/10] irqchip/sun6i-r: Use a stacked irqchip driver

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: <495f71d0-9dbf-23d2-45a1-f27b15138bb2@sholland.org>
Date:   Thu, 14 Jan 2021 22:01:59 -0600
From:   Samuel Holland <samuel@...lland.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Ondrej Jirman <megous@...ous.com>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 03/10] irqchip/sun6i-r: Use a stacked irqchip driver

Hello,

On 1/14/21 3:06 PM, Marc Zyngier wrote:
> Hi Samuel,
> 
> On 2021-01-12 05:59, Samuel Holland wrote:
> 
> [...]
> 
>> +static void sun6i_r_intc_ack_nmi(void)
>> +{
>> +	writel(SUN6I_NMI_BIT, base + SUN6I_IRQ_PENDING(0));
> 
> writel_relaxed()

irq_chip_unmask_parent(), which calls gic_unmask_irq(), is called
immediately after this in .irq_unmask. Since gic_unmask_irq() also uses
writel_relaxed(), the GIC write could be ordered before the write here.

I was getting occasional spurious interrupts (1 out of each 20-25) when
using a level trigger, which were resolved by switching to writel() here.

I mentioned this in the changelog, but it probably deserves a comment in
the code as well. Or maybe I should use an explicit barrier somewhere?

>> +}
>> +
>> +static void sun6i_r_intc_nmi_ack(struct irq_data *data)
>> +{
>> +	if (irqd_get_trigger_type(data) & IRQ_TYPE_EDGE_BOTH)
>> +		sun6i_r_intc_ack_nmi();
>> +	else
>> +		data->chip_data = SUN6I_NMI_NEEDS_ACK;
>> +}
>> +
>> +static void sun6i_r_intc_nmi_eoi(struct irq_data *data)
>> +{
>> +	/* For oneshot IRQs, delay the ack until the IRQ is unmasked. */
>> +	if (data->chip_data == SUN6I_NMI_NEEDS_ACK && !irqd_irq_masked(data)) 
>> {
>> +		sun6i_r_intc_ack_nmi();
>> +		data->chip_data = 0;
> 
> nit: NULL rather than 0?

NULL seemed less appropriate since I'm not using the field as a pointer,
but I don't have a strong opinion about it.

> [...]
> 
>> +static struct irq_chip sun6i_r_intc_nmi_chip = {
>> +	.name			= "sun6i-r-intc",
>> +	.irq_ack		= sun6i_r_intc_nmi_ack,
>> +	.irq_mask		= irq_chip_mask_parent,
>> +	.irq_unmask		= sun6i_r_intc_nmi_unmask,
>> +	.irq_eoi		= sun6i_r_intc_nmi_eoi,
>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>> +	.irq_set_type		= sun6i_r_intc_nmi_set_type,
>> +	.irq_set_irqchip_state	= sun6i_r_intc_nmi_set_irqchip_state,
> 
> You probably also want to wire irq_get_irqchip_state(), while
> you're at it.

I thought if the interrupt was pending here, it would necessarily also
be pending at the GIC, so adding a separate layer would be redundant.

irq_set_vcpu_affinity(), __irq_get_irqchip_state(), and
irq_set_irqchip_state() [the functions, not the callbacks] have the
interesting property that they search up the irqdomain hierarchy for the
first irqdomain with the callback. So if all the callback would do is
defer to its parent, it doesn't need to be provided at all*.

*except in case this irqdomain has a child which calls
irq_chip_get_parent_state(), which does not look past its immediate
parent. But I did not think that case was worth worrying about.

Cheers,
Samuel

> Otherwise, looks pretty good now.
> 
> Thanks,
> 
> 	M.
>