This patch is V2 of uio_pdrv_genirq.c, a platform driver for UIO with
generic IRQ handling code. This driver is very similar to the regular
UIO platform driver, but is only suitable for devices that are
connected to the interrupt controller using unique interrupt lines.
The uio_pdrv_genirq driver includes generic interrupt handling code
which disables the serviced interrupt in the interrupt controller
and makes the user space driver responsible for acknowledging the
interrupt in the device and reenabling the interrupt in the interrupt
controller.
Shared interrupts are not supported since the in-kernel interrupt
handler will disable the interrupt line in the interrupt controller,
and in a shared interrupt configuration this will stop other devices
from delivering interrupts.
Signed-off-by: Magnus Damm <[email protected]>
---
Changes since V1
- allow user space to enable _and_ disable the interrupt
- replaced bitops with spinlock + irq_disabled variable
- renamed pdata variable name to priv
drivers/uio/Kconfig | 13 ++
drivers/uio/Makefile | 1
drivers/uio/uio_pdrv_genirq.c | 186 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 200 insertions(+)
--- 0014/drivers/uio/Kconfig
+++ work/drivers/uio/Kconfig 2008-07-09 17:10:20.000000000 +0900
@@ -33,6 +33,19 @@ config UIO_PDRV
If you don't know what to do here, say N.
+config UIO_PDRV_GENIRQ
+ tristate "Userspace I/O platform driver with generic IRQ handling"
+ help
+ Platform driver for Userspace I/O devices, including generic
+ interrupt handling code. Shared interrupts are not supported.
+
+ This kernel driver requires that the matching userspace driver
+ handles interrupts in a special way. Userspace is responsible
+ for acknowledging the hardware device if needed, and re-enabling
+ interrupts in the interrupt controller using the write() syscall.
+
+ If you don't know what to do here, say N.
+
config UIO_SMX
tristate "SMX cryptengine UIO interface"
depends on UIO
--- 0014/drivers/uio/Makefile
+++ work/drivers/uio/Makefile 2008-07-09 17:10:20.000000000 +0900
@@ -1,4 +1,5 @@
obj-$(CONFIG_UIO) += uio.o
obj-$(CONFIG_UIO_CIF) += uio_cif.o
obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
+obj-$(CONFIG_UIO_PDRV_GENIRQ) += uio_pdrv_genirq.o
obj-$(CONFIG_UIO_SMX) += uio_smx.o
--- /dev/null
+++ work/drivers/uio/uio_pdrv_genirq.c 2008-07-10 12:24:52.000000000 +0900
@@ -0,0 +1,186 @@
+/*
+ * drivers/uio/uio_pdrv_genirq.c
+ *
+ * Userspace I/O platform driver with generic IRQ handling code.
+ *
+ * Copyright (C) 2008 Magnus Damm
+ *
+ * Based on uio_pdrv.c by Uwe Kleine-Koenig,
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/stringify.h>
+
+#define DRIVER_NAME "uio_pdrv_genirq"
+
+struct uio_pdrv_genirq_platdata {
+ struct uio_info *uioinfo;
+ spinlock_t lock;
+ int irq_disabled;
+};
+
+static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
+{
+ struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
+ unsigned long flags;
+
+ /* Just disable the interrupt in the interrupt controller, and
+ * remember the state so we can allow user space to enable it later.
+ */
+ spin_lock_irqsave(&priv->lock, flags);
+ if (!priv->irq_disabled) {
+ disable_irq_nosync(irq);
+ priv->irq_disabled = 1;
+ }
+ spin_unlock_irqrestore(&priv->lock, flags);
+ return IRQ_HANDLED;
+}
+
+static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
+{
+ struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
+ unsigned long flags;
+
+ /* Allow user space to enable and disable the interrupt
+ * in the interrupt controller, but keep track of the
+ * state to prevent per-irq depth damage.
+ */
+
+ spin_lock_irqsave(&priv->lock, flags);
+ if (irq_on && priv->irq_disabled)
+ enable_irq(dev_info->irq);
+ else if (!irq_on && !priv->irq_disabled)
+ disable_irq(dev_info->irq);
+
+ priv->irq_disabled = !irq_on;
+ spin_unlock_irqrestore(&priv->lock, flags);
+ return 0;
+}
+
+static int uio_pdrv_genirq_probe(struct platform_device *pdev)
+{
+ struct uio_info *uioinfo = pdev->dev.platform_data;
+ struct uio_pdrv_genirq_platdata *priv;
+ struct uio_mem *uiomem;
+ int ret = -EINVAL;
+ int i;
+
+ if (!uioinfo || !uioinfo->name || !uioinfo->version) {
+ dev_err(&pdev->dev, "missing platform_data\n");
+ goto bad0;
+ }
+
+ if (uioinfo->handler || uioinfo->irqcontrol || uioinfo->irq_flags) {
+ dev_err(&pdev->dev, "interrupt configuration error\n");
+ goto bad0;
+ }
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ ret = -ENOMEM;
+ dev_err(&pdev->dev, "unable to kmalloc\n");
+ goto bad0;
+ }
+
+ priv->uioinfo = uioinfo;
+ spin_lock_init(&priv->lock);
+ priv->irq_disabled = 0; /* interrupt is enabled to begin with */
+
+ uiomem = &uioinfo->mem[0];
+
+ for (i = 0; i < pdev->num_resources; ++i) {
+ struct resource *r = &pdev->resource[i];
+
+ if (r->flags != IORESOURCE_MEM)
+ continue;
+
+ if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
+ dev_warn(&pdev->dev, "device has more than "
+ __stringify(MAX_UIO_MAPS)
+ " I/O memory resources.\n");
+ break;
+ }
+
+ uiomem->memtype = UIO_MEM_PHYS;
+ uiomem->addr = r->start;
+ uiomem->size = r->end - r->start + 1;
+ ++uiomem;
+ }
+
+ while (uiomem < &uioinfo->mem[MAX_UIO_MAPS]) {
+ uiomem->size = 0;
+ ++uiomem;
+ }
+
+ /* This driver requires no hardware specific kernel code to handle
+ * interrupts. Instead, the interrupt handler simply disables the
+ * interrupt in the interrupt controller. User space is responsible
+ * for performing hardware specific acknowledge and re-enabling of
+ * the interrupt in the interrupt controller.
+ *
+ * Interrupt sharing is not supported.
+ */
+
+ uioinfo->irq_flags = IRQF_DISABLED;
+ uioinfo->handler = uio_pdrv_genirq_handler;
+ uioinfo->irqcontrol = uio_pdrv_genirq_irqcontrol;
+ uioinfo->priv = priv;
+
+ ret = uio_register_device(&pdev->dev, priv->uioinfo);
+ if (ret) {
+ dev_err(&pdev->dev, "unable to register uio device\n");
+ goto bad1;
+ }
+
+ platform_set_drvdata(pdev, priv);
+ return 0;
+ bad1:
+ kfree(priv);
+ bad0:
+ return ret;
+}
+
+static int uio_pdrv_genirq_remove(struct platform_device *pdev)
+{
+ struct uio_pdrv_genirq_platdata *priv = platform_get_drvdata(pdev);
+
+ uio_unregister_device(priv->uioinfo);
+ kfree(priv);
+ return 0;
+}
+
+static struct platform_driver uio_pdrv_genirq = {
+ .probe = uio_pdrv_genirq_probe,
+ .remove = uio_pdrv_genirq_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init uio_pdrv_genirq_init(void)
+{
+ return platform_driver_register(&uio_pdrv_genirq);
+}
+
+static void __exit uio_pdrv_genirq_exit(void)
+{
+ platform_driver_unregister(&uio_pdrv_genirq);
+}
+
+module_init(uio_pdrv_genirq_init);
+module_exit(uio_pdrv_genirq_exit);
+
+MODULE_AUTHOR("Magnus Damm");
+MODULE_DESCRIPTION("Userspace I/O platform driver with generic IRQ handling");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRIVER_NAME);
Hi Magnus,
Magnus Damm wrote:
> This patch is V2 of uio_pdrv_genirq.c, a platform driver for UIO with
> generic IRQ handling code. This driver is very similar to the regular
> UIO platform driver, but is only suitable for devices that are
> connected to the interrupt controller using unique interrupt lines.
>
> The uio_pdrv_genirq driver includes generic interrupt handling code
> which disables the serviced interrupt in the interrupt controller
> and makes the user space driver responsible for acknowledging the
> interrupt in the device and reenabling the interrupt in the interrupt
> controller.
>
> Shared interrupts are not supported since the in-kernel interrupt
> handler will disable the interrupt line in the interrupt controller,
> and in a shared interrupt configuration this will stop other devices
> from delivering interrupts.
>
> Signed-off-by: Magnus Damm <[email protected]>
> ---
>
> Changes since V1
> - allow user space to enable _and_ disable the interrupt
> - replaced bitops with spinlock + irq_disabled variable
> - renamed pdata variable name to priv
>
> drivers/uio/Kconfig | 13 ++
> drivers/uio/Makefile | 1
> drivers/uio/uio_pdrv_genirq.c | 186 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 200 insertions(+)
>
> --- 0014/drivers/uio/Kconfig
> +++ work/drivers/uio/Kconfig 2008-07-09 17:10:20.000000000 +0900
> @@ -33,6 +33,19 @@ config UIO_PDRV
>
> If you don't know what to do here, say N.
>
> +config UIO_PDRV_GENIRQ
> + tristate "Userspace I/O platform driver with generic IRQ handling"
> + help
> + Platform driver for Userspace I/O devices, including generic
> + interrupt handling code. Shared interrupts are not supported.
> +
> + This kernel driver requires that the matching userspace driver
> + handles interrupts in a special way. Userspace is responsible
> + for acknowledging the hardware device if needed, and re-enabling
> + interrupts in the interrupt controller using the write() syscall.
> +
> + If you don't know what to do here, say N.
> +
> config UIO_SMX
> tristate "SMX cryptengine UIO interface"
> depends on UIO
> --- 0014/drivers/uio/Makefile
> +++ work/drivers/uio/Makefile 2008-07-09 17:10:20.000000000 +0900
> @@ -1,4 +1,5 @@
> obj-$(CONFIG_UIO) += uio.o
> obj-$(CONFIG_UIO_CIF) += uio_cif.o
> obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
> +obj-$(CONFIG_UIO_PDRV_GENIRQ) += uio_pdrv_genirq.o
> obj-$(CONFIG_UIO_SMX) += uio_smx.o
> --- /dev/null
> +++ work/drivers/uio/uio_pdrv_genirq.c 2008-07-10 12:24:52.000000000 +0900
> @@ -0,0 +1,186 @@
> +/*
> + * drivers/uio/uio_pdrv_genirq.c
> + *
> + * Userspace I/O platform driver with generic IRQ handling code.
> + *
> + * Copyright (C) 2008 Magnus Damm
> + *
> + * Based on uio_pdrv.c by Uwe Kleine-Koenig,
> + * Copyright (C) 2008 by Digi International Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/uio_driver.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/stringify.h>
> +
> +#define DRIVER_NAME "uio_pdrv_genirq"
> +
> +struct uio_pdrv_genirq_platdata {
> + struct uio_info *uioinfo;
> + spinlock_t lock;
> + int irq_disabled;
> +};
> +
> +static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
> +{
> + struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
> + unsigned long flags;
> +
> + /* Just disable the interrupt in the interrupt controller, and
> + * remember the state so we can allow user space to enable it later.
> + */
> + spin_lock_irqsave(&priv->lock, flags);
> + if (!priv->irq_disabled) {
> + disable_irq_nosync(irq);
> + priv->irq_disabled = 1;
> + }
> + spin_unlock_irqrestore(&priv->lock, flags);
> + return IRQ_HANDLED;
> +}
> +
> +static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
> +{
> + struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
> + unsigned long flags;
> +
> + /* Allow user space to enable and disable the interrupt
> + * in the interrupt controller, but keep track of the
> + * state to prevent per-irq depth damage.
> + */
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + if (irq_on && priv->irq_disabled)
> + enable_irq(dev_info->irq);
> + else if (!irq_on && !priv->irq_disabled)
> + disable_irq(dev_info->irq);
I'm not sure if this is a problem on SMP. Should you use
disable_irq_nosync here, too? Probably it's OK.
> +
> + priv->irq_disabled = !irq_on;
> + spin_unlock_irqrestore(&priv->lock, flags);
> + return 0;
> +}
> + ret = uio_register_device(&pdev->dev, priv->uioinfo);
> + if (ret) {
> + dev_err(&pdev->dev, "unable to register uio device\n");
> + goto bad1;
> + }
> +
> + platform_set_drvdata(pdev, priv);
This should probably go before uio_register_device. (Uups, this is an
issue for uio_pdrv, too.)
Best regards
Uwe
--
Uwe Kleine-K?nig, Software Engineer
Digi International GmbH Branch Breisach, K?ferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
> > + else if (!irq_on && !priv->irq_disabled)
> > + disable_irq(dev_info->irq);
> I'm not sure if this is a problem on SMP. Should you use
> disable_irq_nosync here, too? Probably it's OK.
That one will also deadlock.
The easiest fix is probably to use test_and_set and friends for each I/O
operation. You would then not need the lock to protect ->irq_disabled.
Propogating that throughout means your user space has to handle the case
of an IRQ arriving after disable returns but would be a fair bit saner I
think ?
Alan Cox wrote:
>
> > > + else if (!irq_on && !priv->irq_disabled)
> > > + disable_irq(dev_info->irq);
> > I'm not sure if this is a problem on SMP. Should you use
> > disable_irq_nosync here, too? Probably it's OK.
>
> That one will also deadlock.
Can you explain why? I think irqcontrol is only called in task context.
I only see one possible deadlock and that's disable_irq being called
while the irq is IRQ_INPROGRESS on the same cpu. I'm always willing to
learn.
> The easiest fix is probably to use test_and_set and friends for each I/O
> operation.
Actually using spinlock + irq_disabled variable is new in V2 of this
patch. Don't know why this changed, though.
> You would then not need the lock to protect ->irq_disabled.
> Propogating that throughout means your user space has to handle the case
> of an IRQ arriving after disable returns but would be a fair bit saner I
> think ?
I think I didn't understand you right here, with the lock this can
happen, too, doesn't it?
Best regards
Uwe
--
Uwe Kleine-K?nig, Software Engineer
Digi International GmbH Branch Breisach, K?ferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
On Thu, 10 Jul 2008 12:30:36 +0200
Uwe Kleine-König <[email protected]> wrote:
> Alan Cox wrote:
> >
> > > > + else if (!irq_on && !priv->irq_disabled)
> > > > + disable_irq(dev_info->irq);
> > > I'm not sure if this is a problem on SMP. Should you use
> > > disable_irq_nosync here, too? Probably it's OK.
> >
> > That one will also deadlock.
> Can you explain why? I think irqcontrol is only called in task context.
> I only see one possible deadlock and that's disable_irq being called
> while the irq is IRQ_INPROGRESS on the same cpu. I'm always willing to
> learn.
CPU0 (UIO IRQ) CPU1 (irqcontrol)
take IRQ
take spin lock
spin on spinlock
disable_irq (blocks)
> I think I didn't understand you right here, with the lock this can
> happen, too, doesn't it?
Actually yes - so it would simplify it without changing behaviour.
Alan
Hello Alan,
Alan Cox wrote:
> On Thu, 10 Jul 2008 12:30:36 +0200
> Uwe Kleine-K?nig <[email protected]> wrote:
>
> > Alan Cox wrote:
> > >
> > > > > + else if (!irq_on && !priv->irq_disabled)
> > > > > + disable_irq(dev_info->irq);
> > > > I'm not sure if this is a problem on SMP. Should you use
> > > > disable_irq_nosync here, too? Probably it's OK.
> > >
> > > That one will also deadlock.
> > Can you explain why? I think irqcontrol is only called in task context.
> > I only see one possible deadlock and that's disable_irq being called
> > while the irq is IRQ_INPROGRESS on the same cpu. I'm always willing to
> > learn.
>
> CPU0 (UIO IRQ) CPU1 (irqcontrol)
> take IRQ
> take spin lock
> spin on spinlock
> disable_irq (blocks)
Ah, OK, that's because uio_pdrv_genirq_handler and
uio_pdrv_genirq_irqcontrol share the lock.
Is this something that lockdep can detect?
Best regards and thanks for clearifying
Uwe
--
Uwe Kleine-K?nig, Software Engineer
Digi International GmbH Branch Breisach, K?ferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
On Thu, Jul 10, 2008 at 08:56:39AM +0200, Uwe Kleine-König wrote:
> > +
> > +static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
> > +{
> > + struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
> > + unsigned long flags;
> > +
> > + /* Just disable the interrupt in the interrupt controller, and
> > + * remember the state so we can allow user space to enable it later.
> > + */
> > + spin_lock_irqsave(&priv->lock, flags);
> > + if (!priv->irq_disabled) {
> > + disable_irq_nosync(irq);
> > + priv->irq_disabled = 1;
> > + }
> > + spin_unlock_irqrestore(&priv->lock, flags);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
> > +{
> > + struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
> > + unsigned long flags;
> > +
> > + /* Allow user space to enable and disable the interrupt
> > + * in the interrupt controller, but keep track of the
> > + * state to prevent per-irq depth damage.
> > + */
> > +
> > + spin_lock_irqsave(&priv->lock, flags);
> > + if (irq_on && priv->irq_disabled)
> > + enable_irq(dev_info->irq);
> > + else if (!irq_on && !priv->irq_disabled)
> > + disable_irq(dev_info->irq);
> I'm not sure if this is a problem on SMP. Should you use
> disable_irq_nosync here, too? Probably it's OK.
*_nosync should not be needed, as he doesn't call irqcontrol from the irq
handler. But using the same lock in handler and irqcontrol presents a
problem, as Alan pointed out.
>
> > +
> > + priv->irq_disabled = !irq_on;
> > + spin_unlock_irqrestore(&priv->lock, flags);
> > + return 0;
> > +}
>
>
> > + ret = uio_register_device(&pdev->dev, priv->uioinfo);
> > + if (ret) {
> > + dev_err(&pdev->dev, "unable to register uio device\n");
> > + goto bad1;
> > + }
> > +
> > + platform_set_drvdata(pdev, priv);
> This should probably go before uio_register_device. (Uups, this is an
> issue for uio_pdrv, too.)
Yes, because uio_register_device will enable the irq, and you might
arrive in the handler without having your platform data in place.
Thanks,
Hans
Hi,
Hans J. Koch wrote:
> > > + ret = uio_register_device(&pdev->dev, priv->uioinfo);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "unable to register uio device\n");
> > > + goto bad1;
> > > + }
> > > +
> > > + platform_set_drvdata(pdev, priv);
> > This should probably go before uio_register_device. (Uups, this is an
> > issue for uio_pdrv, too.)
>
> Yes, because uio_register_device will enable the irq, and you might
> arrive in the handler without having your platform data in place.
How do we want to fix that? Should I sent a new version that also
includes the memory-leak fix and copyright specification? Or another
incremental patch?
Best regards
Uwe
--
Uwe Kleine-K?nig, Software Engineer
Digi International GmbH Branch Breisach, K?ferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
On Thu, Jul 10, 2008 at 01:06:31PM +0200, Uwe Kleine-König wrote:
> Hi,
>
> Hans J. Koch wrote:
> > > > + ret = uio_register_device(&pdev->dev, priv->uioinfo);
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "unable to register uio device\n");
> > > > + goto bad1;
> > > > + }
> > > > +
> > > > + platform_set_drvdata(pdev, priv);
> > > This should probably go before uio_register_device. (Uups, this is an
> > > issue for uio_pdrv, too.)
> >
> > Yes, because uio_register_device will enable the irq, and you might
> > arrive in the handler without having your platform data in place.
> How do we want to fix that? Should I sent a new version that also
> includes the memory-leak fix and copyright specification? Or another
> incremental patch?
Send another incremental patch for uio_pdrv, just for the records.
I'll sort it out with Greg later, if he finds many small patches
annoying, I can easily combine them.
Thanks,
Hans
On Thu, Jul 10, 2008 at 7:30 PM, Uwe Kleine-K?nig
<[email protected]> wrote:
> Alan Cox wrote:
>>
>> > > + else if (!irq_on && !priv->irq_disabled)
>> > > + disable_irq(dev_info->irq);
>> > I'm not sure if this is a problem on SMP. Should you use
>> > disable_irq_nosync here, too? Probably it's OK.
>>
>> That one will also deadlock.
> Can you explain why? I think irqcontrol is only called in task context.
> I only see one possible deadlock and that's disable_irq being called
> while the irq is IRQ_INPROGRESS on the same cpu. I'm always willing to
> learn.
>
>> The easiest fix is probably to use test_and_set and friends for each I/O
>> operation.
> Actually using spinlock + irq_disabled variable is new in V2 of this
> patch. Don't know why this changed, though.
Sorry for not being more clear about it. Basically, I wanted to
serialize user space access somehow, but I managed to screw it up. =)
Atomic enable-and-disable operations without serialization has this problem:
irq line state task0 task1
enabled enabled write "0"
if (!test_and_set_bit())
enabled disabled
write "1"
if (test_and_clear_bit())
enable_irq() <- ERR
disable_irq()
ERR will make the interrupt depth counter underflow.
The best solution in my mind is atomic operations,
disable_irq_nosync() in irq handler only, and serialize user space
only - not the irq handler.
Or maybe the UIO core layer should serialization in the case of multiple tasks?
/ magnus
On Thu, Jul 10, 2008 at 7:58 PM, Hans J. Koch <[email protected]> wrote:
> On Thu, Jul 10, 2008 at 08:56:39AM +0200, Uwe Kleine-K?nig wrote:
>> > + ret = uio_register_device(&pdev->dev, priv->uioinfo);
>> > + if (ret) {
>> > + dev_err(&pdev->dev, "unable to register uio device\n");
>> > + goto bad1;
>> > + }
>> > +
>> > + platform_set_drvdata(pdev, priv);
>> This should probably go before uio_register_device. (Uups, this is an
>> issue for uio_pdrv, too.)
>
> Yes, because uio_register_device will enable the irq, and you might
> arrive in the handler without having your platform data in place.
I'd say that this is a non-issue.
Only uio_pdrv_genirq_remove() is using the platform data. This
remove() method won't be called if uio_pdrv_genirq_probe() returns
failure. The UIO subsystem is using uioinfo->priv which of course is
set up correctly already.
Thanks,
/ magnus
Hello,
Magnus Damm wrote:
> On Thu, Jul 10, 2008 at 7:58 PM, Hans J. Koch <[email protected]> wrote:
> > On Thu, Jul 10, 2008 at 08:56:39AM +0200, Uwe Kleine-K?nig wrote:
> >> > + ret = uio_register_device(&pdev->dev, priv->uioinfo);
> >> > + if (ret) {
> >> > + dev_err(&pdev->dev, "unable to register uio device\n");
> >> > + goto bad1;
> >> > + }
> >> > +
> >> > + platform_set_drvdata(pdev, priv);
> >> This should probably go before uio_register_device. (Uups, this is an
> >> issue for uio_pdrv, too.)
> >
> > Yes, because uio_register_device will enable the irq, and you might
> > arrive in the handler without having your platform data in place.
>
> I'd say that this is a non-issue.
>
> Only uio_pdrv_genirq_remove() is using the platform data. This
> remove() method won't be called if uio_pdrv_genirq_probe() returns
> failure. The UIO subsystem is using uioinfo->priv which of course is
> set up correctly already.
... let me check ... yes, it's true. So it only looks ugly :-)
Uwe
--
Uwe Kleine-K?nig, Software Engineer
Digi International GmbH Branch Breisach, K?ferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962