Fix repeated USB scans problems

PATCH New Hans de Goede 3 participants 14 messages Jan 6, 2015 – Jan 10, 2015

lore

Share & tools

lore

Hans de Goede

author

2015-01-06 13:27 UTC #0 lore raw

cover Fix repeated USB scans problems

Hi Marek & Stephen,

As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.

Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).

Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.

Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.

Regards,

Hans

Hans de Goede

author

2015-01-06 13:27 UTC #1 lore raw

PATCH 1/2 USB: make "usb start" start usb only once

Currently we've this magic in include/config_distro_bootcmd.h to avoid scanning the usb bus multiple times.

And it does not work when also using an usb keyboard because then the preboot command has already scanned the bus, so we're still scanning it twice.

This commit makes "usb start" only start usb if it is no already started, allowing us to remove all the magic for it from include/config_distro_bootcmd.h and just call it unconditionally.

This also causes "usb start" and "usb reset" to actually do what their different names suggest, rather then both of them doing exactly the same.

Signed-off-by Hans de Goede <[email protected]>

---
 common/cmd_usb.c | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

common/cmd_usb.c +31 ~~-17~~

index c192498..27813f0 100644
@@ -441,6 +441,26 @@ static int do_usb_stop_keyboard(int force)
 	return 0;
 }
 
+static void do_usb_start(void)
+{
+	bootstage_mark_name(BOOTSTAGE_ID_USB_START, "usb_start");
+
+	if (usb_init() < 0)
+		return;
+
+#ifdef CONFIG_USB_STORAGE
+	/* try to recognize storage devices immediately */
+	usb_stor_curr_dev = usb_stor_scan(1);
+#endif
+#ifdef CONFIG_USB_HOST_ETHER
+	/* try to recognize ethernet devices immediately */
+	usb_ether_curr_dev = usb_host_eth_scan(1);
+#endif
+#ifdef CONFIG_USB_KEYBOARD
+	drv_usb_kbd_init();
+#endif
+}
+
 /******************************************************************************
  * usb command intepreter
  */

@@ -457,26 +477,20 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	if ((strncmp(argv[1], "reset", 5) == 0) ||
-		 (strncmp(argv[1], "start", 5) == 0)) {
-		bootstage_mark_name(BOOTSTAGE_ID_USB_START, "usb_start");
+	if (strncmp(argv[1], "start", 5) == 0) {
+		if (usb_started)
+			return 0; /* Already started */
+		printf("starting USB...\n");
+		do_usb_start();
+		return 0;
+	}
+
+	if (strncmp(argv[1], "reset", 5) == 0) {
+		printf("resetting USB...\n");
 		if (do_usb_stop_keyboard(1) != 0)
 			return 1;
 		usb_stop();
-		printf("(Re)start USB...\n");
-		if (usb_init() >= 0) {
-#ifdef CONFIG_USB_STORAGE
-			/* try to recognize storage devices immediately */
-			usb_stor_curr_dev = usb_stor_scan(1);
-#endif
-#ifdef CONFIG_USB_HOST_ETHER
-			/* try to recognize ethernet devices immediately */
-			usb_ether_curr_dev = usb_host_eth_scan(1);
-#endif
-#ifdef CONFIG_USB_KEYBOARD
-			drv_usb_kbd_init();
-#endif
-		}
+		do_usb_start();
 		return 0;
 	}
 	if (strncmp(argv[1], "stop", 4) == 0) {

Signature · 1 line

2.1.0

Hans de Goede

author

2015-01-06 13:27 UTC #2 lore raw

PATCH 2/2 config_distro_bootcmd.h: Remove unnecessary magic to avoid repeated USB scans

In reply to Hans de Goede

Now that "usb start" will only start usb if not already started, we can simply call "usb start" whenever we (may) need access to usb devices, and it will only actually scan the bus at the first call.

Signed-off-by Hans de Goede <[email protected]>

---
 include/config_distro_bootcmd.h | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

include/config_distro_bootcmd.h +2 ~~-10~~

index be616e8..becbe3f 100644
@@ -90,15 +90,8 @@
 #endif
 
 #ifdef CONFIG_CMD_USB
-#define BOOTENV_RUN_USB_INIT "run usb_init; "
-#define BOOTENV_SET_USB_NEED_INIT "setenv usb_need_init; "
+#define BOOTENV_RUN_USB_INIT "usb start; "
 #define BOOTENV_SHARED_USB \
-	"usb_init=" \
-		"if ${usb_need_init}; then " \
-			"setenv usb_need_init false; " \
-			"usb start 0; " \
-		"fi\0" \
-	\
 	"usb_boot=" \
 		BOOTENV_RUN_USB_INIT \
 		BOOTENV_SHARED_BLKDEV_BODY(usb)

@@ -106,7 +99,6 @@
 #define BOOTENV_DEV_NAME_USB	BOOTENV_DEV_NAME_BLKDEV
 #else
 #define BOOTENV_RUN_USB_INIT
-#define BOOTENV_SET_USB_NEED_INIT
 #define BOOTENV_SHARED_USB
 #define BOOTENV_DEV_USB \
 	BOOT_TARGET_DEVICES_references_USB_without_CONFIG_CMD_USB

@@ -202,7 +194,7 @@
 	\
 	BOOT_TARGET_DEVICES(BOOTENV_DEV)                                  \
 	\
-	"bootcmd=" BOOTENV_SET_USB_NEED_INIT BOOTENV_SET_SCSI_NEED_INIT   \
+	"bootcmd=" BOOTENV_SET_SCSI_NEED_INIT                             \
 		"for target in ${boot_targets}; do "                      \
 			"run bootcmd_${target}; "                         \
 		"done\0"

Signature · 1 line

2.1.0

Stephen Warren

2015-01-06 17:02 UTC #3 lore raw

cover Fix repeated USB scans problems

In reply to Hans de Goede

Quoted 9 lines · Hans de Goede

Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).

Quoted 7 lines

Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.

Users will have to get used to running "usb reset" rather than "usb start", but I guess that's fine.

Should we rename "usb reset" to "usb restart"?

Either way, the series,

Acked-by Stephen Warren <[email protected]>

Will you do something similar for SCSI?

Marek Vasut

2015-01-07 23:35 UTC #4 lore raw

cover Fix repeated USB scans problems

Quoted 14 lines · Stephen Warren

Quoted 13 lines · Hans de Goede
Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).
Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.

Quoted 6 lines

Quoted 3 lines
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.
Users will have to get used to running "usb reset" rather than "usb start", but I guess that's fine.

Hi!

So why do we not have "usb reset" only, why can we not discard the "usb start" altogether?

Quoted 1 line

Should we rename "usb reset" to "usb restart"?

No, let's not mess with the UI any more than we already did.

Quoted 4 lines

Either way, the series,
Acked-by Stephen Warren <[email protected]>
Will you do something similar for SCSI?

Best regards,
Marek Vasut

Stephen Warren

2015-01-08 00:19 UTC #5 lore raw

cover Fix repeated USB scans problems

Quoted 19 lines · Marek Vasut

Quoted 18 lines · Stephen Warren
Quoted 16 lines · Hans de Goede
Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).
Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.

Quoted 7 lines

Quoted 2 lines
Users will have to get used to running "usb reset" rather than "usb start", but I guess that's fine.
Hi!
So why do we not have "usb reset" only, why can we not discard the "usb start" altogether?

The whole point is that we need separate commands for:

* usb start: If USB isn't started, start it and scan the bus, else do nothing.

This is used by automatic scripts that want to ensure that USB is available, but not force bus rescans over and over.

* usb reset: If USB isn't started, start it. Always rescan the bus.

This is used by the user to force a rescan of the USB bus, if they know they've plugged in a new device.

Quoted 3 lines

Quoted 1 line
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.

Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?

Hans de Goede

author

2015-01-08 08:34 UTC #6 lore raw

cover Fix repeated USB scans problems

Hi,

Quoted 2 lines · Stephen Warren

Quoted 1 line · Marek Vasut
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:

<snip>

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?

Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...

Regards,

Hans

Stephen Warren

2015-01-08 16:16 UTC #7 lore raw

cover Fix repeated USB scans problems

Quoted 10 lines · Hans de Goede

Hi,
Quoted 2 lines · Stephen Warren
Quoted 1 line · Marek Vasut
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
<snip>
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
Should we rename "usb reset" to "usb restart"?

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...

Yes, that's true. So I suppose we should indeed not rename anything.

Marek Vasut

2015-01-08 17:06 UTC #8 lore raw

cover Fix repeated USB scans problems

Quoted 2 lines · Stephen Warren

Quoted 1 line · Hans de Goede
Hi,

Hi!

Quoted 9 lines

Quoted 9 lines · Stephen Warren
Quoted 2 lines · Marek Vasut
Quoted 1 line · Stephen Warren
<snip>
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.

OK, let's go with this approach now.

Best regards,
Marek Vasut

Hans de Goede

author

2015-01-08 17:23 UTC #9 lore raw

cover Fix repeated USB scans problems

Hi,

Quoted 18 lines · Marek Vasut

Quoted 2 lines · Stephen Warren
Quoted 1 line · Hans de Goede
Hi,
Hi!
Quoted 12 lines
Quoted 12 lines · Stephen Warren
Quoted 2 lines · Marek Vasut
Quoted 1 line · Stephen Warren
<snip>
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.
OK, let's go with this approach now.

So we are all in agreement, good, so through who's tree are these 2 patches going to go upstream ? Note I can take them upstream through the sunxi tree, but the usb tree seems better to me ...

Regards,

Hans

Marek Vasut

2015-01-08 17:36 UTC #10 lore raw

cover Fix repeated USB scans problems

Quoted 24 lines · Hans de Goede

Hi,
Quoted 21 lines · Marek Vasut
Quoted 2 lines · Stephen Warren
Quoted 1 line · Hans de Goede
Hi,
Hi!
Quoted 15 lines
Quoted 14 lines · Stephen Warren
Quoted 2 lines · Marek Vasut
Quoted 1 line · Stephen Warren
<snip>
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Yes, that's true. So I suppose we should indeed not rename anything.
OK, let's go with this approach now.
So we are all in agreement, good, so through who's tree are these 2 patches going to go upstream ? Note I can take them upstream through the sunxi tree, but the usb tree seems better to me ...

I can pick them, shall I pick them as they are ? Are they for -next or current ?

Best regards,
Marek Vasut

Hans de Goede

author

2015-01-08 17:40 UTC #11 lore raw

cover Fix repeated USB scans problems

Hi,

Quoted 27 lines · Marek Vasut

Quoted 26 lines · Hans de Goede
Hi,
Quoted 23 lines · Marek Vasut
Quoted 2 lines · Stephen Warren
Quoted 1 line · Hans de Goede
Hi,
Hi!
Quoted 16 lines
Quoted 7 lines · Stephen Warren
Quoted 2 lines · Marek Vasut
Quoted 1 line · Stephen Warren
<snip>
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
Should we rename "usb reset" to "usb restart"?
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
OK, let's go with this approach now.
So we are all in agreement, good, so through who's tree are these 2 patches going to go upstream ? Note I can take them upstream through the sunxi tree, but the usb tree seems better to me ...
I can pick them, shall I pick them as they are ?

Yes, AFAIK there were no requests for changes.

Quoted 1 line

Are they for -next or current ?

They are intended for -next, the double usb scan when using a usb keyboard *and* booting from usb is a nuisance, but not fatal, and very few people actually use the combo, so given that current is about to be released in a couple of days lets play it safe :)

Regards,

Hans

Marek Vasut

2015-01-10 00:49 UTC #12 lore raw

cover Fix repeated USB scans problems

Quoted 1 line · Hans de Goede

Hi,

Hi!

[...]

Quoted 13 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
OK, let's go with this approach now.
So we are all in agreement, good, so through who's tree are these 2 patches going to go upstream ? Note I can take them upstream through the sunxi tree, but the usb tree seems better to me ...
I can pick them, shall I pick them as they are ?

Quoted 8 lines

Yes, AFAIK there were no requests for changes.
Quoted 1 line
Are they for -next or current ?
They are intended for -next, the double usb scan when using a usb keyboard *and* booting from usb is a nuisance, but not fatal, and very few people actually use the combo, so given that current is about to be released in a couple of days lets play it safe :)

OK, applied both to next. Thanks!

Best regards,
Marek Vasut

Hans de Goede

author

2015-01-08 10:09 UTC #13 lore raw

cover Fix repeated USB scans problems

In reply to Stephen Warren

Hi,

Quoted 18 lines · Stephen Warren

Quoted 9 lines · Hans de Goede
Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).
Quoted 7 lines
Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.

Quoted 8 lines

Users will have to get used to running "usb reset" rather than "usb start", but I guess that's fine.
Should we rename "usb reset" to "usb restart"?
Either way, the series,
Acked-by Stephen Warren <[email protected]>
Will you do something similar for SCSI?

I think it would probably make sense to do something similar for SCSI, but I'm not all that familiar with the SCSI code, and I really don't have time to work on this for SCSI, so atm I do not plan to work that.

Regards,

Hans