Messages in this thread | | | Subject | Re: [PATCH] uio: disable lazy irq disable to avoid double fire | From | Thommy Jakobsson <> | Date | Sat, 23 May 2020 11:26:20 +0200 |
| |
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
| |