lkml.org 
[lkml]   [2008]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 08/18] lirc driver for the Soundgraph IMON IR Receivers
    Date
    On Wednesday 10 September 2008 17:02:29 Jonathan Corbet wrote:
    > > +#define SUCCESS 0
    > > +#define TRUE 1
    > > +#define FALSE 0
    >
    > (See my grumble in previous reviews...:)
    >
    > > +#define LOCK_CONTEXT mutex_lock(&context->lock)
    > > +#define UNLOCK_CONTEXT mutex_unlock(&context->lock)
    >
    > Here too.

    All gone.

    > > +/* to prevent races between open() and disconnect() */
    > > +static DECLARE_MUTEX(disconnect_sem);
    >
    > This should be a real mutex, I think.

    Done.

    > > +/* lcd or vfd? */
    > > +static int is_lcd;
    >
    > The driver can only do one or the other? You can't have both in the
    > system? And somehow the user is supposed to configure it at load time to
    > do the right thing?

    Should be autodetected by default now, based on usb device ID, but can still
    be overridden by a module para 'display_type'. Not sure exactly what would
    happen if the user had more than one, but I don't know why they would in the
    first place...

    > > +static inline int send_packet(struct imon_context *context)
    [...]
    > > + /* fill request into kmalloc'ed space: */
    > > + control_req = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
    >
    > Why GFP_NOIO?

    Original author probably thought it was necessary. Certainly doesn't look like
    it is to me, I'll make that just GFP_KERNEL.

    > > + } else {
    > > + /* Wait for tranmission to complete(or abort) */
    > > + UNLOCK_CONTEXT;
    > > + wait_for_completion(&context->tx.finished);
    > > + LOCK_CONTEXT;
    >
    > Should that be an interruptible (or killable) wait?

    Don't see why it couldn't be. Done.

    > > +static ssize_t show_associate_remote(struct device *d,
    > > + struct device_attribute *attr,
    > > + char *buf)
    > > +{
    > > + struct imon_context *context = dev_get_drvdata(d);
    > > +
    > > + if (!context)
    > > + return -ENODEV;
    > > +
    > > + if (context->ir_isassociating) {
    > > + strcpy(buf, "The device it associating press some button "
    > > + "on the remote.\n");
    > > + } else if (context->ir_isopen) {
    > > + strcpy(buf, "Device is open and ready to associate.\n"
    > > + "Echo something into this file to start "
    > > + "the process.\n");
    > > + } else {
    > > + strcpy(buf, "Device is closed, you need to open it to "
    > > + "associate the remote(you can use irw).\n");
    > > + }
    > > + return strlen(buf);
    > > +}
    >
    > I *guess* that's one value per file, but it's still not quite the sysfs
    > norm. How about one-word status codes which can be made more verbose by
    > user space? (That's an API change, of course...)

    Yuk. There really is no user space. But chances of someone stumbling upon
    these directions in sysfs are pretty slim. Reducing to single-word, and
    logging a url pointer to the lirc.org page on setting these up. Sound sane
    enough?

    > > +static ssize_t store_associate_remote(struct device *d,
    > > + struct device_attribute *attr,
    > > + const char *buf, size_t count)
    > > +{
    > > + struct imon_context *context;
    > > +
    > > + context = dev_get_drvdata(d);
    > > +
    > > + if (!context)
    > > + return -ENODEV;
    > > +
    > > + if (!context->ir_isopen)
    > > + return -EINVAL;
    > > +
    > > + if (context->ir_isopen) {
    > > + context->ir_isassociating = TRUE;
    > > + send_associate_24g(context);
    > > + }
    > > +
    > > + return count;
    > > +}
    >
    > No need to take the mutex here?

    Added mutex here and in show_associate_remote() for good measure.

    > > +/**
    > > + * 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?

    > > +exit:
    > > + UNLOCK_CONTEXT;
    > > +
    > > + up(&disconnect_sem);
    > > + return SUCCESS;
    > > +}
    >
    > This discards "retval", which could hold an error status - the function
    > returns "SUCCESS" even if ->ir_isopen is not set.

    Gah. That's fugly. Fixed.

    > > +static void imon_disconnect(struct usb_interface *interface)
    [...]
    > > + /* Abort ongoing write */
    > > + if (atomic_read(&context->tx.busy)) {
    > > + usb_kill_urb(context->tx_urb);
    > > + wait_for_completion(&context->tx.finished);
    > > + }
    >
    > What if this races with another thread waiting for the completion? It
    > seems like it should be completed with complete_all(), but that wasn't the
    > case.

    Agreed. At this point, we're disconnecting, no point in waiting on anyone
    else.

    I've attempted to remedy everything, compile-tested and briefly run-time
    tested the changes, and have pushed 'em all into my git tree. My shiny new
    iMon Knob receiver and remote are reasonably happy still (less some lockdep
    spew that I have to figure out what to do with still), but I have no vfd or
    lcd to test with, unfortunately.

    --
    Jarod Wilson
    jarod@redhat.com



    \
     
     \ /
      Last update: 2008-09-22 23:51    [W:4.177 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site