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
| ||
|
Date: Thu, 29 Aug 2013 14:57:50 +0200 From: Linus Walleij <linus.walleij@...aro.org> To: George Cherian <george.cherian@...com>, Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>, Nikolay Balandin <n.a.balandin@...il.com>, Grant Likely <grant.likely@...aro.org> Cc: "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Linux-OMAP <linux-omap@...r.kernel.org> Subject: Re: [PATCH] gpio: pcf857x: cleanup irq_demux_work and use threaded irq On Tue, Aug 27, 2013 at 12:30 PM, George Cherian <george.cherian@...com> wrote: > This patch > - removes the irq_demux_work > - Uses devm_request_threaded_irq > - Call the user handler iff gpio_to_irq is done. > > Signed-off-by: George Cherian <george.cherian@...com> Can you please split this up? It seems like three different patches, and now they block each other. The threading patch is fine and I could apply it unless this was mixed up with other stuff. I'd like Kuninoro and/or Nikolay to have a look at this, so please CC them on subsequent iterations. NB: I really like that you move the irq handling to a thread, good job. > static const struct i2c_device_id pcf857x_id[] = { > @@ -89,12 +89,12 @@ struct pcf857x { > struct gpio_chip chip; > struct i2c_client *client; > struct mutex lock; /* protect 'out' */ > - struct work_struct work; /* irq demux work */ > struct irq_domain *irq_domain; /* for irq demux */ > spinlock_t slock; /* protect irq demux */ > unsigned out; /* software latch */ > unsigned status; /* current status */ > int irq; /* real irq number */ > + int irq_mapped; /* mapped gpio irqs */ This seems like an u32 or atleast unsigned, and state that its one bit flag per IRQ. How many GPIO lines are there? > -static void pcf857x_irq_demux_work(struct work_struct *work) > +static irqreturn_t pcf857x_irq(int irq, void *data) > { > - struct pcf857x *gpio = container_of(work, > - struct pcf857x, > - work); > + struct pcf857x *gpio = data; > unsigned long change, i, status, flags; > > status = gpio->read(gpio->client); > > spin_lock_irqsave(&gpio->slock, flags); > + > + /* > + * call the interrupt handler iff gpio is used as > + * interrupt source, just to avoid bad irqs > + */ > > - change = gpio->status ^ status; > + change = ((gpio->status ^ status) & gpio->irq_mapped); I don't know if that is right. If this happens you are getting what we call a "spurious IRQ" and this shall be reported to the IRQ core by returning IRQ_NONE and handled from there. > @@ -226,9 +223,13 @@ static irqreturn_t pcf857x_irq_demux(int irq, void *data) > static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq, > irq_hw_number_t hw) > { > + struct pcf857x *gpio = domain->host_data; > irq_set_chip_and_handler(virq, > &dummy_irq_chip, > handle_level_irq); > + set_irq_flags(virq, IRQF_VALID); > + gpio->irq_mapped |= (1 << hw); I'm a bit uneasy about this. It feels like its the irqdomain's responsibility to keep track of whether an IRQ is mapped or not. Maybe Grant should have a look at this. Yours, Linus Walleij -- 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