Messages in this thread | | | Date | Fri, 22 May 2020 11:14:44 +0200 | From | Greg KH <> | Subject | Re: [PATCH] uio: disable lazy irq disable to avoid double fire |
| |
On Thu, May 21, 2020 at 04:42:09PM +0200, Thommy Jakobsson wrote: > uio_pdrv_genirq and uio_dmem_genirq interrupts are handled in > userspace. So the condition for the interrupt hasn't normally not been > cleared when top half returns. disable_irq_nosync is called in top half, > but since that normally is lazy the irq isn't actually disabled. > > For level triggered interrupts this will always result in a spurious > additional fire since the level in to the interrupt controller still is > active. The actual interrupt handler isn't run though since this > spurious irq is just recorded, and later on discared (for level). > > This commit disables lazy masking for level triggered interrupts. It > leaves edge triggered interrupts as before, because they work with the > lazy scheme. > > All other UIO drivers already seem to clear the interrupt cause at > driver levels. > > Example of double fire. First goes all the way up to > uio_pdrv_genirq_handler, second is terminated in handle_fasteoi_irq and > marked as pending. > > <idle>-0 [000] d... 8.245870: gic_handle_irq: irq 29 > <idle>-0 [000] d.h. 8.245873: uio_pdrv_genirq_handler: disable irq 29 > <idle>-0 [000] d... 8.245878: gic_handle_irq: irq 29 > <idle>-0 [000] d.h. 8.245880: handle_fasteoi_irq: irq 29 PENDING > HInt-34 [001] d... 8.245897: uio_pdrv_genirq_irqcontrol: enable irq 29 > > Tested on 5.7rc2 using uio_pdrv_genirq and a custom Xilinx MPSoC board. > > Signed-off-by: Thommy Jakobsson <thommyj@gmail.com> > --- > drivers/uio/uio_dmem_genirq.c | 24 ++++++++++++++++++++++++ > drivers/uio/uio_pdrv_genirq.c | 24 ++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c > index f6ab3f28c838..14899ed19143 100644 > --- a/drivers/uio/uio_dmem_genirq.c > +++ b/drivers/uio/uio_dmem_genirq.c > @@ -20,6 +20,7 @@ > #include <linux/pm_runtime.h> > #include <linux/dma-mapping.h> > #include <linux/slab.h> > +#include <linux/irq.h> > > #include <linux/of.h> > #include <linux/of_platform.h> > @@ -200,6 +201,29 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev) > goto bad1; > uioinfo->irq = ret; > } > + > + 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?
> + 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?
> + } > + /* > + * 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.
> + irq_set_status_flags(uioinfo->irq, IRQ_DISABLE_UNLAZY); > + } > + } > uiomem = &uioinfo->mem[0]; > > for (i = 0; i < pdev->num_resources; ++i) { > diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c > index ae319ef3a832..abf8e21d7158 100644 > --- a/drivers/uio/uio_pdrv_genirq.c > +++ b/drivers/uio/uio_pdrv_genirq.c > @@ -20,6 +20,7 @@ > #include <linux/stringify.h> > #include <linux/pm_runtime.h> > #include <linux/slab.h> > +#include <linux/irq.h> > > #include <linux/of.h> > #include <linux/of_platform.h> > @@ -171,6 +172,29 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev) > } > } > > + if (uioinfo->irq) {
<snip>
All of the same comments I made above in the other file, also apply here.
thanks,
greg k-h
| |