| Message ID | 1430404524-31413-2-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Hans, On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote: > Currently we copy over a number of usb_device values stored in the on stack > struct usb_device probed in usb_scan_device() to the final driver-model managed > struct usb_device in usb_child_pre_probe() through usb_device_platdata, and > then call usb_select_config() to fill in the rest. > > There are 2 problems with this approach: > > 1) It does not fill in enough fields before calling usb_select_config(), > specifically it does not fill in ep0's maxpacketsize causing a div by zero > exception in the ehci driver. Didn't you have another patch that fixes that? > > 2) It unnecessarily redoes a number of usb requests making usb probing slower, > and potentially upsetting some devices. Does it actually upset anything? The extra requests are in the second call to usb_select_config(). Do you think we could put the things we want to copy in a struct, and copy just those? > > This commit fixes both issues by removing the usb_select_config() call from > usb_child_pre_probe(), and instead of copying over things field by field > through usb_device_platdata, store a pointer to the in stack usb_device > (which is still valid when usb_child_pre_probe() gets called) and copy > over the entire struct. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/usb/host/usb-uclass.c | 27 ++++++--------------------- > include/usb.h | 5 +---- > 2 files changed, 7 insertions(+), 25 deletions(-) > > diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c > index 714bc0e..95e371d 100644 > --- a/drivers/usb/host/usb-uclass.c > +++ b/drivers/usb/host/usb-uclass.c > @@ -535,12 +535,7 @@ int usb_scan_device(struct udevice *parent, int port, > } > plat = dev_get_parent_platdata(dev); > debug("%s: Probing '%s', plat=%p\n", __func__, dev->name, plat); > - plat->devnum = udev->devnum; > - plat->speed = udev->speed; > - plat->slot_id = udev->slot_id; > - plat->portnr = port; > - debug("** device '%s': stashing slot_id=%d\n", dev->name, > - plat->slot_id); > + plat->udev = udev; > priv->next_addr++; > ret = device_probe(dev); > if (ret) { > @@ -599,25 +594,15 @@ int usb_get_bus(struct udevice *dev, struct udevice **busp) > > int usb_child_pre_probe(struct udevice *dev) > { > - struct udevice *bus; > struct usb_device *udev = dev_get_parentdata(dev); > struct usb_dev_platdata *plat = dev_get_parent_platdata(dev); > - int ret; > > - ret = usb_get_bus(dev, &bus); > - if (ret) > - return ret; > - udev->controller_dev = bus; > + /* > + * Copy over all the values set in the on stack struct usb_device in > + * usb_scan_device() to our final struct usb_device for this dev. > + */ > + *udev = *(plat->udev); > udev->dev = dev; > - udev->devnum = plat->devnum; > - udev->slot_id = plat->slot_id; > - udev->portnr = plat->portnr; > - udev->speed = plat->speed; > - debug("** device '%s': getting slot_id=%d\n", dev->name, plat->slot_id); > - > - ret = usb_select_config(udev); > - if (ret) > - return ret; > > return 0; > } > diff --git a/include/usb.h b/include/usb.h > index 1984e8f..1b667ff 100644 > --- a/include/usb.h > +++ b/include/usb.h > @@ -581,10 +581,7 @@ struct usb_platdata { > */ > struct usb_dev_platdata { > struct usb_device_id id; > - enum usb_device_speed speed; > - int devnum; > - int slot_id; > - int portnr; /* Hub port number, 1..n */ > + struct usb_device *udev; > #ifdef CONFIG_SANDBOX > struct usb_string *strings; > /* NULL-terminated list of descriptor pointers */ > -- > 2.3.6 > Regards, Simon
Hi, On 01-05-15 06:11, Simon Glass wrote: > Hi Hans, > > On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote: >> Currently we copy over a number of usb_device values stored in the on stack >> struct usb_device probed in usb_scan_device() to the final driver-model managed >> struct usb_device in usb_child_pre_probe() through usb_device_platdata, and >> then call usb_select_config() to fill in the rest. >> >> There are 2 problems with this approach: >> >> 1) It does not fill in enough fields before calling usb_select_config(), >> specifically it does not fill in ep0's maxpacketsize causing a div by zero >> exception in the ehci driver. > > Didn't you have another patch that fixes that? Yes, but as explained in the coverletter of that patch I believe that this version is better. It would seem we disagree on that though :) >> 2) It unnecessarily redoes a number of usb requests making usb probing slower, >> and potentially upsetting some devices. > > Does it actually upset anything? Not to my knowledge, but I'm afraid that with some devices it may. > The extra requests are in the second call to usb_select_config(). Correct. > Do you think we could put the things we want to copy in a struct, and > copy just those? We would end up pretty much duplicating usb_device AFAICT, since we need the descriptors, the max packet sizes per endpoint parsed from them, etc. Anyways since you clearly dislike this patch I'll drop it for v2, replacing it with my original fix for the ep0 maxpacket not being set issue, assuming that doing so does not cause any regressions during my testing. Regards, Hans > >> >> This commit fixes both issues by removing the usb_select_config() call from >> usb_child_pre_probe(), and instead of copying over things field by field >> through usb_device_platdata, store a pointer to the in stack usb_device >> (which is still valid when usb_child_pre_probe() gets called) and copy >> over the entire struct. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/usb/host/usb-uclass.c | 27 ++++++--------------------- >> include/usb.h | 5 +---- >> 2 files changed, 7 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c >> index 714bc0e..95e371d 100644 >> --- a/drivers/usb/host/usb-uclass.c >> +++ b/drivers/usb/host/usb-uclass.c >> @@ -535,12 +535,7 @@ int usb_scan_device(struct udevice *parent, int port, >> } >> plat = dev_get_parent_platdata(dev); >> debug("%s: Probing '%s', plat=%p\n", __func__, dev->name, plat); >> - plat->devnum = udev->devnum; >> - plat->speed = udev->speed; >> - plat->slot_id = udev->slot_id; >> - plat->portnr = port; >> - debug("** device '%s': stashing slot_id=%d\n", dev->name, >> - plat->slot_id); >> + plat->udev = udev; >> priv->next_addr++; >> ret = device_probe(dev); >> if (ret) { >> @@ -599,25 +594,15 @@ int usb_get_bus(struct udevice *dev, struct udevice **busp) >> >> int usb_child_pre_probe(struct udevice *dev) >> { >> - struct udevice *bus; >> struct usb_device *udev = dev_get_parentdata(dev); >> struct usb_dev_platdata *plat = dev_get_parent_platdata(dev); >> - int ret; >> >> - ret = usb_get_bus(dev, &bus); >> - if (ret) >> - return ret; >> - udev->controller_dev = bus; >> + /* >> + * Copy over all the values set in the on stack struct usb_device in >> + * usb_scan_device() to our final struct usb_device for this dev. >> + */ >> + *udev = *(plat->udev); >> udev->dev = dev; >> - udev->devnum = plat->devnum; >> - udev->slot_id = plat->slot_id; >> - udev->portnr = plat->portnr; >> - udev->speed = plat->speed; >> - debug("** device '%s': getting slot_id=%d\n", dev->name, plat->slot_id); >> - >> - ret = usb_select_config(udev); >> - if (ret) >> - return ret; >> >> return 0; >> } >> diff --git a/include/usb.h b/include/usb.h >> index 1984e8f..1b667ff 100644 >> --- a/include/usb.h >> +++ b/include/usb.h >> @@ -581,10 +581,7 @@ struct usb_platdata { >> */ >> struct usb_dev_platdata { >> struct usb_device_id id; >> - enum usb_device_speed speed; >> - int devnum; >> - int slot_id; >> - int portnr; /* Hub port number, 1..n */ >> + struct usb_device *udev; >> #ifdef CONFIG_SANDBOX >> struct usb_string *strings; >> /* NULL-terminated list of descriptor pointers */ >> -- >> 2.3.6 >> > > Regards, > Simon >
Hi, On 01-05-15 09:21, Hans de Goede wrote: > Hi, > > On 01-05-15 06:11, Simon Glass wrote: >> Hi Hans, >> >> On 30 April 2015 at 08:35, Hans de Goede <hdegoede@redhat.com> wrote: >>> Currently we copy over a number of usb_device values stored in the on stack >>> struct usb_device probed in usb_scan_device() to the final driver-model managed >>> struct usb_device in usb_child_pre_probe() through usb_device_platdata, and >>> then call usb_select_config() to fill in the rest. >>> >>> There are 2 problems with this approach: >>> >>> 1) It does not fill in enough fields before calling usb_select_config(), >>> specifically it does not fill in ep0's maxpacketsize causing a div by zero >>> exception in the ehci driver. >> >> Didn't you have another patch that fixes that? > > Yes, but as explained in the coverletter of that patch I believe that > this version is better. It would seem we disagree on that though :) > >>> 2) It unnecessarily redoes a number of usb requests making usb probing slower, >>> and potentially upsetting some devices. >> >> Does it actually upset anything? > > Not to my knowledge, but I'm afraid that with some devices it may. > >> The extra requests are in the second call to usb_select_config(). > > Correct. > >> Do you think we could put the things we want to copy in a struct, and >> copy just those? > > We would end up pretty much duplicating usb_device AFAICT, since we need > the descriptors, the max packet sizes per endpoint parsed from them, etc. > > Anyways since you clearly dislike this patch I'll drop it for v2, replacing > it with my original fix for the ep0 maxpacket not being set issue, assuming > that doing so does not cause any regressions during my testing. Ok, so my first test, which is hooking up a sunxi device to my dvi/usb kvm switch fails immediately when I use the maxpacketsize fix instead of my fix to avoid calling get_config twice. This is actually a pretty though test-case as the kvm presents itself + the keyboard and mouse as a complex hub hierarchy with both usb-2 and usb-1 hubs in there, which is what makes it a great test-case. Now I could spend a couple of hours debugging this and maybe find a different fix, but this really shows that there is a reason why all usb stacks do the device probing / descriptor reading all in the exact same sequence, because usb devices are cheap and there qa consists of plug it into $random windows version running machine, does it work? Yes -> ship it. So I really believe that my original fix for this is best. As for trying to pass all the bits we need through platdata rather then passing the struct usb_device itself. I can see value in that as part of a patch-set to get rid of usb_device, iow as part of a larger patchset but until then it just feels like make work to me. So I'm going to stick with my original approach for v1 of this patch. Regards, Hans
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index 714bc0e..95e371d 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -535,12 +535,7 @@ int usb_scan_device(struct udevice *parent, int port, } plat = dev_get_parent_platdata(dev); debug("%s: Probing '%s', plat=%p\n", __func__, dev->name, plat); - plat->devnum = udev->devnum; - plat->speed = udev->speed; - plat->slot_id = udev->slot_id; - plat->portnr = port; - debug("** device '%s': stashing slot_id=%d\n", dev->name, - plat->slot_id); + plat->udev = udev; priv->next_addr++; ret = device_probe(dev); if (ret) { @@ -599,25 +594,15 @@ int usb_get_bus(struct udevice *dev, struct udevice **busp) int usb_child_pre_probe(struct udevice *dev) { - struct udevice *bus; struct usb_device *udev = dev_get_parentdata(dev); struct usb_dev_platdata *plat = dev_get_parent_platdata(dev); - int ret; - ret = usb_get_bus(dev, &bus); - if (ret) - return ret; - udev->controller_dev = bus; + /* + * Copy over all the values set in the on stack struct usb_device in + * usb_scan_device() to our final struct usb_device for this dev. + */ + *udev = *(plat->udev); udev->dev = dev; - udev->devnum = plat->devnum; - udev->slot_id = plat->slot_id; - udev->portnr = plat->portnr; - udev->speed = plat->speed; - debug("** device '%s': getting slot_id=%d\n", dev->name, plat->slot_id); - - ret = usb_select_config(udev); - if (ret) - return ret; return 0; } diff --git a/include/usb.h b/include/usb.h index 1984e8f..1b667ff 100644 --- a/include/usb.h +++ b/include/usb.h @@ -581,10 +581,7 @@ struct usb_platdata { */ struct usb_dev_platdata { struct usb_device_id id; - enum usb_device_speed speed; - int devnum; - int slot_id; - int portnr; /* Hub port number, 1..n */ + struct usb_device *udev; #ifdef CONFIG_SANDBOX struct usb_string *strings; /* NULL-terminated list of descriptor pointers */
Currently we copy over a number of usb_device values stored in the on stack struct usb_device probed in usb_scan_device() to the final driver-model managed struct usb_device in usb_child_pre_probe() through usb_device_platdata, and then call usb_select_config() to fill in the rest. There are 2 problems with this approach: 1) It does not fill in enough fields before calling usb_select_config(), specifically it does not fill in ep0's maxpacketsize causing a div by zero exception in the ehci driver. 2) It unnecessarily redoes a number of usb requests making usb probing slower, and potentially upsetting some devices. This commit fixes both issues by removing the usb_select_config() call from usb_child_pre_probe(), and instead of copying over things field by field through usb_device_platdata, store a pointer to the in stack usb_device (which is still valid when usb_child_pre_probe() gets called) and copy over the entire struct. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/host/usb-uclass.c | 27 ++++++--------------------- include/usb.h | 5 +---- 2 files changed, 7 insertions(+), 25 deletions(-)