| 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
| ||
|
Message-ID: <1425934566.2312.16.camel@synopsys.com>
Date: Mon, 9 Mar 2015 20:56:06 +0000
From: Alexey Brodkin <Alexey.Brodkin@...opsys.com>
To: "linus.walleij@...aro.org" <linus.walleij@...aro.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Vineet.Gupta1@...opsys.com" <Vineet.Gupta1@...opsys.com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"gnurou@...il.com" <gnurou@...il.com>
Subject: Re: [PATCH] gpio-dwapb: reset mask register on probe
Hi Linus,
On Mon, 2015-03-09 at 14:59 +0100, Linus Walleij wrote:
> On Tue, Mar 3, 2015 at 9:47 AM, Alexey Brodkin
> <Alexey.Brodkin@...opsys.com> wrote:
> > +++ b/drivers/gpio/gpio-dwapb.c
> > @@ -370,6 +370,9 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> > irq_create_mapping(gpio->domain, hwirq);
> >
> > port->bgc.gc.to_irq = dwapb_gpio_to_irq;
> > +
> > + /* Reset mask register */
> > + dwapb_write(gpio, GPIO_INTMASK, 0);
>
> I don't get this. This looks like you just enable all interrupts. The
> driver also contains this in .suspend():
DW APB GPIO has 2 separate registers related to interrupts:
[1] Mask interrupt register
[2] Enable interrupt register
So what I do in my patch I unmask all interrupts. But before at least
one interrupt is enabled output interrupt line will never get in active
state. And by default all interrupts are disabled (reset value = 0).
> /* Mask out interrupts */
> dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
>
> If *anything* the probe should *mask* all interrupts so that the
> .unmask() callback can enable them selectively.
I'm going to agree with this statement, but this requires a bit more
significant change in driver. I just wanted to fix an issue I discovered
on my setup.
Interestingly what I observed in my testing that if both
enable()/disable() and mask()/unmask() are implemented in driver then
only enable()/disable() pair will be actually used.
Look at how generic irq_enable() function is implemented -
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/chip.c#n208
--->8---
void irq_enable(struct irq_desc *desc)
{
irq_state_clr_disabled(desc);
if (desc->irq_data.chip->irq_enable)
desc->irq_data.chip->irq_enable(&desc->irq_data);
else
desc->irq_data.chip->irq_unmask(&desc->irq_data);
irq_state_clr_masked(desc);
}
--->8---
> The real problem I think is that struct irq_chip contains
> mask()/unmask() callbacks that are not implemented
> by this driver.
I'd say that mask()/unmask() callbacks are implemented in this driver
already.
See
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-dwapb.c#n334
--->8---
ct->chip.irq_mask = irq_gc_mask_set_bit;
ct->chip.irq_unmask = irq_gc_mask_clr_bit;
ct->chip.irq_set_type = dwapb_irq_set_type;
ct->chip.irq_enable = dwapb_irq_enable;
ct->chip.irq_disable = dwapb_irq_disable;
--->8---
It actually uses generic implementation of mask set bit and clear bit:
irq_gc_mask_set_bit()/irq_gc_mask_clr_bit() that operate under
GPIO_INTMASK register. And I may confirm that these functions correctly
set/reset bits in mask register of GPIO controller.
> Can you please test the below (untested) patch instead:
>
> From: Linus Walleij <linus.walleij@...aro.org>
> Date: Mon, 9 Mar 2015 14:56:18 +0100
> Subject: [PATCH] RFC: gpio: dwapb: handle mask/unmask properly
>
> This implements the callbacks for masking/unmasking IRQs in the
> special IRQ mask/unmask register of the DWAPB GPIO block.
> Previously these mask bits were unhandled and relied on
> boot-up defaults.
>
> Reported-by: Alexey Brodkin <Alexey.Brodkin@...opsys.com>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> drivers/gpio/gpio-dwapb.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 58faf04fce5d..1396f26bac5d 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -158,6 +158,30 @@ static void dwapb_irq_handler(u32 irq, struct
> irq_desc *desc)
> chip->irq_eoi(irq_desc_get_irq_data(desc));
> }
>
> +static void dwapb_irq_mask(struct irq_data *d)
> +{
> + struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
> + struct dwapb_gpio *gpio = igc->private;
> +
> + spin_lock_irqsave(&bgc->lock, flags);
> + val = dwapb_read(gpio, GPIO_INTMASK);
> + val |= BIT(d->hwirq);
> + dwapb_write(gpio, GPIO_INTMASK, val);
> + spin_unlock_irqrestore(&bgc->lock, flags);
> +}
> +
> +static void dwapb_irq_unmask(struct irq_data *d)
> +{
> + struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
> + struct dwapb_gpio *gpio = igc->private;
> +
> + spin_lock_irqsave(&bgc->lock, flags);
> + val = dwapb_read(gpio, GPIO_INTMASK);
> + val &= ~BIT(d->hwirq);
> + dwapb_write(gpio, GPIO_INTMASK, val);
> + spin_unlock_irqrestore(&bgc->lock, flags);
> +}
Why would we need these custom functions if there're already
irq_gc_mask_set_bit()/irq_gc_mask_clr_bit() implemented in
kernel/irq/generic-chip.c
> static void dwapb_irq_enable(struct irq_data *d)
> {
> struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
> @@ -302,6 +326,10 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> struct irq_chip_type *ct;
> int err, i;
>
> + /* Mask out and disable all interrupts */
> + dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
> + dwapb_write(gpio, GPIO_INTEN, 0);
This looks good to me - it's always a good idea to make sure defaults
are set as we expect.
> gpio->domain = irq_domain_add_linear(node, ngpio,
> &irq_generic_chip_ops, gpio);
> if (!gpio->domain)
> @@ -334,6 +362,8 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> ct->chip.irq_mask = irq_gc_mask_set_bit;
> ct->chip.irq_unmask = irq_gc_mask_clr_bit;
> ct->chip.irq_set_type = dwapb_irq_set_type;
> + ct->chip.irq_mask = dwapb_irq_mask;
> + ct->chip.irq_unmask = dwapb_irq_unmask;
Looks like we set "ct->chip.irq_mask" and "ct->chip.irq_unmask" twice,
don't we?
-Alexey
--
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/
Powered by blists - more mailing lists