Uses the new snoop function from commit 4c6e8971cbe0148085,
but includes the buffer data where appropriate, as before.
Signed-off-by: Chris Frey <cdfrey@xxxxxxxxxxxxxx>
---
On Wed, Jan 20, 2010 at 10:43:08AM -0500, Alan Stern wrote:
> 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.
I suppose this is one way of getting others to clean up the kernel
for you. Rip out the existing code, and make them add it back
better than before. :-)
But yes, if we're going to do this, might as well do it right.
So thank you for the feedback.
Done.
> > @@ -777,14 +787,15 @@ static int proc_control(struct dev_state *ps, void __user *arg)
[...]
> > 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.
Done. I was following the code after it, which did something similar:
if ((i > 0) && ctrl.wLength)
But I probably didn't fully understand it.
> > @@ -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.
This was just copying back the old code that was removed earlier, warts
and all. Oops. I changed the order as requested, fixed the old spelling
mistake, and added a corresponding block to proc_control().
> I'd still like to know why you don't find usbmon better than
> snoop_urb() in every respect?
It's not a matter of better or worse. Usbfs_snoop existed, it worked fine,
people relied on it, and all it required to be useful was syslog and a
/sys toggle.
- Chris
drivers/usb/core/devio.c | 51 ++++++++++++++++++++++++++++++++++-----------
1 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 181f78c..7f887f5 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -310,7 +310,8 @@ static struct async *async_getpending(struct dev_state *ps,
static void snoop_urb(struct usb_device *udev,
void __user *userurb, int pipe, unsigned length,
- int timeout_or_status, enum snoop_when when)
+ int timeout_or_status, enum snoop_when when,
+ unsigned char *data, unsigned data_len)
{
static const char *types[] = {"isoc", "int", "ctrl", "bulk"};
static const char *dirs[] = {"out", "in"};
@@ -344,6 +345,11 @@ static void snoop_urb(struct usb_device *udev,
"status %d\n",
ep, t, d, length, timeout_or_status);
}
+
+ if (data && data_len > 0) {
+ print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE, 32, 1,
+ data, data_len, 1);
+ }
}
#define AS_CONTINUATION 1
@@ -410,7 +416,9 @@ 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_flags & URB_DIR_MASK) == USB_DIR_OUT) ?
+ NULL : urb->transfer_buffer, urb->actual_length);
if (as->status < 0 && as->bulk_addr && as->status != -ECONNRESET &&
as->status != -ENOENT)
cancel_bulk_urbs(ps, as->bulk_addr);
@@ -777,15 +785,22 @@ 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(&dev->dev, "control urb: bRequestType=%02x "
+ "bRequest=%02x wValue=%04x "
+ "wIndex=%04x wLength=%04x\n",
+ ctrl.bRequestType, ctrl.bRequest,
+ __le16_to_cpup(&ctrl.wValue),
+ __le16_to_cpup(&ctrl.wIndex),
+ __le16_to_cpup(&ctrl.wLength));
+ snoop_urb(dev, NULL, pipe, max(i, 0), min(i, 0), COMPLETE,
+ tbuf, i);
if ((i > 0) && ctrl.wLength) {
if (copy_to_user(ctrl.data, tbuf, i)) {
free_page((unsigned long)tbuf);
@@ -800,14 +815,15 @@ static int proc_control(struct dev_state *ps, void __user *arg)
}
}
pipe = usb_sndctrlpipe(dev, 0);
- snoop_urb(dev, NULL, pipe, ctrl.wLength, tmo, SUBMIT);
+ snoop_urb(dev, NULL, pipe, ctrl.wLength, tmo, SUBMIT,
+ tbuf, ctrl.wLength);
usb_unlock_device(dev);
i = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), 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, NULL, 0);
}
free_page((unsigned long)tbuf);
if (i < 0 && i != -EPIPE) {
@@ -853,12 +869,12 @@ 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, tbuf, len2);
if (!i && len2) {
if (copy_to_user(bulk.data, tbuf, len2)) {
@@ -873,12 +889,12 @@ static int proc_bulk(struct dev_state *ps, void __user *arg)
return -EFAULT;
}
}
- snoop_urb(dev, NULL, pipe, len1, tmo, SUBMIT);
+ snoop_urb(dev, NULL, pipe, len1, tmo, SUBMIT, tbuf, len1);
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, NULL, 0);
}
kfree(tbuf);
if (i < 0)
@@ -1097,6 +1113,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: bRequestType=%02x "
+ "bRequest=%02x wValue=%04x "
+ "wIndex=%04x wLength=%04x\n",
+ dr->bRequestType, dr->bRequest,
+ __le16_to_cpup(&dr->wValue),
+ __le16_to_cpup(&dr->wIndex),
+ __le16_to_cpup(&dr->wLength));
break;
case USBDEVFS_URB_TYPE_BULK:
@@ -1236,7 +1259,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 ? NULL : as->urb->transfer_buffer,
+ uurb->buffer_length);
async_newpending(as);
if (usb_endpoint_xfer_bulk(&ep->desc)) {
@@ -1274,7 +1299,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
dev_printk(KERN_DEBUG, &ps->dev->dev,
"usbfs: usb_submit_urb returned %d\n", ret);
snoop_urb(ps->dev, as->userurb, as->urb->pipe,
- 0, ret, COMPLETE);
+ 0, ret, COMPLETE, NULL, 0);
async_removepending(as);
free_async(as);
return ret;
--
1.6.2.5
--
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
