Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues
From: Vitalii Demianets
Date: Thu Dec 06 2012 - 04:11:33 EST
On Thursday 06 December 2012 04:41:01 Hans J. Koch wrote:
> > @@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq,
struct uio_info *dev_info)
> > * remember the state so we can allow user space to enable it later.
> > */
> > - if (!test_and_set_bit(0, &priv->flags))
> > + if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
> > disable_irq_nosync(irq);
> That is not safe. That flag can only be changed by userspace, and if the
> userspace part is not running, the CPU on which the irq is pending will
> The handler has to disable the interrupt in any case.
> (The original version had the same problem, I know...)
Perhaps I'm missing something here, but I don't see your point. If userspace
is not (implicitly) calling uio_pdrv_genirq_irqcontrol() by writing to the
device file, then on the first invocation of interrupt handler that IRQ is
disabled by disable_irq_nosync() and that's the end of story - no more irqs,
no problems. Until userspace writes non-zero to the device file, of course.
> > priv->uioinfo = uioinfo;
> > spin_lock_init(&priv->lock);
> > - priv->flags = 0; /* interrupt is enabled to begin with */
> > + /* interrupt is enabled to begin with */
> > + priv->flags = uioinfo_alloced ? (1 << UIO_INFO_ALLOCED) : 0;
> The comment doesn't describe the line it's supposed to comment.
The comment draws attention to the fact that UIO_IRQ_DISABLED is not set
initially, and this is done on purpose. Particularly it is done because of
the potential problem you noted earlier - if userspace is never setting this
flag, the interrupt handler will disable irq on the first invocation thanks
to the absence of the flag (if(!test_and_set_bit(UIO_IRQ_DISABLED,...) will
So, I think we really need some comment explaining how the things are arranged
here; after all even you haven't got the whole picture on the first glance.
Maybe the current wording of the comment is not the best. I've just left what
was there before.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/