Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
> On Tue, 24 Jan 2012, Bjørn Mork wrote:
>
>> @@ -636,7 +638,13 @@ static struct wdm_device *wdm_create(struct usb_interface *intf, struct usb_endp
>> INIT_WORK(&desc->rxwork, wdm_rxwork);
>>
>> rv = -EINVAL;
>> - if (!ep || !usb_endpoint_is_int_in(ep))
>> + iface = intf->cur_altsetting;
>> + for (i = 0; i < iface->desc.bNumEndpoints; i++) {
>> + ep = &iface->endpoint[i].desc;
>> + if (ep && usb_endpoint_is_int_in(ep))
>
> Testing ep at this point makes no sense. Do you understand why? Hint:
> Consider what value ep would have if desc were not the first field in
> the usb_host_endpoint structure.
Absolutely. That's what you end up with when you copy code around
without thinking...
To my defense I will state that it has been like that forever, and I'm
not the first one to copy it when fixing something else. And it was
important to me to change as little as possible.
But you are of course right. Fixing this up will only make my changes
easier to read and verify.
> It would make more sense to test iface->endpoint[i] -- but that can't
> be NULL either; this is guaranteed by usbcore.
Yup, I think there is a lot of weird descriptor parsing around, but
that's not an argument for keeping it like this.
Will fix. Thanks for pointing it out.
Just wonder: Would you prefer this to come as a trivial patch against
the current driver *before* moving stuff around? I guess that makes the
development easier to follow
Bjørn
--
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
