| Message ID | 1374156931-1718-1-git-send-email-jilin@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Jim Lin, > TFTP booting is slow when a USB keyboard is installed and > stdin has usbkbd added. > This fix is to change Ctrl-C polling for USB keyboard to every second > when NET transfer is running. > > Signed-off-by: Jim Lin <jilin@nvidia.com> > --- > Changes in v2: > 1. Change configuration name from CONFIG_CTRLC_POLL_MS to > CONFIG_CTRLC_POLL_S. 2. New code will be executed only when > CONFIG_CTRLC_POLL_S is defined in configuration header file. > 3. Add description in README.console. > Changes in v3: > 1. Move changes to common/usb_kbd.c and doc/README.usb > 2. Rename config setting to CONFIG_USBKB_TESTC_PERIOD. > 3. Remove slow response on USB-keyboard input when TFTP boot is not > running. Changes in v4: > 1. Remove changes in doc/README.usb, common/usb_kbd.c and > CONFIG_USBKB_TESTC_PERIOD > 2. Modify net/net.c > Changes in v5: > 1. Change variable name to ctrlc_t_start. > 2. Use two calls of get_timer(0) to get time gap. > Changes in v6: > 1. In common/usb_kbd.c, check net_busy_flag to determine whether we poll > USB keyboard status. > 2. In include/usb.h, add external variable declaration net_busy_flag > > > common/usb_kbd.c | 15 +++++++++++++++ > include/usb.h | 2 +- > 2 files changed, 16 insertions(+), 1 deletions(-) > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > index 3174b5e..3288c69 100644 > --- a/common/usb_kbd.c > +++ b/common/usb_kbd.c > @@ -121,6 +121,9 @@ struct usb_kbd_pdata { > uint8_t flags; > }; > > +/* The period of time between two calls of usb_kbd_testc(). */ > +static unsigned long kbd_testc_tms; > + > /* Generic keyboard event polling. */ > void usb_kbd_generic_poll(void) > { > @@ -366,6 +369,18 @@ static int usb_kbd_testc(void) > struct usb_device *usb_kbd_dev; > struct usb_kbd_pdata *data; > > + /* > + * If net_busy_flag is 1, NET transfer is running, > + * then we check key pressed every second to improve > + * TFTP booting performance. > + */ > + if (net_busy_flag) { THis variable is not declared so this patch won't compile > + if (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ) > + return 0; > + else > + kbd_testc_tms = get_timer(0); > + } > + > dev = stdio_get_by_name(DEVNAME); > usb_kbd_dev = (struct usb_device *)dev->priv; > data = usb_kbd_dev->privptr; > diff --git a/include/usb.h b/include/usb.h > index d7b082d..824b394 100644 > --- a/include/usb.h > +++ b/include/usb.h > @@ -206,7 +206,7 @@ int usb_host_eth_scan(int mode); > > int drv_usb_kbd_init(void); > int usb_kbd_deregister(void); > - > +extern int net_busy_flag; > #endif > /* routines */ > int usb_init(void); /* initialize the USB Controller */ Best regards, Marek Vasut
On 07/18/2013 08:15 AM, Jim Lin wrote: > TFTP booting is slow when a USB keyboard is installed and > stdin has usbkbd added. > This fix is to change Ctrl-C polling for USB keyboard to every second > when NET transfer is running. I think this general approach is a reasonable compromise to the problem. Some issues need to be considered: 1) git bisect is broken as Marek pointed out. 2) Does the code still compile/link if USB keyboard and/or networking support is not enabled? I suspect it won't in the case where USB keyboard is enabled, yet networking support isn't. This probably needs to be solved by putting "int net_busy_flag;" into some common non-optional file so that all the users of that variable don't have to be chronically ifdef'd for all the possible feature combinations. 3) Is "net_busy_flag" the best name for the variable? Perhaps the flag could be used to disable other time-consuming polling beyond just USB keyboard CTRL-C handling. Perhaps other operations besides networking transfers could benefit from setting this flag. At the risk of bike-shedding, how about renaming this one of: reduce_non_critical_polling_frequency non_interactive_operation_in_progress ? A comment on the code below: > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > @@ -366,6 +369,18 @@ static int usb_kbd_testc(void) > struct usb_device *usb_kbd_dev; > struct usb_kbd_pdata *data; > > + /* > + * If net_busy_flag is 1, NET transfer is running, > + * then we check key pressed every second to improve > + * TFTP booting performance. > + */ > + if (net_busy_flag) { > + if (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ) > + return 0; > + else > + kbd_testc_tms = get_timer(0); > + } I think you always want to assign to kbd_testc_tms so it always records the most recent time of the most recent USB keyboard activity, not the most recent USB keyboard activity while a network operation was in progress. So, if (net_busy_flag && get_timer(kbd_testc_tms) < CONFIG_SYS_HZ) return 0; kbd_testc_tms = get_timer(0);
Dear Jim Lin, In message <1374156931-1718-1-git-send-email-jilin@nvidia.com> you wrote: > TFTP booting is slow when a USB keyboard is installed and > stdin has usbkbd added. > This fix is to change Ctrl-C polling for USB keyboard to every second > when NET transfer is running. > > Signed-off-by: Jim Lin <jilin@nvidia.com> The sequence of your patches is wrong; you must reverse it: as is, with only patch 1/2 applied, you will get compile errors because net_busy_flag is undefined. This breaks bisectability. Please switch the order of your patches. > + /* > + * If net_busy_flag is 1, NET transfer is running, > + * then we check key pressed every second to improve > + * TFTP booting performance. > + */ > + if (net_busy_flag) { > + if (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ) > + return 0; > + else > + kbd_testc_tms = get_timer(0); > + } When first entering this code, the variable kbd_testc_tms is (implicitly) initialized as zero; later, for example when running multiple network commands, the last used value (i. e. a random number) is used. So strictly speaking the comment above is incorrect, as you don't test for key presses "every second" - the first test may happen much earlier (even immediately). I think this should be explained in the comment to prevent incorrect expectations on the behaviour. Best regards, Wolfgang Denk
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 3174b5e..3288c69 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -121,6 +121,9 @@ struct usb_kbd_pdata { uint8_t flags; }; +/* The period of time between two calls of usb_kbd_testc(). */ +static unsigned long kbd_testc_tms; + /* Generic keyboard event polling. */ void usb_kbd_generic_poll(void) { @@ -366,6 +369,18 @@ static int usb_kbd_testc(void) struct usb_device *usb_kbd_dev; struct usb_kbd_pdata *data; + /* + * If net_busy_flag is 1, NET transfer is running, + * then we check key pressed every second to improve + * TFTP booting performance. + */ + if (net_busy_flag) { + if (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ) + return 0; + else + kbd_testc_tms = get_timer(0); + } + dev = stdio_get_by_name(DEVNAME); usb_kbd_dev = (struct usb_device *)dev->priv; data = usb_kbd_dev->privptr; diff --git a/include/usb.h b/include/usb.h index d7b082d..824b394 100644 --- a/include/usb.h +++ b/include/usb.h @@ -206,7 +206,7 @@ int usb_host_eth_scan(int mode); int drv_usb_kbd_init(void); int usb_kbd_deregister(void); - +extern int net_busy_flag; #endif /* routines */ int usb_init(void); /* initialize the USB Controller */
TFTP booting is slow when a USB keyboard is installed and stdin has usbkbd added. This fix is to change Ctrl-C polling for USB keyboard to every second when NET transfer is running. Signed-off-by: Jim Lin <jilin@nvidia.com> --- Changes in v2: 1. Change configuration name from CONFIG_CTRLC_POLL_MS to CONFIG_CTRLC_POLL_S. 2. New code will be executed only when CONFIG_CTRLC_POLL_S is defined in configuration header file. 3. Add description in README.console. Changes in v3: 1. Move changes to common/usb_kbd.c and doc/README.usb 2. Rename config setting to CONFIG_USBKB_TESTC_PERIOD. 3. Remove slow response on USB-keyboard input when TFTP boot is not running. Changes in v4: 1. Remove changes in doc/README.usb, common/usb_kbd.c and CONFIG_USBKB_TESTC_PERIOD 2. Modify net/net.c Changes in v5: 1. Change variable name to ctrlc_t_start. 2. Use two calls of get_timer(0) to get time gap. Changes in v6: 1. In common/usb_kbd.c, check net_busy_flag to determine whether we poll USB keyboard status. 2. In include/usb.h, add external variable declaration net_busy_flag common/usb_kbd.c | 15 +++++++++++++++ include/usb.h | 2 +- 2 files changed, 16 insertions(+), 1 deletions(-)