[U-Boot] [PATCH v3 0/1] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[U-Boot] [PATCH v3 0/1] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Hans de Goede]

Hi Simon,

Here is v3 of my patch to fix the maxpacketsize0 not being set issue I
found and related issues.

I've added a big fat comment to explain that the usb_device pointer is a
hack and should not be used outside of usb-uclass.c as requested.

I hope this version is to your liking and you can ack it.

Assuming that you do ack it, then my entire dm-usb fixes + ehci set is
all acked, and the question becomes how to take uit upstream, I can take
the entire set upstream through the sunxi tree, or you can take it
upstream through the dm tree. Let me know which way you prefer to move
forward with this.

Regards,

Hans

[U-Boot] [PATCH v3] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Hans de Goede]

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 3 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

3) Calling usb_select_config() a second time fails on some usb-1 devices
plugged into usb-2 hubs, causing u-boot to not recognize these devices.

This commit fixes these 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 | 30 +++++++++---------------------
 include/usb.h                 | 19 ++++++++++---------
 2 files changed, 19 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 714bc0e..c25db2f 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,18 @@ 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;
-	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);
+	/*
+	 * 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);
+	/* And clear plat->udev as it will not be valid for long */
+	plat->udev = NULL;
 
-	ret = usb_select_config(udev);
-	if (ret)
-		return ret;
+	udev->dev = dev;
 
 	return 0;
 }
diff --git a/include/usb.h b/include/usb.h
index 1984e8f..22ea491 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -571,20 +571,21 @@ struct usb_platdata {
  * This is used by sandbox to provide emulation data also.
  *
  * @id:		ID used to match this device
- * @speed:	Stores the speed associated with a USB device
- * @devnum:	Device address on the USB bus
- * @slot_id:	USB3 slot ID, which is separate from the device address
- * @portnr:	Port number of this device on its parent hub, numbered from 1
- *		(0 mean this device is the root hub)
+ * @udev:	usb-uclass internal use only do NOT use
  * @strings:	List of descriptor strings (for sandbox emulation purposes)
  * @desc_list:	List of descriptors (for sandbox emulation purposes)
  */
 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 */
+	/*
+	 * This pointer is used to pass the usb_device used in usb_scan_device,
+	 * to get the usb descriptors before the driver is known, to the
+	 * actual udevice once the driver is known and the udevice is created.
+	 * This will be NULL except during probe, do NOT use.
+	 *
+	 * This should eventually go away.
+	 */
+	struct usb_device *udev;
 #ifdef CONFIG_SANDBOX
 	struct usb_string *strings;
 	/* NULL-terminated list of descriptor pointers */
-- 
2.3.6

[U-Boot] [PATCH v3 0/1] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Simon Glass]

Hi Hans,

On 4 May 2015 at 11:19, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi Simon,
>
> Here is v3 of my patch to fix the maxpacketsize0 not being set issue I
> found and related issues.
>
> I've added a big fat comment to explain that the usb_device pointer is a
> hack and should not be used outside of usb-uclass.c as requested.
>
> I hope this version is to your liking and you can ack it.
>
> Assuming that you do ack it, then my entire dm-usb fixes + ehci set is
> all acked, and the question becomes how to take uit upstream, I can take
> the entire set upstream through the sunxi tree, or you can take it
> upstream through the dm tree. Let me know which way you prefer to move
> forward with this.

I should probably take it through DM. I'll do a pull request once the
x86 pull request goes in.

But I'm still not sure how it is safe to pass a pointer to a local
stack variable out through another function. Can you please explain
that?

Regards,
Simon

[U-Boot] [PATCH v3 0/1] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Hans de Goede]

Hi,

On 04-05-15 19:22, Simon Glass wrote:
> Hi Hans,
>
> On 4 May 2015 at 11:19, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi Simon,
>>
>> Here is v3 of my patch to fix the maxpacketsize0 not being set issue I
>> found and related issues.
>>
>> I've added a big fat comment to explain that the usb_device pointer is a
>> hack and should not be used outside of usb-uclass.c as requested.
>>
>> I hope this version is to your liking and you can ack it.
>>
>> Assuming that you do ack it, then my entire dm-usb fixes + ehci set is
>> all acked, and the question becomes how to take uit upstream, I can take
>> the entire set upstream through the sunxi tree, or you can take it
>> upstream through the dm tree. Let me know which way you prefer to move
>> forward with this.
>
> I should probably take it through DM. I'll do a pull request once the
> x86 pull request goes in.

Ack.

> But I'm still not sure how it is safe to pass a pointer to a local
> stack variable out through another function. Can you please explain
> that?

A function 'a' can safely pass a pointer to an on stack variable to
a function 'b' which it calls, since it is the caller of function 'b',
and as long as code from 'b' is execution we are still within the
lifetime of function 'a' (function 'a' will only end after b has
returned) and as long as execution is within the lifetime of function
'a' any on stack variables of function 'a' are valid.

AFAICT usb_child_pre_probe() gets called from the device_probe()
call inside usb_scan_device() so using a pointer to an on stack
variable of usb_scan_device() is valid since we are still executing
within the lifetime of usb_scan_device().

Hope this helps to follow my reasoning.

Regards,

Hans





>
> Regards,
> Simon
>

[U-Boot] [PATCH v3 0/1] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Simon Glass]

Hi Hans,

On 4 May 2015 at 13:12, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 04-05-15 19:22, Simon Glass wrote:
>>
>> Hi Hans,
>>
>> On 4 May 2015 at 11:19, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> Here is v3 of my patch to fix the maxpacketsize0 not being set issue I
>>> found and related issues.
>>>
>>> I've added a big fat comment to explain that the usb_device pointer is a
>>> hack and should not be used outside of usb-uclass.c as requested.
>>>
>>> I hope this version is to your liking and you can ack it.
>>>
>>> Assuming that you do ack it, then my entire dm-usb fixes + ehci set is
>>> all acked, and the question becomes how to take uit upstream, I can take
>>> the entire set upstream through the sunxi tree, or you can take it
>>> upstream through the dm tree. Let me know which way you prefer to move
>>> forward with this.
>>
>>
>> I should probably take it through DM. I'll do a pull request once the
>> x86 pull request goes in.
>
>
> Ack.
>
>> But I'm still not sure how it is safe to pass a pointer to a local
>> stack variable out through another function. Can you please explain
>> that?
>
>
> A function 'a' can safely pass a pointer to an on stack variable to
> a function 'b' which it calls, since it is the caller of function 'b',
> and as long as code from 'b' is execution we are still within the
> lifetime of function 'a' (function 'a' will only end after b has
> returned) and as long as execution is within the lifetime of function
> 'a' any on stack variables of function 'a' are valid.
>
> AFAICT usb_child_pre_probe() gets called from the device_probe()
> call inside usb_scan_device() so using a pointer to an on stack
> variable of usb_scan_device() is valid since we are still executing
> within the lifetime of usb_scan_device().
>
> Hope this helps to follow my reasoning.
>

Ah I see. Gosh that is evil. The comment helps.

Acked-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

[U-Boot] [PATCH v3] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Simon Glass]

On 4 May 2015 at 11:19, 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 3 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
>
> 3) Calling usb_select_config() a second time fails on some usb-1 devices
> plugged into usb-2 hubs, causing u-boot to not recognize these devices.
>
> This commit fixes these 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>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v3] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Simon Glass]

Hi Hans,

On 4 May 2015 at 14:28, Simon Glass <sjg@chromium.org> wrote:
> On 4 May 2015 at 11:19, 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 3 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
>>
>> 3) Calling usb_select_config() a second time fails on some usb-1 devices
>> plugged into usb-2 hubs, causing u-boot to not recognize these devices.
>>
>> This commit fixes these 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>
>
> Acked-by: Simon Glass <sjg@chromium.org>

But sadly this fails with sandbox. Please do run the driver model tests.

Regards,
Simon

[U-Boot] [PATCH v3] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Simon Glass]

On 4 May 2015 at 15:35, Simon Glass <sjg@chromium.org> wrote:
> Hi Hans,
>
> On 4 May 2015 at 14:28, Simon Glass <sjg@chromium.org> wrote:
>> On 4 May 2015 at 11:19, 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 3 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
>>>
>>> 3) Calling usb_select_config() a second time fails on some usb-1 devices
>>> plugged into usb-2 hubs, causing u-boot to not recognize these devices.
>>>
>>> This commit fixes these 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>
>>
>> Acked-by: Simon Glass <sjg@chromium.org>
>
> But sadly this fails with sandbox. Please do run the driver model tests.

Output:

02: dm: usb: Copy over usb_device values from usb_scan_device() to
final usb_device
   sandbox:  +   sandbox
+../drivers/usb/emul/usb-emul-uclass.c: In function ?usb_emul_find?:
+../drivers/usb/emul/usb-emul-uclass.c:125:11: error: ?struct
usb_dev_platdata? has no member named ?devnum?
+   if (udev->devnum == devnum) {
+           ^
+In file included from ../drivers/usb/emul/usb-emul-uclass.c:8:0:
+../drivers/usb/emul/usb-emul-uclass.c:127:25: error: ?struct
usb_dev_platdata? has no member named ?devnum?
+          dev->name, udev->devnum);
+                         ^
+../include/common.h:109:26: note: in definition of macro ?debug_cond?
+    printf(pr_fmt(fmt), ##args); \
+                          ^
+../drivers/usb/emul/usb-emul-uclass.c:126:4: note: in expansion of
macro ?debug?
+    debug("%s: Found emulator '%s', addr %d\n", __func__,
+    ^
+../drivers/usb/emul/usb-emul-uclass.c: In function ?usb_emul_control?:
+../drivers/usb/emul/usb-emul-uclass.c:168:8: error: ?struct
usb_dev_platdata? has no member named ?devnum?
+    plat->devnum = setup->value;
+        ^
+../drivers/usb/emul/usb-emul-uclass.c: In function ?usb_emul_reset?:
+../drivers/usb/emul/usb-emul-uclass.c:253:6: error: ?struct
usb_dev_platdata? has no member named ?devnum?
+  plat->devnum = 0;
+      ^

>
> Regards,
> Simon

[U-Boot] [PATCH v3] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Hans de Goede]

Hi,

On 04-05-15 23:35, Simon Glass wrote:
> On 4 May 2015 at 15:35, Simon Glass <sjg@chromium.org> wrote:
>> Hi Hans,
>>
>> On 4 May 2015 at 14:28, Simon Glass <sjg@chromium.org> wrote:
>>> On 4 May 2015 at 11:19, 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 3 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
>>>>
>>>> 3) Calling usb_select_config() a second time fails on some usb-1 devices
>>>> plugged into usb-2 hubs, causing u-boot to not recognize these devices.
>>>>
>>>> This commit fixes these 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>
>>>
>>> Acked-by: Simon Glass <sjg@chromium.org>
>>
>> But sadly this fails with sandbox. Please do run the driver model tests.
>
> Output:
>
> 02: dm: usb: Copy over usb_device values from usb_scan_device() to
> final usb_device
>     sandbox:  +   sandbox
> +../drivers/usb/emul/usb-emul-uclass.c: In function ?usb_emul_find?:
> +../drivers/usb/emul/usb-emul-uclass.c:125:11: error: ?struct
> usb_dev_platdata? has no member named ?devnum?
> +   if (udev->devnum == devnum) {
> +           ^
> +In file included from ../drivers/usb/emul/usb-emul-uclass.c:8:0:
> +../drivers/usb/emul/usb-emul-uclass.c:127:25: error: ?struct
> usb_dev_platdata? has no member named ?devnum?
> +          dev->name, udev->devnum);
> +                         ^
> +../include/common.h:109:26: note: in definition of macro ?debug_cond?
> +    printf(pr_fmt(fmt), ##args); \
> +                          ^
> +../drivers/usb/emul/usb-emul-uclass.c:126:4: note: in expansion of
> macro ?debug?
> +    debug("%s: Found emulator '%s', addr %d\n", __func__,
> +    ^
> +../drivers/usb/emul/usb-emul-uclass.c: In function ?usb_emul_control?:
> +../drivers/usb/emul/usb-emul-uclass.c:168:8: error: ?struct
> usb_dev_platdata? has no member named ?devnum?
> +    plat->devnum = setup->value;
> +        ^
> +../drivers/usb/emul/usb-emul-uclass.c: In function ?usb_emul_reset?:
> +../drivers/usb/emul/usb-emul-uclass.c:253:6: error: ?struct
> usb_dev_platdata? has no member named ?devnum?
> +  plat->devnum = 0;
> +      ^


Oops sorry about that, I'll try to learn myself to always run dm-test.sh
after making dm related changes.

Fixing this was a bit trickier then I would have liked, but I've it fixed
now. I also had to reshuffle the order of some patches for this, so I'm
going to post a v4 of the entire set. I've also added some extra comments
(but no functional changes) to the sunxi ehci dm patch as request by Ian,
so please take all patches from the v4 posting when merging.

Regards,

Hans