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    [W:0.402 / U:36.980 seconds]
©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site