linux-kernel - Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

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: <20141201115400.GD3836@n2100.arm.linux.org.uk>
Date:	Mon, 1 Dec 2014 11:54:00 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Marc Zyngier <marc.zyngier@....com>
Cc:	Liviu Dudau <Liviu.Dudau@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <Mark.Rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	LAKML <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] irqchip: gic: Allow interrupt level to be set for PPIs.

On Mon, Dec 01, 2014 at 11:31:05AM +0000, Marc Zyngier wrote:
> On 01/12/14 11:23, Russell King - ARM Linux wrote:
> > On Mon, Dec 01, 2014 at 11:19:41AM +0000, Marc Zyngier wrote:
> >> Hi Russell,
> >>
> >> On 01/12/14 11:03, Russell King - ARM Linux wrote:
> >>> If all you want to do is to bypass the following check, what's wrong
> >>> with actually doing that:
> >>>
> >>> -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> >>> +	if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
> >>> +	    type != IRQ_TYPE_EDGE_RISING)
> >>> 		return -EINVAL;
> >>>
> >>
> >> I think that will require some additional changes to gic_configure_irq
> >> (in irq-gic-common.c).
> > 
> > I don't think so - gic_configure_irq() will treat it as a no-op as far
> > as trying to configure the IRQ settings.
> 
> I agree. But that's following ARM's tradition of making PPIs
> non-configurable. I seem to remember that there is at least one
> occurrence of a GIC with configurable PPIs (Qualcomm, IIRC).
> 
> With this use case in mind, Liviu's patch allows an active-low interrupt
> to be correctly configured as level, for example.

Liviu's patch is nothing more than a hack.  It changes (eg) the active
low level to be an active high level with the explicitly stated reason to
bypass a test, and then hopes that the remaining functions do the right
thing.

It would be better to explicitly bypass the test, and then explicitly
handle other cases in gic_configure_irq().

It would be even better to make the code reflect reality right the way
through.  If PPIs are non-configurable, then we should return -EINVAL if
trying to set them to a setting which is not supported.  For example,
pass through all states to gic_configure_irq(), and have gic_configure_irq()
return whether the state is valid.

Then, gic_configure_irq() can read back the register after trying to set
the appropriate value, and see whether it was taken.  If the bits remain
unchanged, then it can decide that the requested mode is not supported,
and return -EINVAL.

In any case, let's not hack the code in the way that Liviu is trying to
do.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/