Messages in this thread | | | Date | Thu, 10 Jul 2008 12:58:51 +0200 | From | "Hans J. Koch" <> | Subject | Re: [PATCH] uio: uio_pdrv_genirq V2 |
| |
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
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |