[U-Boot] [PATCH fix for v2014.10 0/5] USB keyboard: don't crash on "usb reset"

[U-Boot] [PATCH fix for v2014.10 0/5] USB keyboard: don't crash on "usb reset"

[Hans de Goede]

Hi Marek,

Here is the second set of USB patches / fixes I've been working on, currently
u-boot does "really bad things" (tm) when doing "usb reset" while using an
usb keyboard. This set fixes this.

Given that these fix "really bad things" (tm) I not only believe that these are
suitable to go into v2014.10, but I also believe that they actually should :)

Regards,

Hans

[U-Boot] [PATCH fix for v2014.10 1/5] usb: kbd: Do not treat -ENODEV as an error for usb_kbd_deregister

[Hans de Goede]

ENODEV menas no usb keyboard was registered, threat this as a successful
usb_kbd_deregister.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/usb_kbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 87f4125..4c17b0d 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -8,6 +8,7 @@
  * SPDX-License-Identifier:	GPL-2.0+
  */
 #include <common.h>
+#include <errno.h>
 #include <malloc.h>
 #include <stdio_dev.h>
 #include <asm/byteorder.h>
@@ -559,7 +560,11 @@ int drv_usb_kbd_init(void)
 int usb_kbd_deregister(void)
 {
 #ifdef CONFIG_SYS_STDIO_DEREGISTER
-	return stdio_deregister(DEVNAME);
+	int ret = stdio_deregister(DEVNAME);
+	if (ret && ret != -ENODEV)
+		return ret;
+
+	return 0;
 #else
 	return 1;
 #endif
-- 
2.1.0

[U-Boot] [PATCH fix for v2014.10 2/5] usb: kbd: On a "usb reset" call usb_kbd_deregister() before calling usb_stop()

[Hans de Goede]

We need to call usb_kbd_deregister() before calling usb_stop().

usbkbd's stdio_dev->priv points to the usb_device, and usb_kbd_testc
dereferences usb_device->privptr.

usb_stop zeros usb_device, leaving usb_device->privptr NULL, causing
bad things (tm) to happen once control returns to the main loop and
usb_kbd_testc gets called.

Calling usb_kbd_deregister() avoids this. Note that we do not allow
the "usb reset" to continue when the deregister fails. This will be fixed
in a later patch.

For the same reasons always fail "usb stop" if the usb_kbd_deregister() fails,
even in the force path. This can happen when CONFIG_SYS_STDIO_DEREGISTER is
not set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/cmd_usb.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/common/cmd_usb.c b/common/cmd_usb.c
index 2519497..b2aa44c 100644
--- a/common/cmd_usb.c
+++ b/common/cmd_usb.c
@@ -430,6 +430,16 @@ static int do_usbboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 }
 #endif /* CONFIG_USB_STORAGE */
 
+static int do_usb_stop_keyboard(void)
+{
+#ifdef CONFIG_USB_KEYBOARD
+	if (usb_kbd_deregister() != 0) {
+		printf("USB not stopped: usbkbd still using USB\n");
+		return 1;
+	}
+#endif
+	return 0;
+}
 
 /******************************************************************************
  * usb command intepreter
@@ -450,6 +460,8 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if ((strncmp(argv[1], "reset", 5) == 0) ||
 		 (strncmp(argv[1], "start", 5) == 0)) {
 		bootstage_mark_name(BOOTSTAGE_ID_USB_START, "usb_start");
+		if (do_usb_stop_keyboard() != 0)
+			return 1;
 		usb_stop();
 		printf("(Re)start USB...\n");
 		if (usb_init() >= 0) {
@@ -468,19 +480,10 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return 0;
 	}
 	if (strncmp(argv[1], "stop", 4) == 0) {
-#ifdef CONFIG_USB_KEYBOARD
-		if (argc == 2) {
-			if (usb_kbd_deregister() != 0) {
-				printf("USB not stopped: usbkbd still"
-					" using USB\n");
-				return 1;
-			}
-		} else {
-			/* forced stop, switch console in to serial */
+		if (argc != 2)
 			console_assign(stdin, "serial");
-			usb_kbd_deregister();
-		}
-#endif
+		if (do_usb_stop_keyboard() != 0)
+			return 1;
 		printf("stopping USB..\n");
 		usb_stop();
 		return 0;
-- 
2.1.0

[U-Boot] [PATCH fix for v2014.10 3/5] usb: kbd: Remove check for already being registered

[Hans de Goede]

We now always properly deregister the keyboard before calling
drv_usb_kbd_init(), so we can drop the check for already being registered.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/usb_kbd.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 4c17b0d..d4d5f48 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -490,7 +490,7 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
 /* Search for keyboard and register it if found. */
 int drv_usb_kbd_init(void)
 {
-	struct stdio_dev usb_kbd_dev, *old_dev;
+	struct stdio_dev usb_kbd_dev;
 	struct usb_device *dev;
 	char *stdinname = getenv("stdin");
 	int error, i;
@@ -509,16 +509,6 @@ int drv_usb_kbd_init(void)
 		if (usb_kbd_probe(dev, 0) != 1)
 			continue;
 
-		/* We found a keyboard, check if it is already registered. */
-		debug("USB KBD: found set up device.\n");
-		old_dev = stdio_get_by_name(DEVNAME);
-		if (old_dev) {
-			/* Already registered, just return ok. */
-			debug("USB KBD: is already registered.\n");
-			usb_kbd_deregister();
-			return 1;
-		}
-
 		/* Register the keyboard */
 		debug("USB KBD: register.\n");
 		memset(&usb_kbd_dev, 0, sizeof(struct stdio_dev));
-- 
2.1.0

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Hans de Goede]

In some cases we really want to move forward with a deregister, add a force
parameter to allow this, and replace the dev with a nulldev in this case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/stdio.c                 | 13 ++++++++++---
 common/usb_kbd.c               |  2 +-
 drivers/serial/serial-uclass.c |  2 +-
 include/stdio_dev.h            |  4 ++--
 4 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/common/stdio.c b/common/stdio.c
index c878103..8232815 100644
--- a/common/stdio.c
+++ b/common/stdio.c
@@ -34,6 +34,9 @@ char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" };
 #define	CONFIG_SYS_DEVICE_NULLDEV	1
 #endif
 
+#ifdef	CONFIG_SYS_STDIO_DEREGISTER
+#define	CONFIG_SYS_DEVICE_NULLDEV	1
+#endif
 
 #ifdef CONFIG_SYS_DEVICE_NULLDEV
 void nulldev_putc(struct stdio_dev *dev, const char c)
@@ -172,7 +175,7 @@ int stdio_register(struct stdio_dev *dev)
  * returns 0 if success, -1 if device is assigned and 1 if devname not found
  */
 #ifdef	CONFIG_SYS_STDIO_DEREGISTER
-int stdio_deregister_dev(struct stdio_dev *dev)
+int stdio_deregister_dev(struct stdio_dev *dev, int force)
 {
 	int l;
 	struct list_head *pos;
@@ -181,6 +184,10 @@ int stdio_deregister_dev(struct stdio_dev *dev)
 	/* get stdio devices (ListRemoveItem changes the dev list) */
 	for (l=0 ; l< MAX_FILES; l++) {
 		if (stdio_devices[l] == dev) {
+			if (force) {
+				strcpy(temp_names[l], "nulldev");
+				continue;
+			}
 			/* Device is assigned -> report error */
 			return -1;
 		}
@@ -202,7 +209,7 @@ int stdio_deregister_dev(struct stdio_dev *dev)
 	return 0;
 }
 
-int stdio_deregister(const char *devname)
+int stdio_deregister(const char *devname, int force)
 {
 	struct stdio_dev *dev;
 
@@ -211,7 +218,7 @@ int stdio_deregister(const char *devname)
 	if (!dev) /* device not found */
 		return -ENODEV;
 
-	return stdio_deregister_dev(dev);
+	return stdio_deregister_dev(dev, force);
 }
 #endif	/* CONFIG_SYS_STDIO_DEREGISTER */
 
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index d4d5f48..dcb693d 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -550,7 +550,7 @@ int drv_usb_kbd_init(void)
 int usb_kbd_deregister(void)
 {
 #ifdef CONFIG_SYS_STDIO_DEREGISTER
-	int ret = stdio_deregister(DEVNAME);
+	int ret = stdio_deregister(DEVNAME, 0);
 	if (ret && ret != -ENODEV)
 		return ret;
 
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index d04104e..61cbdc6 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
 #ifdef CONFIG_SYS_STDIO_DEREGISTER
 	struct serial_dev_priv *upriv = dev->uclass_priv;
 
-	if (stdio_deregister_dev(upriv->sdev))
+	if (stdio_deregister_dev(upriv->sdev), 0)
 		return -EPERM;
 #endif
 
diff --git a/include/stdio_dev.h b/include/stdio_dev.h
index 268de8e..24da23f 100644
--- a/include/stdio_dev.h
+++ b/include/stdio_dev.h
@@ -103,8 +103,8 @@ int stdio_init(void);
 
 void	stdio_print_current_devices(void);
 #ifdef CONFIG_SYS_STDIO_DEREGISTER
-int	stdio_deregister(const char *devname);
-int stdio_deregister_dev(struct stdio_dev *dev);
+int stdio_deregister(const char *devname, int force);
+int stdio_deregister_dev(struct stdio_dev *dev, int force);
 #endif
 struct list_head* stdio_get_list(void);
 struct stdio_dev* stdio_get_by_name(const char* name);
-- 
2.1.0

[U-Boot] [PATCH fix for v2014.10 5/5] usb: kbd: Allow "usb reset" to continue when an usb kbd is used

[Hans de Goede]

Use the new force parameter to make the stdio_deregister succeed, replacing
stdin with a nulldev, and assume that the usb keyboard will come back after
the reset.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/cmd_usb.c | 8 ++++----
 common/usb_kbd.c | 4 ++--
 include/usb.h    | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/common/cmd_usb.c b/common/cmd_usb.c
index b2aa44c..c192498 100644
--- a/common/cmd_usb.c
+++ b/common/cmd_usb.c
@@ -430,10 +430,10 @@ static int do_usbboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 }
 #endif /* CONFIG_USB_STORAGE */
 
-static int do_usb_stop_keyboard(void)
+static int do_usb_stop_keyboard(int force)
 {
 #ifdef CONFIG_USB_KEYBOARD
-	if (usb_kbd_deregister() != 0) {
+	if (usb_kbd_deregister(force) != 0) {
 		printf("USB not stopped: usbkbd still using USB\n");
 		return 1;
 	}
@@ -460,7 +460,7 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if ((strncmp(argv[1], "reset", 5) == 0) ||
 		 (strncmp(argv[1], "start", 5) == 0)) {
 		bootstage_mark_name(BOOTSTAGE_ID_USB_START, "usb_start");
-		if (do_usb_stop_keyboard() != 0)
+		if (do_usb_stop_keyboard(1) != 0)
 			return 1;
 		usb_stop();
 		printf("(Re)start USB...\n");
@@ -482,7 +482,7 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (strncmp(argv[1], "stop", 4) == 0) {
 		if (argc != 2)
 			console_assign(stdin, "serial");
-		if (do_usb_stop_keyboard() != 0)
+		if (do_usb_stop_keyboard(0) != 0)
 			return 1;
 		printf("stopping USB..\n");
 		usb_stop();
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index dcb693d..fdc083c 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -547,10 +547,10 @@ int drv_usb_kbd_init(void)
 }
 
 /* Deregister the keyboard. */
-int usb_kbd_deregister(void)
+int usb_kbd_deregister(int force)
 {
 #ifdef CONFIG_SYS_STDIO_DEREGISTER
-	int ret = stdio_deregister(DEVNAME, 0);
+	int ret = stdio_deregister(DEVNAME, force);
 	if (ret && ret != -ENODEV)
 		return ret;
 
diff --git a/include/usb.h b/include/usb.h
index d9fedee..c355fbe 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -216,7 +216,7 @@ int usb_host_eth_scan(int mode);
 #ifdef CONFIG_USB_KEYBOARD
 
 int drv_usb_kbd_init(void);
-int usb_kbd_deregister(void);
+int usb_kbd_deregister(int force);
 
 #endif
 /* routines */
-- 
2.1.0

[U-Boot] [PATCH fix for v2014.10 0/5] USB keyboard: don't crash on "usb reset"

[Marek Vasut]

On Saturday, September 20, 2014 at 04:54:33 PM, Hans de Goede wrote:
> Hi Marek,
> 
> Here is the second set of USB patches / fixes I've been working on,
> currently u-boot does "really bad things" (tm) when doing "usb reset"
> while using an usb keyboard. This set fixes this.
> 
> Given that these fix "really bad things" (tm) I not only believe that these
> are suitable to go into v2014.10, but I also believe that they actually
> should :)

Applied all, thanks a lot!

Best regards,
Marek Vasut

[U-Boot] [PATCH fix for v2014.10 2/5] usb: kbd: On a "usb reset" call usb_kbd_deregister() before calling usb_stop()

[Marek Vasut]

On Saturday, September 20, 2014 at 04:54:35 PM, Hans de Goede wrote:
> We need to call usb_kbd_deregister() before calling usb_stop().
> 
> usbkbd's stdio_dev->priv points to the usb_device, and usb_kbd_testc
> dereferences usb_device->privptr.
> 
> usb_stop zeros usb_device, leaving usb_device->privptr NULL, causing
> bad things (tm) to happen once control returns to the main loop and
> usb_kbd_testc gets called.
> 
> Calling usb_kbd_deregister() avoids this. Note that we do not allow
> the "usb reset" to continue when the deregister fails. This will be fixed
> in a later patch.
> 
> For the same reasons always fail "usb stop" if the usb_kbd_deregister()
> fails, even in the force path. This can happen when
> CONFIG_SYS_STDIO_DEREGISTER is not set.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  common/cmd_usb.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
> index 2519497..b2aa44c 100644
> --- a/common/cmd_usb.c
> +++ b/common/cmd_usb.c
> @@ -430,6 +430,16 @@ static int do_usbboot(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) }
>  #endif /* CONFIG_USB_STORAGE */
> 
> +static int do_usb_stop_keyboard(void)
> +{
> +#ifdef CONFIG_USB_KEYBOARD
> +	if (usb_kbd_deregister() != 0) {
> +		printf("USB not stopped: usbkbd still using USB\n");

I tried this set of patches with the DWC2 driver on RPI just now and i get this 
message and 'usb start' fails and doesn't scan bus. I suspect we have a problem 
here, can you check and post a fix ?

Best regards,
Marek Vasut

[U-Boot] [PATCH fix for v2014.10 2/5] usb: kbd: On a "usb reset" call usb_kbd_deregister() before calling usb_stop()

[Hans de Goede]

Hi,

On 09/22/2014 02:01 PM, Marek Vasut wrote:
> On Saturday, September 20, 2014 at 04:54:35 PM, Hans de Goede wrote:
>> We need to call usb_kbd_deregister() before calling usb_stop().
>>
>> usbkbd's stdio_dev->priv points to the usb_device, and usb_kbd_testc
>> dereferences usb_device->privptr.
>>
>> usb_stop zeros usb_device, leaving usb_device->privptr NULL, causing
>> bad things (tm) to happen once control returns to the main loop and
>> usb_kbd_testc gets called.
>>
>> Calling usb_kbd_deregister() avoids this. Note that we do not allow
>> the "usb reset" to continue when the deregister fails. This will be fixed
>> in a later patch.
>>
>> For the same reasons always fail "usb stop" if the usb_kbd_deregister()
>> fails, even in the force path. This can happen when
>> CONFIG_SYS_STDIO_DEREGISTER is not set.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  common/cmd_usb.c | 27 +++++++++++++++------------
>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
>> index 2519497..b2aa44c 100644
>> --- a/common/cmd_usb.c
>> +++ b/common/cmd_usb.c
>> @@ -430,6 +430,16 @@ static int do_usbboot(cmd_tbl_t *cmdtp, int flag, int
>> argc, char * const argv[]) }
>>  #endif /* CONFIG_USB_STORAGE */
>>
>> +static int do_usb_stop_keyboard(void)
>> +{
>> +#ifdef CONFIG_USB_KEYBOARD
>> +	if (usb_kbd_deregister() != 0) {
>> +		printf("USB not stopped: usbkbd still using USB\n");
> 
> I tried this set of patches with the DWC2 driver on RPI just now and i get this 
> message and 'usb start' fails and doesn't scan bus. I suspect we have a problem 
> here, can you check and post a fix ?

Do you have the entire set applied ? This is a known regression as mentioned
in the commit msg. Once the force parameter is in place this should go
away for a start/reset.

Also is usb start being called twice ? The first deregister should never fail,
since then no device is registered.

Regards,

Hans

[U-Boot] [PATCH fix for v2014.10 2/5] usb: kbd: On a "usb reset" call usb_kbd_deregister() before calling usb_stop()

[Marek Vasut]

On Tuesday, September 23, 2014 at 11:10:34 AM, Hans de Goede wrote:
> Hi,
> 
> On 09/22/2014 02:01 PM, Marek Vasut wrote:
> > On Saturday, September 20, 2014 at 04:54:35 PM, Hans de Goede wrote:
> >> We need to call usb_kbd_deregister() before calling usb_stop().
> >> 
> >> usbkbd's stdio_dev->priv points to the usb_device, and usb_kbd_testc
> >> dereferences usb_device->privptr.
> >> 
> >> usb_stop zeros usb_device, leaving usb_device->privptr NULL, causing
> >> bad things (tm) to happen once control returns to the main loop and
> >> usb_kbd_testc gets called.
> >> 
> >> Calling usb_kbd_deregister() avoids this. Note that we do not allow
> >> the "usb reset" to continue when the deregister fails. This will be
> >> fixed in a later patch.
> >> 
> >> For the same reasons always fail "usb stop" if the usb_kbd_deregister()
> >> fails, even in the force path. This can happen when
> >> CONFIG_SYS_STDIO_DEREGISTER is not set.
> >> 
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> 
> >>  common/cmd_usb.c | 27 +++++++++++++++------------
> >>  1 file changed, 15 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
> >> index 2519497..b2aa44c 100644
> >> --- a/common/cmd_usb.c
> >> +++ b/common/cmd_usb.c
> >> @@ -430,6 +430,16 @@ static int do_usbboot(cmd_tbl_t *cmdtp, int flag,
> >> int argc, char * const argv[]) }
> >> 
> >>  #endif /* CONFIG_USB_STORAGE */
> >> 
> >> +static int do_usb_stop_keyboard(void)
> >> +{
> >> +#ifdef CONFIG_USB_KEYBOARD
> >> +	if (usb_kbd_deregister() != 0) {
> >> +		printf("USB not stopped: usbkbd still using USB\n");
> > 
> > I tried this set of patches with the DWC2 driver on RPI just now and i
> > get this message and 'usb start' fails and doesn't scan bus. I suspect
> > we have a problem here, can you check and post a fix ?
> 
> Do you have the entire set applied ?

Yes, the current u-boot-usb/master

> This is a known regression as
> mentioned in the commit msg. Once the force parameter is in place this
> should go away for a start/reset.

You mean the 'force' param ?

> Also is usb start being called twice ? The first deregister should never
> fail, since then no device is registered.

Ugh, I must be dumb or somesuch, for whatever reason the issue went away and I 
can no longer replicate it. I will keep on a lookout.

Best regards,
Marek Vasut

[U-Boot] [PATCH fix for v2014.10 2/5] usb: kbd: On a "usb reset" call usb_kbd_deregister() before calling usb_stop()

[Hans de Goede]

Hi,

On 09/23/2014 02:15 PM, Marek Vasut wrote:
> On Tuesday, September 23, 2014 at 11:10:34 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 09/22/2014 02:01 PM, Marek Vasut wrote:
>>> On Saturday, September 20, 2014 at 04:54:35 PM, Hans de Goede wrote:
>>>> We need to call usb_kbd_deregister() before calling usb_stop().
>>>>
>>>> usbkbd's stdio_dev->priv points to the usb_device, and usb_kbd_testc
>>>> dereferences usb_device->privptr.
>>>>
>>>> usb_stop zeros usb_device, leaving usb_device->privptr NULL, causing
>>>> bad things (tm) to happen once control returns to the main loop and
>>>> usb_kbd_testc gets called.
>>>>
>>>> Calling usb_kbd_deregister() avoids this. Note that we do not allow
>>>> the "usb reset" to continue when the deregister fails. This will be
>>>> fixed in a later patch.
>>>>
>>>> For the same reasons always fail "usb stop" if the usb_kbd_deregister()
>>>> fails, even in the force path. This can happen when
>>>> CONFIG_SYS_STDIO_DEREGISTER is not set.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>
>>>>  common/cmd_usb.c | 27 +++++++++++++++------------
>>>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
>>>> index 2519497..b2aa44c 100644
>>>> --- a/common/cmd_usb.c
>>>> +++ b/common/cmd_usb.c
>>>> @@ -430,6 +430,16 @@ static int do_usbboot(cmd_tbl_t *cmdtp, int flag,
>>>> int argc, char * const argv[]) }
>>>>
>>>>  #endif /* CONFIG_USB_STORAGE */
>>>>
>>>> +static int do_usb_stop_keyboard(void)
>>>> +{
>>>> +#ifdef CONFIG_USB_KEYBOARD
>>>> +	if (usb_kbd_deregister() != 0) {
>>>> +		printf("USB not stopped: usbkbd still using USB\n");
>>>
>>> I tried this set of patches with the DWC2 driver on RPI just now and i
>>> get this message and 'usb start' fails and doesn't scan bus. I suspect
>>> we have a problem here, can you check and post a fix ?
>>
>> Do you have the entire set applied ?
> 
> Yes, the current u-boot-usb/master
> 
>> This is a known regression as
>> mentioned in the commit msg. Once the force parameter is in place this
>> should go away for a start/reset.
> 
> You mean the 'force' param ?

Yes, when that is 1, which it is on a start / reset, the deregister should never fail.

>> Also is usb start being called twice ? The first deregister should never
>> fail, since then no device is registered.
> 
> Ugh, I must be dumb or somesuch, for whatever reason the issue went away and I 
> can no longer replicate it. I will keep on a lookout.

Ok.

Regards,

Hans

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Rommel Custodio]

Dear Hans de Goede,

Typographical error here:

> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
>  <at>  <at>  -197,7 +197,7  <at>  <at>  static int serial_pre_remove(struct 
udevice *dev)
>  #ifdef CONFIG_SYS_STDIO_DEREGISTER
>  	struct serial_dev_priv *upriv = dev->uclass_priv;
> 
> -	if (stdio_deregister_dev(upriv->sdev))
> +	if (stdio_deregister_dev(upriv->sdev), 0)

Breaks sandbox build, and probably others.


All the best,
RgC


Hans de Goede <hdegoede <at> redhat.com> writes:

> 
> In some cases we really want to move forward with a deregister, add a force
> parameter to allow this, and replace the dev with a nulldev in this case.
> 
> Signed-off-by: Hans de Goede <hdegoede <at> redhat.com>

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Simon Glass]

Hi,

On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
> In some cases we really want to move forward with a deregister, add a force
> parameter to allow this, and replace the dev with a nulldev in this case.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  common/stdio.c                 | 13 ++++++++++---
>  common/usb_kbd.c               |  2 +-
>  drivers/serial/serial-uclass.c |  2 +-
>  include/stdio_dev.h            |  4 ++--
>  4 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/common/stdio.c b/common/stdio.c
> index c878103..8232815 100644
> --- a/common/stdio.c
> +++ b/common/stdio.c
> @@ -34,6 +34,9 @@ char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" };
>  #define        CONFIG_SYS_DEVICE_NULLDEV       1
>  #endif
>
> +#ifdef CONFIG_SYS_STDIO_DEREGISTER
> +#define        CONFIG_SYS_DEVICE_NULLDEV       1
> +#endif
>
>  #ifdef CONFIG_SYS_DEVICE_NULLDEV
>  void nulldev_putc(struct stdio_dev *dev, const char c)
> @@ -172,7 +175,7 @@ int stdio_register(struct stdio_dev *dev)
>   * returns 0 if success, -1 if device is assigned and 1 if devname not found
>   */
>  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> -int stdio_deregister_dev(struct stdio_dev *dev)
> +int stdio_deregister_dev(struct stdio_dev *dev, int force)
>  {
>         int l;
>         struct list_head *pos;
> @@ -181,6 +184,10 @@ int stdio_deregister_dev(struct stdio_dev *dev)
>         /* get stdio devices (ListRemoveItem changes the dev list) */
>         for (l=0 ; l< MAX_FILES; l++) {
>                 if (stdio_devices[l] == dev) {
> +                       if (force) {
> +                               strcpy(temp_names[l], "nulldev");
> +                               continue;
> +                       }
>                         /* Device is assigned -> report error */
>                         return -1;
>                 }
> @@ -202,7 +209,7 @@ int stdio_deregister_dev(struct stdio_dev *dev)
>         return 0;
>  }
>
> -int stdio_deregister(const char *devname)
> +int stdio_deregister(const char *devname, int force)
>  {
>         struct stdio_dev *dev;
>
> @@ -211,7 +218,7 @@ int stdio_deregister(const char *devname)
>         if (!dev) /* device not found */
>                 return -ENODEV;
>
> -       return stdio_deregister_dev(dev);
> +       return stdio_deregister_dev(dev, force);
>  }
>  #endif /* CONFIG_SYS_STDIO_DEREGISTER */
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index d4d5f48..dcb693d 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -550,7 +550,7 @@ int drv_usb_kbd_init(void)
>  int usb_kbd_deregister(void)
>  {
>  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> -       int ret = stdio_deregister(DEVNAME);
> +       int ret = stdio_deregister(DEVNAME, 0);
>         if (ret && ret != -ENODEV)
>                 return ret;
>
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index d04104e..61cbdc6 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
>  #ifdef CONFIG_SYS_STDIO_DEREGISTER
>         struct serial_dev_priv *upriv = dev->uclass_priv;
>
> -       if (stdio_deregister_dev(upriv->sdev))
> +       if (stdio_deregister_dev(upriv->sdev), 0)

That bracket seems to be in a strange place.

>                 return -EPERM;
>  #endif
>
> diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> index 268de8e..24da23f 100644
> --- a/include/stdio_dev.h
> +++ b/include/stdio_dev.h
> @@ -103,8 +103,8 @@ int stdio_init(void);
>
>  void   stdio_print_current_devices(void);
>  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> -int    stdio_deregister(const char *devname);
> -int stdio_deregister_dev(struct stdio_dev *dev);
> +int stdio_deregister(const char *devname, int force);
> +int stdio_deregister_dev(struct stdio_dev *dev, int force);
>  #endif
>  struct list_head* stdio_get_list(void);
>  struct stdio_dev* stdio_get_by_name(const char* name);
> --
> 2.1.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Marek Vasut]

On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
> Hi,
> 
> On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
> > In some cases we really want to move forward with a deregister, add a
> > force parameter to allow this, and replace the dev with a nulldev in
> > this case.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>

[...]

> > diff --git a/drivers/serial/serial-uclass.c
> > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
> > --- a/drivers/serial/serial-uclass.c
> > +++ b/drivers/serial/serial-uclass.c
> > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
> > 
> >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> >  
> >         struct serial_dev_priv *upriv = dev->uclass_priv;
> > 
> > -       if (stdio_deregister_dev(upriv->sdev))
> > +       if (stdio_deregister_dev(upriv->sdev), 0)
> 
> That bracket seems to be in a strange place.

Good find, thanks! I have two questions:
1) How come I did not notice this and my build didn't spit?
2) Can either of you guys please prepare a patch?

Best regards,
Marek Vasut

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Simon Glass]

Hi Marek,

On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote:
>
> On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
> > Hi,
> >
> > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
> > > In some cases we really want to move forward with a deregister, add a
> > > force parameter to allow this, and replace the dev with a nulldev in
> > > this case.
> > >
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> [...]
>
> > > diff --git a/drivers/serial/serial-uclass.c
> > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
> > > --- a/drivers/serial/serial-uclass.c
> > > +++ b/drivers/serial/serial-uclass.c
> > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
> > >
> > >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> > >
> > >         struct serial_dev_priv *upriv = dev->uclass_priv;
> > >
> > > -       if (stdio_deregister_dev(upriv->sdev))
> > > +       if (stdio_deregister_dev(upriv->sdev), 0)
> >
> > That bracket seems to be in a strange place.
>
> Good find, thanks! I have two questions:
> 1) How come I did not notice this and my build didn't spit?

If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and
CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has
all of these but it might be the only board.

>
> 2) Can either of you guys please prepare a patch?
>
> Best regards,
> Marek Vasut

Regards,
Simon

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Tom Rini]

On Thu, Oct 09, 2014 at 05:12:03PM +0200, Marek Vasut wrote:
> On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
> > Hi,
> > 
> > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
> > > In some cases we really want to move forward with a deregister, add a
> > > force parameter to allow this, and replace the dev with a nulldev in
> > > this case.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> [...]
> 
> > > diff --git a/drivers/serial/serial-uclass.c
> > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
> > > --- a/drivers/serial/serial-uclass.c
> > > +++ b/drivers/serial/serial-uclass.c
> > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
> > > 
> > >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> > >  
> > >         struct serial_dev_priv *upriv = dev->uclass_priv;
> > > 
> > > -       if (stdio_deregister_dev(upriv->sdev))
> > > +       if (stdio_deregister_dev(upriv->sdev), 0)
> > 
> > That bracket seems to be in a strange place.
> 
> Good find, thanks! I have two questions:
> 1) How come I did not notice this and my build didn't spit?

Because only sandbox uses this right now I think.  But I'm unhappy that
I can't seem to make repeated runs of:
$ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \
'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc'
$ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \
'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' \
-svel

Show me just new problems from the last time I did it.  I must be doing
something wrong, Simon?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141009/0c76d601/attachment.pgp>

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Marek Vasut]

On Thursday, October 09, 2014 at 06:14:11 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
> > > Hi,
> > > 
> > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
> > > > In some cases we really want to move forward with a deregister, add a
> > > > force parameter to allow this, and replace the dev with a nulldev in
> > > > this case.
> > > > 
> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > [...]
> > 
> > > > diff --git a/drivers/serial/serial-uclass.c
> > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
> > > > --- a/drivers/serial/serial-uclass.c
> > > > +++ b/drivers/serial/serial-uclass.c
> > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
> > > > 
> > > >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> > > >  
> > > >         struct serial_dev_priv *upriv = dev->uclass_priv;
> > > > 
> > > > -       if (stdio_deregister_dev(upriv->sdev))
> > > > +       if (stdio_deregister_dev(upriv->sdev), 0)
> > > 
> > > That bracket seems to be in a strange place.
> > 
> > Good find, thanks! I have two questions:
> > 1) How come I did not notice this and my build didn't spit?
> 
> If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and
> CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has
> all of these but it might be the only board.

I see, error on my end then. I will start building sandbox for the USB tree.
Thank you for pointing this out! This also stresses my point that U-Boot project
does need a proper CI (which we could have had thanks to Vadim, but we didn't
persudate that, dang again).

I think this CI stuff should be added to the agenda of the U-Boot minisummit 
discussion.

Another point to the CI discussion could be that we could prepare a docker image
with all the toolchains preinstalled, so one can run buildman easily in a well 
defined environment on his/her own.

Best regards,
Marek Vasut

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Simon Glass]

Hi Tom,

On 9 October 2014 10:23, Tom Rini <trini@ti.com> wrote:
> On Thu, Oct 09, 2014 at 05:12:03PM +0200, Marek Vasut wrote:
>> On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
>> > Hi,
>> >
>> > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
>> > > In some cases we really want to move forward with a deregister, add a
>> > > force parameter to allow this, and replace the dev with a nulldev in
>> > > this case.
>> > >
>> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> [...]
>>
>> > > diff --git a/drivers/serial/serial-uclass.c
>> > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
>> > > --- a/drivers/serial/serial-uclass.c
>> > > +++ b/drivers/serial/serial-uclass.c
>> > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
>> > >
>> > >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
>> > >
>> > >         struct serial_dev_priv *upriv = dev->uclass_priv;
>> > >
>> > > -       if (stdio_deregister_dev(upriv->sdev))
>> > > +       if (stdio_deregister_dev(upriv->sdev), 0)
>> >
>> > That bracket seems to be in a strange place.
>>
>> Good find, thanks! I have two questions:
>> 1) How come I did not notice this and my build didn't spit?
>
> Because only sandbox uses this right now I think.  But I'm unhappy that
> I can't seem to make repeated runs of:
> $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \
> 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc'
> $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \
> 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' \
> -svel
>
> Show me just new problems from the last time I did it.  I must be doing
> something wrong, Simon?

I don't really see anything obviously wrong. But I'm not sure what you
are expecting. This is going to just build the top commit in branch
master, and if the commit hash has changed it will remove any previous
results for that commit before starting the build. So you will always
get a full list of errors, not a delta from last time. If you want
that you could add a second commit with your fixes and not adjust the
first commit in the branch (and use -c2).

If you leave off '-b master -c1' it would have about the same effect,
assuming that you have 'master' checked out.

In the second line you are specifying -ve twice but that doesn't
matter. Why are you changing the thread/jobs defaults? Does that speed
it up? Also you don't need the quotes and the | between archs.

Also note there is --step to avoid building every commit. For example,
this will build the upstream commit and your branch's top commit
(only) assuming that master tracks upstream/master.

buildman -b master --step 0

Regards,
Simon

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Simon Glass]

Hi Marek,

On 9 October 2014 10:27, Marek Vasut <marex@denx.de> wrote:
> On Thursday, October 09, 2014 at 06:14:11 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote:
>> > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
>> > > Hi,
>> > >
>> > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
>> > > > In some cases we really want to move forward with a deregister, add a
>> > > > force parameter to allow this, and replace the dev with a nulldev in
>> > > > this case.
>> > > >
>> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> >
>> > [...]
>> >
>> > > > diff --git a/drivers/serial/serial-uclass.c
>> > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
>> > > > --- a/drivers/serial/serial-uclass.c
>> > > > +++ b/drivers/serial/serial-uclass.c
>> > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
>> > > >
>> > > >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
>> > > >
>> > > >         struct serial_dev_priv *upriv = dev->uclass_priv;
>> > > >
>> > > > -       if (stdio_deregister_dev(upriv->sdev))
>> > > > +       if (stdio_deregister_dev(upriv->sdev), 0)
>> > >
>> > > That bracket seems to be in a strange place.
>> >
>> > Good find, thanks! I have two questions:
>> > 1) How come I did not notice this and my build didn't spit?
>>
>> If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and
>> CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has
>> all of these but it might be the only board.
>
> I see, error on my end then. I will start building sandbox for the USB tree.
> Thank you for pointing this out! This also stresses my point that U-Boot project
> does need a proper CI (which we could have had thanks to Vadim, but we didn't
> persudate that, dang again).

What is a Cl? Do you mean his gerrit code review stuff?

>
> I think this CI stuff should be added to the agenda of the U-Boot minisummit
> discussion.
>
> Another point to the CI discussion could be that we could prepare a docker image
> with all the toolchains preinstalled, so one can run buildman easily in a well
> defined environment on his/her own.
>
> Best regards,
> Marek Vasut

Regards,
Simon

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Tom Rini]

On Thu, Oct 09, 2014 at 11:03:02AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On 9 October 2014 10:23, Tom Rini <trini@ti.com> wrote:
> > On Thu, Oct 09, 2014 at 05:12:03PM +0200, Marek Vasut wrote:
> >> On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
> >> > Hi,
> >> >
> >> > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
> >> > > In some cases we really want to move forward with a deregister, add a
> >> > > force parameter to allow this, and replace the dev with a nulldev in
> >> > > this case.
> >> > >
> >> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>
> >> [...]
> >>
> >> > > diff --git a/drivers/serial/serial-uclass.c
> >> > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
> >> > > --- a/drivers/serial/serial-uclass.c
> >> > > +++ b/drivers/serial/serial-uclass.c
> >> > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev)
> >> > >
> >> > >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> >> > >
> >> > >         struct serial_dev_priv *upriv = dev->uclass_priv;
> >> > >
> >> > > -       if (stdio_deregister_dev(upriv->sdev))
> >> > > +       if (stdio_deregister_dev(upriv->sdev), 0)
> >> >
> >> > That bracket seems to be in a strange place.
> >>
> >> Good find, thanks! I have two questions:
> >> 1) How come I did not notice this and my build didn't spit?
> >
> > Because only sandbox uses this right now I think.  But I'm unhappy that
> > I can't seem to make repeated runs of:
> > $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \
> > 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc'
> > $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \
> > 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' \
> > -svel
> >
> > Show me just new problems from the last time I did it.  I must be doing
> > something wrong, Simon?
> 
> I don't really see anything obviously wrong. But I'm not sure what you
> are expecting. This is going to just build the top commit in branch
> master, and if the commit hash has changed it will remove any previous
> results for that commit before starting the build. So you will always
> get a full list of errors, not a delta from last time. If you want
> that you could add a second commit with your fixes and not adjust the
> first commit in the branch (and use -c2).

Ah, OK, this got me going towards what I wanted.  I was assuming for
some incorrect reason that with -c1 it would just implicitly compare vs
the last build it had available.  I really want -c2 as I should have a
full build from the last go-round (which is to say really, last merge).
This is what I wanted (a simplified build):
trini at bill-the-cat:~/work/u-boot/u-boot (32d0192...)$
./tools/buildman/buildman -b HEAD -c 2 -ve -T 1 -j 9 'sandbox|sparc'
-svel
boards.cfg is up to date. Nothing to do.
Summary of 2 commits for 6 boards (1 thread, 9 jobs per thread)
01: usb: kbd: Remove check for already being registered
sparc: +   grsim_leon2 gr_cpci_ax2000 gr_xc3s_1500 grsim gr_ep2s60
+(grsim_leon2,gr_cpci_ax2000,gr_xc3s_1500,grsim,gr_ep2s60)
disk/part.c: In function `get_device':
+(grsim_leon2,gr_cpci_ax2000,gr_xc3s_1500,grsim,gr_ep2s60)
disk/part.c:454: warning: 'hwpart' might be used uninitialized in
this function
02: stdio: Add force parameter to stdio_deregister
sandbox: +   sandbox
+(sandbox) drivers/serial/serial-uclass.c: In function
?serial_pre_remove?:
+(sandbox) drivers/serial/serial-uclass.c:201:2: error: too few
arguments to function ?stdio_deregister_dev?
+(sandbox) include/stdio_dev.h:107:5: note: declared here
+(sandbox) make[2]: *** [drivers/serial/serial-uclass.o] Error 1
+(sandbox) make[1]: *** [drivers/serial] Error 2
+(sandbox) make: *** [sub-make] Error 2	
w+(sandbox) drivers/serial/serial-uclass.c:201:39: warning:
left-hand operand of comma expression has no effect
[-Wunused-value]

And I don't care about the stuff around 01 (it was like that before), I
care about 02 which is new problems.

> If you leave off '-b master -c1' it would have about the same effect,
> assuming that you have 'master' checked out.
> 
> In the second line you are specifying -ve twice but that doesn't
> matter. Why are you changing the thread/jobs defaults? Does that speed
> it up?

We have some race condition still on very large machines that I haven't
been able to track down.  Doing it that way was less problematic I
think.

> Also you don't need the quotes and the | between archs.

Oh, interesting.

> Also note there is --step to avoid building every commit. For example,
> this will build the upstream commit and your branch's top commit
> (only) assuming that master tracks upstream/master.
> 
> buildman -b master --step 0

I think this is what I really wanted, yes.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141009/bfd496c8/attachment.pgp>

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Marek Vasut]

On Thursday, October 09, 2014 at 07:03:42 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 9 October 2014 10:27, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, October 09, 2014 at 06:14:11 PM, Simon Glass wrote:
> >> Hi Marek,
> >> 
> >> On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote:
> >> > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
> >> > > Hi,
> >> > > 
> >> > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote:
> >> > > > In some cases we really want to move forward with a deregister,
> >> > > > add a force parameter to allow this, and replace the dev with a
> >> > > > nulldev in this case.
> >> > > > 
> >> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> > 
> >> > [...]
> >> > 
> >> > > > diff --git a/drivers/serial/serial-uclass.c
> >> > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
> >> > > > --- a/drivers/serial/serial-uclass.c
> >> > > > +++ b/drivers/serial/serial-uclass.c
> >> > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice
> >> > > > *dev)
> >> > > > 
> >> > > >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> >> > > >  
> >> > > >         struct serial_dev_priv *upriv = dev->uclass_priv;
> >> > > > 
> >> > > > -       if (stdio_deregister_dev(upriv->sdev))
> >> > > > +       if (stdio_deregister_dev(upriv->sdev), 0)
> >> > > 
> >> > > That bracket seems to be in a strange place.
> >> > 
> >> > Good find, thanks! I have two questions:
> >> > 1) How come I did not notice this and my build didn't spit?
> >> 
> >> If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and
> >> CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has
> >> all of these but it might be the only board.
> > 
> > I see, error on my end then. I will start building sandbox for the USB
> > tree. Thank you for pointing this out! This also stresses my point that
> > U-Boot project does need a proper CI (which we could have had thanks to
> > Vadim, but we didn't persudate that, dang again).
> 
> What is a Cl? Do you mean his gerrit code review stuff?

I mean more continuous integration (build testing) of the code before a PR
is submitted to the ML. Right now, we all do our own thing when it comes to
testing before PR, but it would be nice to have one easy way of doing the
build testing before submitting the PR, don't you think ? This might apply
to Linux too.

Best regards,
Marek Vasut

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Simon Glass]

Hi Marek,

On 9 October 2014 11:32, Marek Vasut <marex@denx.de> wrote:

> On Thursday, October 09, 2014 at 07:03:42 PM, Simon Glass wrote:
> > Hi Marek,
> >
> > On 9 October 2014 10:27, Marek Vasut <marex@denx.de> wrote:
> > > On Thursday, October 09, 2014 at 06:14:11 PM, Simon Glass wrote:
> > >> Hi Marek,
> > >>
> > >> On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote:
> > >> > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote:
> > >> > > Hi,
> > >> > >
> > >> > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com>
> wrote:
> > >> > > > In some cases we really want to move forward with a deregister,
> > >> > > > add a force parameter to allow this, and replace the dev with a
> > >> > > > nulldev in this case.
> > >> > > >
> > >> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >> >
> > >> > [...]
> > >> >
> > >> > > > diff --git a/drivers/serial/serial-uclass.c
> > >> > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644
> > >> > > > --- a/drivers/serial/serial-uclass.c
> > >> > > > +++ b/drivers/serial/serial-uclass.c
> > >> > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice
> > >> > > > *dev)
> > >> > > >
> > >> > > >  #ifdef CONFIG_SYS_STDIO_DEREGISTER
> > >> > > >
> > >> > > >         struct serial_dev_priv *upriv = dev->uclass_priv;
> > >> > > >
> > >> > > > -       if (stdio_deregister_dev(upriv->sdev))
> > >> > > > +       if (stdio_deregister_dev(upriv->sdev), 0)
> > >> > >
> > >> > > That bracket seems to be in a strange place.
> > >> >
> > >> > Good find, thanks! I have two questions:
> > >> > 1) How come I did not notice this and my build didn't spit?
> > >>
> > >> If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and
> > >> CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has
> > >> all of these but it might be the only board.
> > >
> > > I see, error on my end then. I will start building sandbox for the USB
> > > tree. Thank you for pointing this out! This also stresses my point that
> > > U-Boot project does need a proper CI (which we could have had thanks to
> > > Vadim, but we didn't persudate that, dang again).
> >
> > What is a Cl? Do you mean his gerrit code review stuff?
>
> I mean more continuous integration (build testing) of the code before a PR
> is submitted to the ML. Right now, we all do our own thing when it comes to
> testing before PR, but it would be nice to have one easy way of doing the
> build testing before submitting the PR, don't you think ? This might apply
> to Linux too.
>

Sure it would be useful. Before submitting my pull request I get all the
patches in a branch and run:

./tools/buildman/buildman -b x86-push

This checks every commit for every board that I build, and gives me good
confidence that no patch introduces new breakages.

Regards,
Simon

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Marek Vasut]

On Thursday, October 09, 2014 at 08:04:29 PM, Simon Glass wrote:
> Hi Marek,

Hi Simon,

[..]

> > I mean more continuous integration (build testing) of the code before a
> > PR is submitted to the ML. Right now, we all do our own thing when it
> > comes to testing before PR, but it would be nice to have one easy way of
> > doing the build testing before submitting the PR, don't you think ? This
> > might apply to Linux too.
> 
> Sure it would be useful. Before submitting my pull request I get all the
> patches in a branch and run:
> 
> ./tools/buildman/buildman -b x86-push
> 
> This checks every commit for every board that I build, and gives me good
> confidence that no patch introduces new breakages.

I agree that buildman solves the CI part nicely, but we also have the part
where one has to install the myriad of toolchains for all the architectures
to be able to do the compile testing. I wonder if this cannot be made easy
in some way -- maybe through a re-usable docker image with all the parts in
it already.

Best regards,
Marek Vasut

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Simon Glass]

Hi Marek,

On 9 October 2014 17:59, Marek Vasut <marex@denx.de> wrote:
> On Thursday, October 09, 2014 at 08:04:29 PM, Simon Glass wrote:
>> Hi Marek,
>
> Hi Simon,
>
> [..]
>
>> > I mean more continuous integration (build testing) of the code before a
>> > PR is submitted to the ML. Right now, we all do our own thing when it
>> > comes to testing before PR, but it would be nice to have one easy way of
>> > doing the build testing before submitting the PR, don't you think ? This
>> > might apply to Linux too.
>>
>> Sure it would be useful. Before submitting my pull request I get all the
>> patches in a branch and run:
>>
>> ./tools/buildman/buildman -b x86-push
>>
>> This checks every commit for every board that I build, and gives me good
>> confidence that no patch introduces new breakages.
>
> I agree that buildman solves the CI part nicely, but we also have the part
> where one has to install the myriad of toolchains for all the architectures
> to be able to do the compile testing. I wonder if this cannot be made easy
> in some way -- maybe through a re-usable docker image with all the parts in
> it already.

It would be create if we could download all the toolchains from one
place. Maybe we need a toolchain maintainer?

Regards,
Simon

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Fabio Estevam]

Hi Marek,

On Thu, Oct 9, 2014 at 8:59 PM, Marek Vasut <marex@denx.de> wrote:

> I agree that buildman solves the CI part nicely, but we also have the part
> where one has to install the myriad of toolchains for all the architectures
> to be able to do the compile testing. I wonder if this cannot be made easy
> in some way -- maybe through a re-usable docker image with all the parts in
> it already.

Maybe you could take a look at the make.cross script kbuild robot uses
for cross compiling for lots of different architectures:
https://lists.01.org/pipermail/kbuild-all/2014-October/006965.html

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Simon Glass]

Hi,

On 9 October 2014 20:06, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Marek,
>
> On Thu, Oct 9, 2014 at 8:59 PM, Marek Vasut <marex@denx.de> wrote:
>
>> I agree that buildman solves the CI part nicely, but we also have the part
>> where one has to install the myriad of toolchains for all the architectures
>> to be able to do the compile testing. I wonder if this cannot be made easy
>> in some way -- maybe through a re-usable docker image with all the parts in
>> it already.
>
> Maybe you could take a look at the make.cross script kbuild robot uses
> for cross compiling for lots of different architectures:
> https://lists.01.org/pipermail/kbuild-all/2014-October/006965.html

It's not really the builder that is the problem - we have one. It's
all the different toolchains that no one knows about. I still can't
build all the boards successfully.

Regards,
Simon

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Fabio Estevam]

On Thu, Oct 9, 2014 at 11:07 PM, Simon Glass <sjg@chromium.org> wrote:

> It's not really the builder that is the problem - we have one. It's
> all the different toolchains that no one knows about. I still can't
> build all the boards successfully.

Aren't all the toolchains U-boot need included in the make.cross script?

https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross

It allows to build for all the archs that the kernel support.

Regards,

Fabio Estevam

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Marek Vasut]

On Friday, October 10, 2014 at 04:00:00 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 9 October 2014 17:59, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, October 09, 2014 at 08:04:29 PM, Simon Glass wrote:
> >> Hi Marek,
> > 
> > Hi Simon,
> > 
> > [..]
> > 
> >> > I mean more continuous integration (build testing) of the code before
> >> > a PR is submitted to the ML. Right now, we all do our own thing when
> >> > it comes to testing before PR, but it would be nice to have one easy
> >> > way of doing the build testing before submitting the PR, don't you
> >> > think ? This might apply to Linux too.
> >> 
> >> Sure it would be useful. Before submitting my pull request I get all the
> >> patches in a branch and run:
> >> 
> >> ./tools/buildman/buildman -b x86-push
> >> 
> >> This checks every commit for every board that I build, and gives me good
> >> confidence that no patch introduces new breakages.
> > 
> > I agree that buildman solves the CI part nicely, but we also have the
> > part where one has to install the myriad of toolchains for all the
> > architectures to be able to do the compile testing. I wonder if this
> > cannot be made easy in some way -- maybe through a re-usable docker
> > image with all the parts in it already.
> 
> It would be create if we could download all the toolchains from one
> place. Maybe we need a toolchain maintainer?

What about [1], this is where we can source the more exotic toolchains from,
can we not? I think it was even you who pointed me to this site and it really
is a nice one ;-)

[1] https://www.kernel.org/pub/tools/crosstool/

Best regards,
Marek Vasut

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Fabio Estevam]

On Thu, Oct 9, 2014 at 11:23 PM, Marek Vasut <marex@denx.de> wrote:

> What about [1], this is where we can source the more exotic toolchains from,
> can we not? I think it was even you who pointed me to this site and it really
> is a nice one ;-)
>
> [1] https://www.kernel.org/pub/tools/crosstool/

Exactly, the https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
script pulls the toolchains from this same location ;-)

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Simon Glass]

Hi,

On 9 October 2014 20:26, Fabio Estevam <festevam@gmail.com> wrote:
> On Thu, Oct 9, 2014 at 11:23 PM, Marek Vasut <marex@denx.de> wrote:
>
>> What about [1], this is where we can source the more exotic toolchains from,
>> can we not? I think it was even you who pointed me to this site and it really
>> is a nice one ;-)
>>
>> [1] https://www.kernel.org/pub/tools/crosstool/
>
> Exactly, the https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
> script pulls the toolchains from this same location ;-)

OK I see. So all we need is someone to add an option to buildman to
pull them down and put them somewhere?

Regards,
Simon

[U-Boot] [PATCH fix for v2014.10 4/5] stdio: Add force parameter to stdio_deregister

[Marek Vasut]

On Friday, October 10, 2014 at 04:35:11 AM, Simon Glass wrote:
> Hi,

Hi,

> On 9 October 2014 20:26, Fabio Estevam <festevam@gmail.com> wrote:
> > On Thu, Oct 9, 2014 at 11:23 PM, Marek Vasut <marex@denx.de> wrote:
> >> What about [1], this is where we can source the more exotic toolchains
> >> from, can we not? I think it was even you who pointed me to this site
> >> and it really is a nice one ;-)
> >> 
> >> [1] https://www.kernel.org/pub/tools/crosstool/
> > 
> > Exactly, the
> > https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbi
> > n/make.cross script pulls the toolchains from this same location ;-)
> 
> OK I see. So all we need is someone to add an option to buildman to
> pull them down and put them somewhere?

That might just work.

Best regards,
Marek Vasut