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

Date:	Mon, 1 Dec 2014 12:36:56 +0000
From:	Liviu Dudau <Liviu.Dudau@....com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Marc Zyngier <Marc.Zyngier@....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:54:00AM +0000, Russell King - ARM Linux wrote:
> 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.

I was trying to make the smallest amount of changes possible. Given that
ARM's GICs don't distinguish between high/low or raising/falling all I wanted
was to get the level vs edge type being sticky.

> 
> 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.

OK, I will send a v2 patch, doing the right thing.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

--
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/