On Fri, 22 Jan 2010, Chris Frey wrote:
> 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. :-)
Well, it seems to have worked. :-)
> > > @@ -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.
It's a question of whether to return partial data to userspace if there
was an error. The existing implementation probably isn't the best.
Regardless, you will always want to snoop the partial data.
> @@ -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);
You print out the setup packet details for control-IN URBs but not
control-OUT. If the snoop() statement were moved earlier it would
apply to both directions.
The rest looks okay.
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
