linux-kernel - Re: [PATCH] gpio: pcf857x: cleanup irq_demux

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: <521F5684.3090703@ti.com>
Date:	Thu, 29 Aug 2013 19:41:16 +0530
From:	George Cherian <george.cherian@...com>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
	Nikolay Balandin <n.a.balandin@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	"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

Hi Linus,

Thanks for the review. I will split it and send v2.

On 8/29/2013 6:27 PM, Linus Walleij wrote:
> 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.
okay
> 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?
pcf857x driver supports expanders with 8 and 16 gpio lines.
>> -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.
While testing I got prints like bad irq and no handler installed.
Now I notice its mostly when the n_latch is not passed properly.
>
>> @@ -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.
Mainly these expanders dont have an ier sort of registers and if at all 
the initial value is not set proper
then it gives bad irq prints only once per changed bit for which there 
is no handler.
Should I drop this in v2?

>
> Maybe Grant should have a look at this.
>
> Yours,
> Linus Walleij


-- 
-George

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