Re: [PATCH 03/18] lirc driver for 1st-gen Media Center Ed. USB IR transceivers

On Tuesday 09 September 2008 21:21:40 Jonathan Corbet wrote:

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

I don't really care and I agree that the SCM is the preferred place for
the information. The only problem is that the info is in a different
SCM (lirc cvs repository).

I've removed the changelogs from all files as long as no names wre
mentioned.

+#include <linux/smp_lock.h>

It doesn't look like this include is needed.

removed

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

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

renamed to mceusb_device

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

renamed to lock

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

sigh, It would be a good idea, not sure if I'm motivated enough to add
that to a driver I can't test and I know almost nothing about.


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

That's probably a question for the original author, Dan. CC added. I'm
just cleaning them up for kernel inclusion.

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

there are more, I'll fix them all before the next patchset get posted.

...

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

yes, the declaration looks like this:
static DECLARE_MUTEX(...);

which gives us a single holder semaphore. the static in front of
DECLARE_MUTEX() fools checkpatch. changed to to DEFINE_MUTEX, also in
two other drivers.


...

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

fixed.

Thanks for the review.

Janne

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

Relevant Pages