Re: [PATCH 08/18] lirc driver for the Soundgraph IMON IR Receivers

[Jarod Wilson]

On Monday 22 September 2008 17:47:56 Jarod Wilson wrote:
> > > +/**
> > > + * Called by lirc_dev when the application opens /dev/lirc
> > > + */
> > > +static int ir_open(void *data)
> > > +{
> > > +   int retval = SUCCESS;
> > > +   struct imon_context *context;
> > > +
> > > +   /* prevent races with disconnect */
> > > +   down(&disconnect_sem);
> > > +
> > > +   context = (struct imon_context *) data;
> > > +
> > > +   LOCK_CONTEXT;
> > > +
> > > +   if (context->ir_isopen) {
> > > +           err("%s: IR port is already open", __func__);
> > > +           retval = -EBUSY;
> > > +           goto exit;
> > > +   }
> >
> > I wonder if the single-open semantics are really doing what the author
> > intended?  It is unsufficient to prevent concurrent calls elsewhere.
>
> I think we're good to go with disconnect_sem being converted to an actual
> mutex, no?

No. The lock here is worthless, as is the check for context->ir_isopen, as 
this function *only* gets called with the driver_lock held in lirc_dev, and 
lirc_dev won't call this if the device is already open. I've simply dropped 
the context lock, as its not needed at all (and in fact, triggered lockdep 
spew).

-- 
Jarod Wilson
jarod@xxxxxxxxxx

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