linux-kernel - Re: [PATCH 07/11] x86, olpc-xo1-sci: Add GPE handler and ebook switch functionality

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: <BANLkTimpFYMpQFY9J7pa0NygGoQ8dLdArA@mail.gmail.com>
Date:	Tue, 24 May 2011 22:40:25 +0100
From:	Daniel Drake <dsd@...top.org>
To:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:	x86@...nel.org, linux-kernel@...r.kernel.org, dilinger@...ued.net
Subject: Re: [PATCH 07/11] x86, olpc-xo1-sci: Add GPE handler and ebook switch functionality

On 16 May 2011 10:08, Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
>>+
>>+      /* Enable EC SCI events */
>>+      cs5535_gpio_set(OLPC_GPIO_ECSCI, GPIO_EVENTS_ENABLE);
>>+
>>+      /* Set the SCI to cause a PME event on group 7 */
>>+      cs5535_gpio_setup_event(OLPC_GPIO_ECSCI, 7, 1);
>>+
>>+      /* And have group 7 also fire the SCI interrupt */
>>+      cs5535_gpio_set_irq(7, sci_irq);
>
> What do you do here? Could this be hidden behind a gpio irq_chip or is
> this too olpc specific?

Thanks for looking at these patches.

I looked into irq_chip alongside the CS5535 docs. Initially this
looked like a good suggestion - any CS5535 GPIO can be mapped to one
of 8 PIC-level interrupts, which seems like a good match for irq_chip.

However, this isn't what we're doing here.

CS5535 GPIOs can either be mapped to interrupts, *or* to a Power
Management Event (PME). A PME is a CS5535-specific concept; its an
event that will bring the system out of suspend if it is sleeping. In
this case, we are mapping a GPIO to a PME. I'm not aware of any
appropriate Linux abstraction for this. Thats all done in
cs5535_gpio_setup_event().

The CS5535 also has a highly programmable PIC, with loads of possible
input and mapping options. The call to cs5535_gpio_set_irq() then
programs the PIC to map PME input to an IRQ line. This function is
both misnamed and misplaced; it is unrelated to GPIOs and is simply
one of the many generic knobs on the PIC. If there was one, this
function should belong in a cs5535-pic driver, named something
according to "IRQ Mapper Unrestricted Z Select High".

So, I'm left feeling that irq_chip isn't an appropriate abstraction,
because the mapping made to the IRQ line is indirect via PME events,
and that is significant in the context of this driver (where wakeup
control is a big thing). However, it may be worthwhile moving
cs5535_gpio_set_irq() into cs5535.h as a static inline with a more
appropriate name. I could also add some explanatory comments in this
part of the code.

What do you think?

Thanks,
Daniel
--
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/