On Tue, 24 Jan 2012, Bjørn Mork wrote:
> 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
I don't care. You can even leave it in if you want; it's a small
thing. I was just pointing out what you had done in case you weren't
aware, that's all.
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
