Re: [PATCH v2] drivers/uio/uio_pdrv_genirq.c: Fix memory freeing issues

["Hans J. Koch" <hjk@hansjkoch.de>]

On Thu, Dec 06, 2012 at 11:11:38AM +0200, Vitalii Demianets wrote:
> 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
> > hang. 
> > 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.

If you run into the irq handler, the interrupt was obviously enabled. Since
irq sharing is not supported by this driver, you ALWAYS have to disable it
because it IS your interrupt. Storing the enabled/disabled status in a
variable is not needed at all here. Or am I missing something?

> 
> > >  	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.

And that's right because the interrupt is initially enabled by
uio_register_device().

> 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 
> succeed).
> 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.

OK, but we humans tend to read a text from top to bottom, so a comment should
describe the code immediately following it. And that is a line that deals with
memory allocation, not with interrupts.

Thanks,
Hans