On Tue, 19 Jan 2010, Chris Frey wrote:
> @@ -344,6 +346,13 @@ static void snoop_urb(struct usb_device *udev,
> "status %d\n",
> ep, t, d, length, timeout_or_status);
> }
> +
> + if (data) {
> + dev_info(&udev->dev, "data: ");
> + for (j = 0; j < data_len; ++j)
> + printk("%02x ", data[j]);
> + printk("\n");
> + }
For new code you should use print_hex_dump() instead of rolling it
yourself. And you should test data_len > 0 as well as data != NULL.
> @@ -410,7 +419,8 @@ static void async_completed(struct urb *urb)
> }
> snoop(&urb->dev->dev, "urb complete\n");
> snoop_urb(urb->dev, as->userurb, urb->pipe, urb->actual_length,
> - as->status, COMPLETE);
> + as->status, COMPLETE,
> + urb->transfer_buffer, urb->actual_length);
You shouldn't print the data upon URB completion if
(urb->transfer_flags & URB_DIR_MASK) == USB_DIR_OUT.
> @@ -777,14 +787,15 @@ static int proc_control(struct dev_state *ps, void __user *arg)
> return -EINVAL;
> }
> pipe = usb_rcvctrlpipe(dev, 0);
> - snoop_urb(dev, NULL, pipe, ctrl.wLength, tmo, SUBMIT);
> + snoop_urb(dev, NULL, pipe, ctrl.wLength, tmo, SUBMIT, NULL, 0);
>
> usb_unlock_device(dev);
> i = usb_control_msg(dev, pipe, ctrl.bRequest,
> ctrl.bRequestType, ctrl.wValue, ctrl.wIndex,
> tbuf, ctrl.wLength, tmo);
> usb_lock_device(dev);
> - snoop_urb(dev, NULL, pipe, max(i, 0), min(i, 0), COMPLETE);
> + snoop_urb(dev, NULL, pipe, max(i, 0), min(i, 0), COMPLETE,
> + (i > 0 && ctrl.wLength) ? tbuf : NULL, i);
It's silly to test both i and ctrl.wLength. In fact, why test
anything? Just pass tbuf.
> @@ -853,12 +865,13 @@ static int proc_bulk(struct dev_state *ps, void __user *arg)
> kfree(tbuf);
> return -EINVAL;
> }
> - snoop_urb(dev, NULL, pipe, len1, tmo, SUBMIT);
> + snoop_urb(dev, NULL, pipe, len1, tmo, SUBMIT, NULL, 0);
>
> usb_unlock_device(dev);
> i = usb_bulk_msg(dev, pipe, tbuf, len1, &len2, tmo);
> usb_lock_device(dev);
> - snoop_urb(dev, NULL, pipe, len2, i, COMPLETE);
> + snoop_urb(dev, NULL, pipe, len2, i, COMPLETE,
> + (!i && len2) ? tbuf : NULL, len2);
Again, why test anything? Just pass tbuf.
> @@ -1097,6 +1110,13 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
> is_in = 0;
> uurb->endpoint &= ~USB_DIR_IN;
> }
> + snoop(&ps->dev->dev, "control urb: bRequest=%02x "
> + "bRrequestType=%02x wValue=%04x "
> + "wIndex=%04x wLength=%04x\n",
> + dr->bRequest, dr->bRequestType,
> + __le16_to_cpup(&dr->wValue),
> + __le16_to_cpup(&dr->wIndex),
> + __le16_to_cpup(&dr->wLength));
> break;
Why bother to add this here if you don't add equivalent code in
proc_control()? Also, people expect to see bRequestType (which you
misspelled) before bRequest, not after, since that's the order of the
fields in the actual packet.
> @@ -1236,7 +1256,9 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
> }
> }
> snoop_urb(ps->dev, as->userurb, as->urb->pipe,
> - as->urb->transfer_buffer_length, 0, SUBMIT);
> + as->urb->transfer_buffer_length, 0, SUBMIT,
> + (!is_in) ? as->urb->transfer_buffer : NULL,
Something like this is a little easier to understand if you avoid using
a negative:
is_in ? NULL : as->urb->transfer_buffer,
I'd still like to know why you don't find usbmon better than
snoop_urb() in every respect?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
