lkml.org 
[lkml]   [2008]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 03/18] lirc driver for 1st-gen Media Center Ed. USB IR transceivers
    > +config LIRC_MCEUSB
    > + tristate "Microsoft Media Center Ed. Receiver, v1"
    > + default n
    > + depends on LIRC_DEV
    > + help
    > + Driver for the Microsoft Media Center Ed. Receiver, v1

    A little more verbosity in these help texts might be nice.

    > + * 2003_11_11 - Restructured to minimalize code interpretation in the
    > + * driver. The normal use case will be with lirc.
    > + *
    > + * 2004_01_01 - Removed all code interpretation. Generate mode2 data
    > + * for passing off to lirc. Cleanup
    > + *
    > + * 2004_01_04 - Removed devfs handle. Put in a temporary workaround
    > + * for a known issue where repeats generate two
    > + * sequential spaces (last_was_repeat_gap)
    > + *
    > + * 2004_02_17 - Changed top level api to no longer use fops, and
    > + * instead use new interface for polling via
    > + * lirc_thread. Restructure data read/mode2 generation to
    > + * a single pass, reducing number of buffers. Rev to .2
    > + *
    > + * 2004_02_27 - Last of fixups to plugin->add_to_buf API. Properly
    > + * handle broken fragments from the receiver. Up the
    > + * sample rate and remove any pacing from
    > + * fetch_more_data. Fixes all known issues.

    General convention is that this sort of changelog information belongs in
    the SCM, not in the code. That's doubly true for the USB skeleton driver
    info.

    > +#include <linux/smp_lock.h>

    It doesn't look like this include is needed.

    > +/* Structure to hold all of our device specific stuff */
    > +struct usb_skel {

    Perhaps renaming this structure to something more directly descriptive
    would make sense?

    > + struct usb_device *udev; /* save off the usb device pointer */
    > + struct usb_interface *interface; /* the interface for this device */
    > + unsigned char minor; /* the starting minor number for this device */

    Minor numbers don't necessarily fit in an unsigned char.

    > + unsigned char num_ports; /* the number of ports this device has */
    > + char num_interrupt_in; /* number of interrupt in endpoints */
    > + char num_bulk_in; /* number of bulk in endpoints */
    > + char num_bulk_out; /* number of bulk out endpoints */
    > +
    > + unsigned char *bulk_in_buffer; /* the buffer to receive data */
    > + int bulk_in_size; /* the size of the receive buffer */
    > + __u8 bulk_in_endpointAddr; /* the address of bulk in endpoint */
    > +
    > + unsigned char *bulk_out_buffer; /* the buffer to send data */
    > + int bulk_out_size; /* the size of the send buffer */
    > + struct urb *write_urb; /* the urb used to send data */
    > + __u8 bulk_out_endpointAddr; /* the address of bulk out endpoint */
    > +
    > + atomic_t write_busy; /* true iff write urb is busy */
    > + struct completion write_finished; /* wait for the write to finish */
    > +
    > + wait_queue_head_t wait_q; /* for timeouts */
    > + int open_count; /* number of times this port has been opened */
    > + struct mutex sem; /* locks this structure */

    "sem" is not a semaphore; it should probably have a different name.

    > +static void mceusb_setup(struct usb_device *udev)
    > +{
    > + char data[8];
    > + int res;
    > +
    > + memset(data, 0, 8);
    > +
    > + /* Get Status */
    > + res = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
    > + USB_REQ_GET_STATUS, USB_DIR_IN,
    > + 0, 0, data, 2, HZ * 3);

    res is set many times in this function, but it is never checked. It seems
    to me like the addition of some error handling would be a good idea.

    ...

    > + /* These two are sent by the windows driver, but stall for
    > + * me. I dont have an analyzer on the linux side so i can't
    > + * see what is actually different and why the device takes
    > + * issue with them
    > + */

    Hmm...how was that information obtained? If this driver was
    reverse-engineered, it would be good to know just what process was
    followed.

    > +static int msir_fetch_more_data(struct usb_skel *dev, int dont_block)
    > +{
    ...
    > + /* retry a few times on overruns; map all
    > + other errors to -EIO */
    > + if (retval) {
    > + if (retval == -EOVERFLOW && retries < 5) {
    > + retries++;
    > + interruptible_sleep_on_timeout(
    > + &dev->wait_q, HZ);
    > + continue;

    As others have noted, I think, getting new sleep_on() calls into the kernel
    is kind of a hard sell.

    > +/**
    > + * mceusb_probe
    > + *
    > + * Called by the usb core when a new device is connected that it
    > + * thinks this driver might be interested in.
    > + */
    > +static int mceusb_probe(struct usb_interface *interface,
    > + const struct usb_device_id *id)
    > +{

    ...
    > +
    > + /* select a "subminor" number (part of a minor number) */
    > + down(&minor_table_mutex);

    So...this driver has a mutex called "sem" and a semaphore called
    "minor_table_mutex". I suspect that minor_table_mutex should, in fact, be
    a mutex.

    ...
    > +error:
    > + mceusb_delete(dev);
    > + dev = NULL;
    > + dprintk("%s: retval = %x", __func__, retval);
    > + up(&minor_table_mutex);
    > + return retval;
    > +}

    This will leak the memory allocated for dev. It also leaves the entry in
    minor_table pointing to a nonfunctional device.

    jon


    \
     
     \ /
      Last update: 2008-09-09 21:25    [from the cache]
    ©2003-2011 Jasper Spaans