linux-kernel - Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support

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]

Date:	Fri, 8 Apr 2016 11:38:30 +0300
From:	Mika Westerberg <mika.westerberg@...ux.intel.com>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	qiujiang <qiujiang@...wei.com>,
	Alexandre Courbot <gnurou@...il.com>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Alan Tull <delicious.quinoa@...il.com>,
	Jamie Iles <jamie@...ieiles.com>, charles.chenxin@...wei.com,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Linuxarm <linuxarm@...wei.com>
Subject: Re: [PATCH v7 3/3] gpio: dwapb: add gpio-signaled acpi event support

On Fri, Apr 08, 2016 at 10:26:28AM +0200, Linus Walleij wrote:
> On Wed, Apr 6, 2016 at 9:08 AM, qiujiang <qiujiang@...wei.com> wrote:
> 
> > This patch adds gpio-signaled acpi event support. It is used for
> > power button on hisilicon D02 board, an arm64 platform.
> >
> > The corresponding DSDT file is defined as follows:
> >  Device(GPI0) {
> >         Name(_HID, "HISI0181")
> >         Name(_ADR, 0)
> >         Name(_UID, 0)
> >
> >         Name (_CRS, ResourceTemplate ()  {
> >                 Memory32Fixed (ReadWrite, 0x802e0000, 0x10000)
> >                 Interrupt (ResourceConsumer, Level, ActiveHigh,
> >                 Exclusive,,,)  {344}
> >         })
> >
> >         Device(PRTa) {
> >                 Name (_DSD, Package () {
> >                 Package () {
> >                         Package () {"reg",0},
> >                         Package () {"snps,nr-gpios",32},
> >                         }
> >                 })
> >         }
> >
> >         Name (_AEI, ResourceTemplate () {
> >                 GpioInt(Edge, ActiveLow, ExclusiveAndWake,
> >                 PullUp, , " \\_SB.GPI0") {8}
> >         })
> >
> >         Method (_E08, 0x0, NotSerialized) {
> >                 Notify (\_SB.PWRB, 0x80)
> >         }
> > }
> >
> > Acked-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> > Signed-off-by: qiujiang <qiujiang@...wei.com>
> 
> Admittedly I'm an ACPI novice and need help with deciding
> about ACPI, but I mostly trust Mika to know these things right.
> 
> About this:
> 
> > +       /* Add GPIO-signaled ACPI event support */
> > +       if (pp->irq)
> > +               acpi_gpiochip_request_interrupts(&port->gc);
> 
> It's weird to me that the driver already has a requested IRQ and
> everything, now it has to request it again from ACPI.

This is different thing, though.

Calling acpi_gpiochip_request_interrupts() results _AEI ACPI method
being evaluated that returns a list of GPIOs which are used as event
sources. acpi_gpiochip_request_interrupts() then goes and installs
interrupt handler per each GPIO in that list.

> When I look into the acpi_gpiochip_request_interrupts()
> I find it weird that it is void given how much can go wrong
> inside it. Should it not return an errorcode?

Currently it just complains if something goes wrong. The GPIO driver
itself can still work just fine (including interrupts).

I'm fine to change it to return an error code.

> > +               if (has_acpi_companion(dev) && pp->idx == 0)
> > +                       pp->irq = platform_get_irq(to_platform_device(dev), 0);
> 
> As it was already fetched here and then later requested,
> we still have to call acpi_gpiochip_request_interrupts()
> further down the road? That is confusing to me, can you
> explain what is going on?

See above.