On Tuesday 12 February 2008 04:07:00 David Brownell wrote:
> > So here is the new proposal.
>
> Which presumably works for your device ...
yest it does.
> it's looks mostly
> OK, other than what scripts/checkpatch.pl will say and the added
>
> whitespace, but:
<snip>
> > + /* now we need to compute dev->maxpacket */
> > + dev->maxpacket = usb_maxpacket (dev->udev, dev->out, 1);
> > +
> > + if (dev->maxpacket == 0) {
> > + dev_err(&intf->dev, "dev->maxpacket can't be 0\n");
> > + retval = -EINVAL;
> > + goto fail_and_release;
> > + }
>
> Why is this "if (dev->maxpacket == 0)" test needed?
>
> It looks like pure paranoia. If it's possible, the
> bug would be someplace else. Specifically, an endpoint
> descriptor with an invalid maxpacket field ... something
> that, if we check for it, should be tested in usbcore
> as part of descriptor parsing, not special cased in
> this driver.
Maybe it is paranoia but the point is that if it is 0 (for whatever reason)
the driver will not work as it will end with 0 in dev->rx_urb_size. So, it
seems to me that adding a small test here (where there is no real performance
constraint) in order to be sure that everything is OK is reasonable. If this
test was here before the problem would have been detected much earlier.
Let's ask the question: if usbcore was detecting an endpoint with a 0
dev->maxpacket would it reject it right away or would it pass it anyway
trusting that the specific driver would know about this problem (buggy
hardware is everywhere) and work around it in software.
In my case I preffer to test things and reject the bind() if I find a problem.
Now removing the test will obviously be OK in most cases ...
-
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
