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 <[email protected]>
---
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) {
+ 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;
+ }
+ /*
+ * 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");
+ 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) {
+ 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");
+ kfree(priv);
+ return -ENXIO;
+ }
+ /*
+ * 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");
+ irq_set_status_flags(uioinfo->irq, IRQ_DISABLE_UNLAZY);
+ }
+ }
+
uiomem = &uioinfo->mem[0];
for (i = 0; i < pdev->num_resources; ++i) {
--
2.17.1
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 <[email protected]>
> ---
> 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
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
Hi Thommy,
url: https://github.com/0day-ci/linux/commits/Thommy-Jakobsson/uio-disable-lazy-irq-disable-to-avoid-double-fire/20200521-225755
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git c9d7e3da1f3c4cf5dddfc5d7ce4d76d013aba1cc
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
drivers/uio/uio_pdrv_genirq.c:185 uio_pdrv_genirq_probe() warn: passing devm_ allocated variable to kfree. 'priv'
# https://github.com/0day-ci/linux/commit/c6460d7bd1fb8e3ff5aa0c252c37bbfcb9245367
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c6460d7bd1fb8e3ff5aa0c252c37bbfcb9245367
vim +/priv +185 drivers/uio/uio_pdrv_genirq.c
c767db0ab4bc85 Magnus Damm 2008-07-11 110 static int uio_pdrv_genirq_probe(struct platform_device *pdev)
c767db0ab4bc85 Magnus Damm 2008-07-11 111 {
08cb2e2148f763 Jingoo Han 2013-08-30 112 struct uio_info *uioinfo = dev_get_platdata(&pdev->dev);
b0297622a9726b Daniel Mack 2019-08-15 113 struct device_node *node = pdev->dev.of_node;
c767db0ab4bc85 Magnus Damm 2008-07-11 114 struct uio_pdrv_genirq_platdata *priv;
c767db0ab4bc85 Magnus Damm 2008-07-11 115 struct uio_mem *uiomem;
c767db0ab4bc85 Magnus Damm 2008-07-11 116 int ret = -EINVAL;
c767db0ab4bc85 Magnus Damm 2008-07-11 117 int i;
c767db0ab4bc85 Magnus Damm 2008-07-11 118
b0297622a9726b Daniel Mack 2019-08-15 119 if (node) {
b0297622a9726b Daniel Mack 2019-08-15 120 const char *name;
b0297622a9726b Daniel Mack 2019-08-15 121
27760f86866331 Hans J. Koch 2011-07-07 122 /* alloc uioinfo for one device */
e6789cd3dfb553 Michal Simek 2013-09-12 123 uioinfo = devm_kzalloc(&pdev->dev, sizeof(*uioinfo),
e6789cd3dfb553 Michal Simek 2013-09-12 124 GFP_KERNEL);
27760f86866331 Hans J. Koch 2011-07-07 125 if (!uioinfo) {
27760f86866331 Hans J. Koch 2011-07-07 126 dev_err(&pdev->dev, "unable to kmalloc\n");
e6789cd3dfb553 Michal Simek 2013-09-12 127 return -ENOMEM;
27760f86866331 Hans J. Koch 2011-07-07 128 }
b0297622a9726b Daniel Mack 2019-08-15 129
b0297622a9726b Daniel Mack 2019-08-15 130 if (!of_property_read_string(node, "linux,uio-name", &name))
b0297622a9726b Daniel Mack 2019-08-15 131 uioinfo->name = devm_kstrdup(&pdev->dev, name, GFP_KERNEL);
b0297622a9726b Daniel Mack 2019-08-15 132 else
b0297622a9726b Daniel Mack 2019-08-15 133 uioinfo->name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
b0297622a9726b Daniel Mack 2019-08-15 134 "%pOFn", node);
b0297622a9726b Daniel Mack 2019-08-15 135
27760f86866331 Hans J. Koch 2011-07-07 136 uioinfo->version = "devicetree";
27760f86866331 Hans J. Koch 2011-07-07 137 /* Multiple IRQs are not supported */
27760f86866331 Hans J. Koch 2011-07-07 138 }
27760f86866331 Hans J. Koch 2011-07-07 139
c767db0ab4bc85 Magnus Damm 2008-07-11 140 if (!uioinfo || !uioinfo->name || !uioinfo->version) {
c767db0ab4bc85 Magnus Damm 2008-07-11 141 dev_err(&pdev->dev, "missing platform_data\n");
e6789cd3dfb553 Michal Simek 2013-09-12 142 return ret;
c767db0ab4bc85 Magnus Damm 2008-07-11 143 }
c767db0ab4bc85 Magnus Damm 2008-07-11 144
e543ae896626a5 Mike Frysinger 2008-10-29 145 if (uioinfo->handler || uioinfo->irqcontrol ||
e543ae896626a5 Mike Frysinger 2008-10-29 146 uioinfo->irq_flags & IRQF_SHARED) {
c767db0ab4bc85 Magnus Damm 2008-07-11 147 dev_err(&pdev->dev, "interrupt configuration error\n");
e6789cd3dfb553 Michal Simek 2013-09-12 148 return ret;
c767db0ab4bc85 Magnus Damm 2008-07-11 149 }
c767db0ab4bc85 Magnus Damm 2008-07-11 150
e6789cd3dfb553 Michal Simek 2013-09-12 151 priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
^^^^^^^^^^^^^^^^^^^
c767db0ab4bc85 Magnus Damm 2008-07-11 152 if (!priv) {
c767db0ab4bc85 Magnus Damm 2008-07-11 153 dev_err(&pdev->dev, "unable to kmalloc\n");
e6789cd3dfb553 Michal Simek 2013-09-12 154 return -ENOMEM;
c767db0ab4bc85 Magnus Damm 2008-07-11 155 }
c767db0ab4bc85 Magnus Damm 2008-07-11 156
c767db0ab4bc85 Magnus Damm 2008-07-11 157 priv->uioinfo = uioinfo;
c767db0ab4bc85 Magnus Damm 2008-07-11 158 spin_lock_init(&priv->lock);
c767db0ab4bc85 Magnus Damm 2008-07-11 159 priv->flags = 0; /* interrupt is enabled to begin with */
af76756e6e8c26 Magnus Damm 2009-08-14 160 priv->pdev = pdev;
c767db0ab4bc85 Magnus Damm 2008-07-11 161
94ca629e40eb7e Benedikt Spranger 2012-05-14 162 if (!uioinfo->irq) {
94ca629e40eb7e Benedikt Spranger 2012-05-14 163 ret = platform_get_irq(pdev, 0);
e3a3c3a205554e Pavel Machek 2013-06-18 164 uioinfo->irq = ret;
e3a3c3a205554e Pavel Machek 2013-06-18 165 if (ret == -ENXIO && pdev->dev.of_node)
e3a3c3a205554e Pavel Machek 2013-06-18 166 uioinfo->irq = UIO_IRQ_NONE;
34bc4f468a9fab Oscar Ravadilla 2020-01-08 167 else if (ret == -EPROBE_DEFER)
34bc4f468a9fab Oscar Ravadilla 2020-01-08 168 return ret;
e3a3c3a205554e Pavel Machek 2013-06-18 169 else if (ret < 0) {
94ca629e40eb7e Benedikt Spranger 2012-05-14 170 dev_err(&pdev->dev, "failed to get IRQ\n");
e6789cd3dfb553 Michal Simek 2013-09-12 171 return ret;
94ca629e40eb7e Benedikt Spranger 2012-05-14 172 }
94ca629e40eb7e Benedikt Spranger 2012-05-14 173 }
e3a3c3a205554e Pavel Machek 2013-06-18 174
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 175 if (uioinfo->irq) {
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 176 struct irq_data *irq_data = irq_get_irq_data(uioinfo->irq);
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 177
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 178 /*
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 179 * If a level interrupt, dont do lazy disable. Otherwise the
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 180 * irq will fire again since clearing of the actual cause, on
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 181 * device level, is done in userspace
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 182 */
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 183 if (!irq_data) {
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 184 dev_err(&pdev->dev, "unable to get irq data\n");
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 @185 kfree(priv);
This should be deleted. It is a double free.
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 186 return -ENXIO;
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 187 }
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 188 /*
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 189 * irqd_is_level_type() isn't used since isn't valid unitil
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 190 * irq is configured.
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 191 */
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 192 if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_MASK) {
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 193 dev_info(&pdev->dev, "disable lazy unmask\n");
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 194 irq_set_status_flags(uioinfo->irq, IRQ_DISABLE_UNLAZY);
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 195 }
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 196 }
c6460d7bd1fb8e Thommy Jakobsson 2020-05-21 197
c767db0ab4bc85 Magnus Damm 2008-07-11 198 uiomem = &uioinfo->mem[0];
c767db0ab4bc85 Magnus Damm 2008-07-11 199
c767db0ab4bc85 Magnus Damm 2008-07-11 200 for (i = 0; i < pdev->num_resources; ++i) {
c767db0ab4bc85 Magnus Damm 2008-07-11 201 struct resource *r = &pdev->resource[i];
c767db0ab4bc85 Magnus Damm 2008-07-11 202
c767db0ab4bc85 Magnus Damm 2008-07-11 203 if (r->flags != IORESOURCE_MEM)
c767db0ab4bc85 Magnus Damm 2008-07-11 204 continue;
c767db0ab4bc85 Magnus Damm 2008-07-11 205
c767db0ab4bc85 Magnus Damm 2008-07-11 206 if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
c767db0ab4bc85 Magnus Damm 2008-07-11 207 dev_warn(&pdev->dev, "device has more than "
c767db0ab4bc85 Magnus Damm 2008-07-11 208 __stringify(MAX_UIO_MAPS)
c767db0ab4bc85 Magnus Damm 2008-07-11 209 " I/O memory resources.\n");
c767db0ab4bc85 Magnus Damm 2008-07-11 210 break;
c767db0ab4bc85 Magnus Damm 2008-07-11 211 }
c767db0ab4bc85 Magnus Damm 2008-07-11 212
c767db0ab4bc85 Magnus Damm 2008-07-11 213 uiomem->memtype = UIO_MEM_PHYS;
c767db0ab4bc85 Magnus Damm 2008-07-11 214 uiomem->addr = r->start;
28f65c11f2ffb3 Joe Perches 2011-06-09 215 uiomem->size = resource_size(r);
ecd43c0d7e504f Manuel Traut 2012-11-09 216 uiomem->name = r->name;
c767db0ab4bc85 Magnus Damm 2008-07-11 217 ++uiomem;
c767db0ab4bc85 Magnus Damm 2008-07-11 218 }
c767db0ab4bc85 Magnus Damm 2008-07-11 219
c767db0ab4bc85 Magnus Damm 2008-07-11 220 while (uiomem < &uioinfo->mem[MAX_UIO_MAPS]) {
c767db0ab4bc85 Magnus Damm 2008-07-11 221 uiomem->size = 0;
c767db0ab4bc85 Magnus Damm 2008-07-11 222 ++uiomem;
c767db0ab4bc85 Magnus Damm 2008-07-11 223 }
c767db0ab4bc85 Magnus Damm 2008-07-11 224
c767db0ab4bc85 Magnus Damm 2008-07-11 225 /* This driver requires no hardware specific kernel code to handle
c767db0ab4bc85 Magnus Damm 2008-07-11 226 * interrupts. Instead, the interrupt handler simply disables the
c767db0ab4bc85 Magnus Damm 2008-07-11 227 * interrupt in the interrupt controller. User space is responsible
c767db0ab4bc85 Magnus Damm 2008-07-11 228 * for performing hardware specific acknowledge and re-enabling of
c767db0ab4bc85 Magnus Damm 2008-07-11 229 * the interrupt in the interrupt controller.
c767db0ab4bc85 Magnus Damm 2008-07-11 230 *
c767db0ab4bc85 Magnus Damm 2008-07-11 231 * Interrupt sharing is not supported.
c767db0ab4bc85 Magnus Damm 2008-07-11 232 */
c767db0ab4bc85 Magnus Damm 2008-07-11 233
c767db0ab4bc85 Magnus Damm 2008-07-11 234 uioinfo->handler = uio_pdrv_genirq_handler;
c767db0ab4bc85 Magnus Damm 2008-07-11 235 uioinfo->irqcontrol = uio_pdrv_genirq_irqcontrol;
af76756e6e8c26 Magnus Damm 2009-08-14 236 uioinfo->open = uio_pdrv_genirq_open;
af76756e6e8c26 Magnus Damm 2009-08-14 237 uioinfo->release = uio_pdrv_genirq_release;
c767db0ab4bc85 Magnus Damm 2008-07-11 238 uioinfo->priv = priv;
c767db0ab4bc85 Magnus Damm 2008-07-11 239
af76756e6e8c26 Magnus Damm 2009-08-14 240 /* Enable Runtime PM for this device:
af76756e6e8c26 Magnus Damm 2009-08-14 241 * The device starts in suspended state to allow the hardware to be
af76756e6e8c26 Magnus Damm 2009-08-14 242 * turned off by default. The Runtime PM bus code should power on the
af76756e6e8c26 Magnus Damm 2009-08-14 243 * hardware and enable clocks at open().
af76756e6e8c26 Magnus Damm 2009-08-14 244 */
af76756e6e8c26 Magnus Damm 2009-08-14 245 pm_runtime_enable(&pdev->dev);
af76756e6e8c26 Magnus Damm 2009-08-14 246
eff1dd87fae244 Alexandru Ardelean 2020-03-06 247 ret = devm_add_action_or_reset(&pdev->dev, uio_pdrv_genirq_cleanup,
eff1dd87fae244 Alexandru Ardelean 2020-03-06 248 &pdev->dev);
eff1dd87fae244 Alexandru Ardelean 2020-03-06 249 if (ret)
e6789cd3dfb553 Michal Simek 2013-09-12 250 return ret;
c767db0ab4bc85 Magnus Damm 2008-07-11 251
eff1dd87fae244 Alexandru Ardelean 2020-03-06 252 ret = devm_uio_register_device(&pdev->dev, priv->uioinfo);
eff1dd87fae244 Alexandru Ardelean 2020-03-06 253 if (ret)
eff1dd87fae244 Alexandru Ardelean 2020-03-06 254 dev_err(&pdev->dev, "unable to register uio device\n");
47296b1962ead8 Jie Zhou 2011-04-06 255
eff1dd87fae244 Alexandru Ardelean 2020-03-06 256 return ret;
c767db0ab4bc85 Magnus Damm 2008-07-11 257 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
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 <[email protected]>
---
New in v2:
- use dev_dbg instead of dev_info
- make sure not to change current behaviour. If irq_data doesn't exists
for some reasone, just skip doing disabling unlazy altogether
- with above also the wrongly added kfree is removed (found by kbuild
test robot)
I did not add the Reported-by tag for the kbuild test robot finding.
Not sure about the correct procedure here, but since it found the error
in a patch not merged, my reasoning was that it would look wrong in the
git log later on. Please advice if this is customary anyway.
drivers/uio/uio_dmem_genirq.c | 19 +++++++++++++++++++
drivers/uio/uio_pdrv_genirq.c | 18 ++++++++++++++++++
2 files changed, 37 insertions(+)
diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
index 6e27fe4fcca3..ec7f66f4555a 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>
@@ -199,6 +200,24 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
goto bad1;
uioinfo->irq = ret;
}
+
+ if (uioinfo->irq) {
+ 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
+ * irqd_is_level_type() isn't used since isn't valid until
+ * irq is configured.
+ */
+ if (irq_data &&
+ irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_MASK) {
+ dev_dbg(&pdev->dev, "disable lazy unmask\n");
+ 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..6de0b115c2da 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,23 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
}
}
+ if (uioinfo->irq) {
+ 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
+ * irqd_is_level_type() isn't used since isn't valid until
+ * irq is configured.
+ */
+ if (irq_data &&
+ irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_MASK) {
+ dev_dbg(&pdev->dev, "disable lazy unmask\n");
+ irq_set_status_flags(uioinfo->irq, IRQ_DISABLE_UNLAZY);
+ }
+ }
+
uiomem = &uioinfo->mem[0];
for (i = 0; i < pdev->num_resources; ++i) {
--
2.17.1