[U-Boot] [PATCH fix for v2014.10 0/5] USB keyboard: don't crash on "usb reset"
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
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
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;
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
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
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
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
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));
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);
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>
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Regards, Simon
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
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:
- 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.
- Can either of you guys please prepare a patch?
Best regards, Marek Vasut
Regards, Simon
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:
- 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
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:
- 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
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:
- 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
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:
- 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
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
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
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
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 ;-)
Exactly, the https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/ma... script pulls the toolchains from this same location ;-)
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 ;-)
Exactly, the https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/ma... 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
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 ;-)
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
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
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
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/ma...
It allows to build for all the archs that the kernel support.
Regards,
Fabio Estevam
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:
- 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?
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:
- 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
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:
- 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@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.
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 */
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
participants (6)
-
Fabio Estevam -
Hans de Goede -
Marek Vasut -
Rommel Custodio -
Simon Glass -
Tom Rini