[U-Boot,v4,1/1] NET: Improve TFTP booting performance when CONFIG_USB_KEYBOARD

TFTP booting is slow when a USB keyboard is installed and
CONFIG_USB_KEYBOARD is defined.
The fix is to change Ctrl-C polling 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

 net/net.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

Dear Jim Lin,

In message <1372847667-31928-1-git-send-email-jilin@nvidia.com> you wrote:
> TFTP booting is slow when a USB keyboard is installed and
> CONFIG_USB_KEYBOARD is defined.
> The fix is to change Ctrl-C polling to every second when NET transfer
> is running.

I'm not sure if we can accept this implementation.

> +#ifdef CONFIG_USB_KEYBOARD
> +		/*
> +		 * Reduce ctrl-c checking to 1 second once
> +		 * to improve TFTP boot performance.
> +		 */
> +		ctrlc_t = get_timer(kbd_ctrlc_tms);
> +		if (ctrlc_t > CONFIG_SYS_HZ) {
> +			ctrlc_result = ctrlc();
> +			kbd_ctrlc_tms = get_timer(0);
> +		} else {
> +			ctrlc_result = 0;
> +		}
> +		if (ctrlc_result) {
> +#else

get_timer() is used by a number of network related services.  For
information, just grep for it in the net/ and drivers/net/
directories.  The "get_timer(0)" used in your code resets a global
resource, and has thus the potential of messing up a number of
timeouts running elsewhere in the network code.  I wonder to which
extend this has actually been considered (and tested) ?

Best regards,

Wolfgang Denk

On 07/03/2013 05:20 AM, Wolfgang Denk wrote:
> Dear Jim Lin,
> 
> In message <1372847667-31928-1-git-send-email-jilin@nvidia.com> you wrote:
>> TFTP booting is slow when a USB keyboard is installed and
>> CONFIG_USB_KEYBOARD is defined.
>> The fix is to change Ctrl-C polling to every second when NET transfer
>> is running.
> 
> I'm not sure if we can accept this implementation.
> 
>> +#ifdef CONFIG_USB_KEYBOARD
>> +		/*
>> +		 * Reduce ctrl-c checking to 1 second once
>> +		 * to improve TFTP boot performance.
>> +		 */
>> +		ctrlc_t = get_timer(kbd_ctrlc_tms);
>> +		if (ctrlc_t > CONFIG_SYS_HZ) {
>> +			ctrlc_result = ctrlc();
>> +			kbd_ctrlc_tms = get_timer(0);
>> +		} else {
>> +			ctrlc_result = 0;
>> +		}
>> +		if (ctrlc_result) {
>> +#else
> 
> get_timer() is used by a number of network related services.  For
> information, just grep for it in the net/ and drivers/net/
> directories.  The "get_timer(0)" used in your code resets a global
> resource, and has thus the potential of messing up a number of
> timeouts running elsewhere in the network code.  I wonder to which
> extend this has actually been considered (and tested) ?

I recall you mentioning this before, but can you expand on this a bit
please?

For the two platforms I'm familiar with (Tegra and BCM2835), the
implementation of get_timer() is simply:

unlong get_timer(ulong base)
{
    ulong time = read_hw_register()
    time -= base;
    return time;
}

There's no global state involved. Is this implementation of get_timer()
wrong somehow? I'm having a hard time envisaging what kind of global
state it's supposed to maintain.

I always thought that every user of get_timer() was supposed to do
something like:

ulong base = get_timer(0);
... work to be timed
ulong time_diff = get_timer(base);

in other words, every user maintains their own base variable, and hence
get_timer(0) doesn't affect any other users.

Dear Stephen,

In message <51D45F4D.2010908@wwwdotorg.org> you wrote:
>
> > get_timer() is used by a number of network related services.  For
> > information, just grep for it in the net/ and drivers/net/
> > directories.  The "get_timer(0)" used in your code resets a global
> > resource, and has thus the potential of messing up a number of
> > timeouts running elsewhere in the network code.  I wonder to which
> > extend this has actually been considered (and tested) ?
> 
> I recall you mentioning this before, but can you expand on this a bit
> please?
> 
> For the two platforms I'm familiar with (Tegra and BCM2835), the
> implementation of get_timer() is simply:
> 
> unlong get_timer(ulong base)
> {
>     ulong time = read_hw_register()
>     time -= base;
>     return time;
> }
> 
> There's no global state involved. Is this implementation of get_timer()
> wrong somehow? I'm having a hard time envisaging what kind of global
> state it's supposed to maintain.

It appears you are (basically) right.  Eventually I remember an old
implementation that has been fixed since.

I checked all implementations I could find (all 102 of them) and they
all behave as you showed in your example, i. e. harmless.

An exception is "arch/arm/cpu/sa1100/timer.c" which does not respect
the "base" argument at all, i. e. which is broken.


> I always thought that every user of get_timer() was supposed to do
> something like:
> 
> ulong base = get_timer(0);
> ... work to be timed
> ulong time_diff = get_timer(base);
> 
> in other words, every user maintains their own base variable, and hence
> get_timer(0) doesn't affect any other users.

Yes, you are right. Sorry for causing confusion here.

Best regards,

Wolfgang Denk