| Message ID | 1525077174-6211-12-git-send-email-alex.kiernan@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | Lukasz Majewski |
| Headers | show |
| Series | Add fastboot UDP support | expand |
On Mon, Apr 30, 2018 at 3:32 AM, Alex Kiernan <alex.kiernan@gmail.com> wrote: > Change the behaviour of 'continue' so that we simply exit the fastboot > server and leave the caller to decide what to do next. This matches > the USB fastboot behaviour. Good, I was considering recommending this approach. Acked-by: Joe Hershberger <joe.hershberger@ni.com> > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > --- > > Changes in v2: None > > net/fastboot.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/net/fastboot.c b/net/fastboot.c > index cd09ada..ed13890 100644 > --- a/net/fastboot.c > +++ b/net/fastboot.c > @@ -218,8 +218,6 @@ static void fastboot_send(struct fastboot_header fb_header, char *fastboot_data, > if (!strncmp("OKAY", response, 4)) { > if (!strcmp("boot", cmd_string)) { > boot_downloaded_image(); > - } else if (!strcmp("continue", cmd_string)) { > - run_command(env_get("bootcmd"), CMD_FLAG_ENV); > } else if (!strncmp("reboot", cmd_string, 6)) { > /* Matches reboot or reboot-bootloader */ > do_reset(NULL, 0, 0, NULL); > @@ -313,20 +311,15 @@ static void fb_erase(char *response) > #endif > > /** > - * Continues normal boot process by running "bootcmd". Writes > + * Continues normal boot process by exiting fastboot server. Writes > * to response. > * > * @param repsonse Pointer to fastboot response buffer > */ > static void fb_continue(char *response) > { > - char *bootcmd; > - > - bootcmd = env_get("bootcmd"); > - if (bootcmd) > - fastboot_okay(NULL, response); > - else > - fastboot_fail("bootcmd not set", response); > + net_set_state(NETLOOP_SUCCESS); > + fastboot_okay(NULL, response); > } > > /** > -- > 2.7.4 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On Thu, May 3, 2018 at 1:58 PM Joe Hershberger <joe.hershberger@ni.com> wrote: > On Mon, Apr 30, 2018 at 3:32 AM, Alex Kiernan <alex.kiernan@gmail.com> > wrote: > > Change the behaviour of 'continue' so that we simply exit the fastboot > > server and leave the caller to decide what to do next. This matches > > the USB fastboot behaviour. > > Good, I was considering recommending this approach. > > Acked-by: Joe Hershberger <joe.hershberger@ni.com> Agreed, this is really the correct behavior of continue. Reviewed-by: Jocelyn Bohr <bohr@google.com>
On Thu, May 3, 2018 at 9:58 PM Joe Hershberger <joe.hershberger@ni.com> wrote: > On Mon, Apr 30, 2018 at 3:32 AM, Alex Kiernan <alex.kiernan@gmail.com> wrote: > > Change the behaviour of 'continue' so that we simply exit the fastboot > > server and leave the caller to decide what to do next. This matches > > the USB fastboot behaviour. > Good, I was considering recommending this approach. > Acked-by: Joe Hershberger <joe.hershberger@ni.com> > > > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > > --- > > > > Changes in v2: None > > > > net/fastboot.c | 13 +++---------- > > 1 file changed, 3 insertions(+), 10 deletions(-) > > > > diff --git a/net/fastboot.c b/net/fastboot.c > > index cd09ada..ed13890 100644 > > --- a/net/fastboot.c > > +++ b/net/fastboot.c > > @@ -218,8 +218,6 @@ static void fastboot_send(struct fastboot_header fb_header, char *fastboot_data, > > if (!strncmp("OKAY", response, 4)) { > > if (!strcmp("boot", cmd_string)) { > > boot_downloaded_image(); > > - } else if (!strcmp("continue", cmd_string)) { > > - run_command(env_get("bootcmd"), CMD_FLAG_ENV); > > } else if (!strncmp("reboot", cmd_string, 6)) { > > /* Matches reboot or reboot-bootloader */ > > do_reset(NULL, 0, 0, NULL); > > @@ -313,20 +311,15 @@ static void fb_erase(char *response) > > #endif > > > > /** > > - * Continues normal boot process by running "bootcmd". Writes > > + * Continues normal boot process by exiting fastboot server. Writes > > * to response. > > * > > * @param repsonse Pointer to fastboot response buffer > > */ > > static void fb_continue(char *response) > > { > > - char *bootcmd; > > - > > - bootcmd = env_get("bootcmd"); > > - if (bootcmd) > > - fastboot_okay(NULL, response); > > - else > > - fastboot_fail("bootcmd not set", response); > > + net_set_state(NETLOOP_SUCCESS); > > + fastboot_okay(NULL, response); > > } > > I'm struggling with this returning the final ACK to the client correctly. If I add something like mdelay(5) before exiting the server, then I get the final ACK, without that, I never see it hit the network, even though I can see it traversing all the layers of the net code if I wind up the debug (and of course it makes it out if I wind up the debug, presumably as it then goes much slower). Wild hand wavey speculation... it's getting lost in something like a DMA buffer in the network driver which gets torn down before it makes it onto the wire? This is using the TI CPSW on an am335x.
On Tue, May 8, 2018 at 4:20 AM, Alex Kiernan <alex.kiernan@gmail.com> wrote: > On Thu, May 3, 2018 at 9:58 PM Joe Hershberger <joe.hershberger@ni.com> > wrote: > >> On Mon, Apr 30, 2018 at 3:32 AM, Alex Kiernan <alex.kiernan@gmail.com> > wrote: >> > Change the behaviour of 'continue' so that we simply exit the fastboot >> > server and leave the caller to decide what to do next. This matches >> > the USB fastboot behaviour. > >> Good, I was considering recommending this approach. > >> Acked-by: Joe Hershberger <joe.hershberger@ni.com> > >> > >> > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> >> > --- >> > >> > Changes in v2: None >> > >> > net/fastboot.c | 13 +++---------- >> > 1 file changed, 3 insertions(+), 10 deletions(-) >> > >> > diff --git a/net/fastboot.c b/net/fastboot.c >> > index cd09ada..ed13890 100644 >> > --- a/net/fastboot.c >> > +++ b/net/fastboot.c >> > @@ -218,8 +218,6 @@ static void fastboot_send(struct fastboot_header > fb_header, char *fastboot_data, >> > if (!strncmp("OKAY", response, 4)) { >> > if (!strcmp("boot", cmd_string)) { >> > boot_downloaded_image(); >> > - } else if (!strcmp("continue", cmd_string)) { >> > - run_command(env_get("bootcmd"), CMD_FLAG_ENV); >> > } else if (!strncmp("reboot", cmd_string, 6)) { >> > /* Matches reboot or reboot-bootloader */ >> > do_reset(NULL, 0, 0, NULL); >> > @@ -313,20 +311,15 @@ static void fb_erase(char *response) >> > #endif >> > >> > /** >> > - * Continues normal boot process by running "bootcmd". Writes >> > + * Continues normal boot process by exiting fastboot server. Writes >> > * to response. >> > * >> > * @param repsonse Pointer to fastboot response buffer >> > */ >> > static void fb_continue(char *response) >> > { >> > - char *bootcmd; >> > - >> > - bootcmd = env_get("bootcmd"); >> > - if (bootcmd) >> > - fastboot_okay(NULL, response); >> > - else >> > - fastboot_fail("bootcmd not set", response); >> > + net_set_state(NETLOOP_SUCCESS); >> > + fastboot_okay(NULL, response); >> > } >> > > > I'm struggling with this returning the final ACK to the client correctly. > If I add something like mdelay(5) before exiting the server, then I get the > final ACK, without that, I never see it hit the network, even though I can > see it traversing all the layers of the net code if I wind up the debug > (and of course it makes it out if I wind up the debug, presumably as it > then goes much slower). > > Wild hand wavey speculation... it's getting lost in something like a DMA > buffer in the network driver which gets torn down before it makes it onto > the wire? This is using the TI CPSW on an am335x. Sounds like a reasonable theory. The eth driver for that board probably needs to check for the TX channel to be in a quiescent state in the davinci_eth_close() function before tearing it down. -Joe
diff --git a/net/fastboot.c b/net/fastboot.c index cd09ada..ed13890 100644 --- a/net/fastboot.c +++ b/net/fastboot.c @@ -218,8 +218,6 @@ static void fastboot_send(struct fastboot_header fb_header, char *fastboot_data, if (!strncmp("OKAY", response, 4)) { if (!strcmp("boot", cmd_string)) { boot_downloaded_image(); - } else if (!strcmp("continue", cmd_string)) { - run_command(env_get("bootcmd"), CMD_FLAG_ENV); } else if (!strncmp("reboot", cmd_string, 6)) { /* Matches reboot or reboot-bootloader */ do_reset(NULL, 0, 0, NULL); @@ -313,20 +311,15 @@ static void fb_erase(char *response) #endif /** - * Continues normal boot process by running "bootcmd". Writes + * Continues normal boot process by exiting fastboot server. Writes * to response. * * @param repsonse Pointer to fastboot response buffer */ static void fb_continue(char *response) { - char *bootcmd; - - bootcmd = env_get("bootcmd"); - if (bootcmd) - fastboot_okay(NULL, response); - else - fastboot_fail("bootcmd not set", response); + net_set_state(NETLOOP_SUCCESS); + fastboot_okay(NULL, response); } /**
Change the behaviour of 'continue' so that we simply exit the fastboot server and leave the caller to decide what to do next. This matches the USB fastboot behaviour. Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> --- Changes in v2: None net/fastboot.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)