Re: [PATCH] uio: disable lazy irq disable to avoid double fire

From: Thommy Jakobsson
Date: Sat May 23 2020 - 05:26:27 EST


On 2020-05-22 11:14, Greg KH wrote:
> On Thu, May 21, 2020 at 04:42:09PM +0200, Thommy Jakobsson wrote:
>> + if (uioinfo->irq) {
>
> How is this not true at this point in time based on the code above this?
> ->irq should always be set here, right?
It seems to me like there is a path to continue without an IRQ in the
section above, "uioinfo->irq = UIO_IRQ_NONE;", where UIO_IRQ_NONE is
0. Do you agree?

>
>> + struct irq_data *irq_data = irq_get_irq_data(uioinfo->irq);
>> +
>> + /*
>> + * If a level interrupt, dont do lazy disable. Otherwise the
>> + * irq will fire again since clearing of the actual cause, on
>> + * device level, is done in userspace
>> + */
>> + if (!irq_data) {
>> + dev_err(&pdev->dev, "unable to get irq data\n");
>> + ret = -ENXIO;
>> + goto bad1;
>
> Why is not having this information all of a sudden an error for this
> code? What could cause that info to not be there? Existing systems
> without this set would work just fine before this change, and I think
> this breaks them, right?
irq_data should always exists as long as there is an irq descriptor and
I assumed that the descriptors "should" exists at the point when this
code run. So a NULL would either mean something serious and would be
more of a sanity check than anything else, or (more likely) it is the
wrong irq configured.

The "should" comes from that this code path later registers the irq
(devm_uio_register_device->... ->__uio_register_device->...
->request_irq->... ->request_threaded_irq), and when this happen its an
-EINVAL back if there isn't any descriptor from irq_to_desc (which is
the same function as irq_get_data internally uses). So I don't see any
new breaks from this, but I could be wrong so please correct me in that
case. At least it was my intention to not break anything currently
working. Maybe it is better to just do a dev_dbg and let
request_threaded_irq handle the wrong irq case?

>
>> + }
>> + /*
>> + * irqd_is_level_type() isn't used since isn't valid unitil
>> + * irq is configured.
>> + */
>> + if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_MASK) {
>> + dev_info(&pdev->dev, "disable lazy unmask\n");
>
> Why dev_info? If drivers are working properly, they should be quiet.
> dev_dbg() is fine to use here if you want to debug things to see what is
> happening.
Agreed

BR,
Thommy Jakobsson