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

From: Greg KH
Date: Fri May 22 2020 - 05:14:49 EST


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@xxxxxxxxx>
> ---
> 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