[U-Boot] [RFC PATCH v2 07/20] net: fastboot: Merge AOSP UDP fastboot
Joe Hershberger
joe.hershberger at ni.com
Tue May 8 15:24:08 UTC 2018
On Tue, May 8, 2018 at 4:11 AM, Alex Kiernan <alex.kiernan at gmail.com> wrote:
> On Thu, May 3, 2018 at 9:39 PM Joe Hershberger <joe.hershberger at ni.com>
> wrote:
>
>> On Mon, Apr 30, 2018 at 3:32 AM, Alex Kiernan <alex.kiernan at gmail.com>
> wrote:
>
> <snip>
>
>> > +/**
>> > + * Constructs and sends a packet in response to received fastboot
> packet
>> > + *
>> > + * @param fb_header Header for response packet
>> > + * @param fastboot_data Pointer to received fastboot data
>> > + * @param fastboot_data_len Length of received fastboot data
>> > + * @param retransmit Nonzero if sending last sent packet
>> > + */
>> > +static void fastboot_send(struct fastboot_header fb_header, char
> *fastboot_data,
>> > + unsigned int fastboot_data_len, uchar
> retransmit)
>> > +{
>> > + uchar *packet;
>> > + uchar *packet_base;
>> > + int len = 0;
>> > + const char *error_msg = "An error occurred.";
>> > + short tmp;
>> > + struct fastboot_header fb_response_header = fb_header;
>> > + char response[FASTBOOT_RESPONSE_LEN] = {0};
>> > + /*
>> > + * We will always be sending some sort of packet, so
>> > + * cobble together the packet headers now.
>> > + */
>> > + packet = net_tx_packet + net_eth_hdr_size() + IP_UDP_HDR_SIZE;
>> > + packet_base = packet;
>> > +
>> > + /* Resend last packet */
>> > + if (retransmit) {
>> > + memcpy(packet, last_packet, last_packet_len);
>> > + net_send_udp_packet(net_server_ethaddr,
> fastboot_remote_ip,
>> > + fastboot_remote_port,
> fastboot_our_port,
>> > + last_packet_len);
>> > + return;
>> > + }
>> > +
>> > + fb_response_header.seq = htons(fb_response_header.seq);
>> > + memcpy(packet, &fb_response_header, sizeof(fb_response_header));
>> > + packet += sizeof(fb_response_header);
>> > +
>> > + switch (fb_header.id) {
>> > + case FASTBOOT_QUERY:
>> > + tmp = htons(fb_sequence_number);
>> > + memcpy(packet, &tmp, sizeof(tmp));
>> > + packet += sizeof(tmp);
>> > + break;
>> > + case FASTBOOT_INIT:
>> > + tmp = htons(fb_udp_version);
>> > + memcpy(packet, &tmp, sizeof(tmp));
>> > + packet += sizeof(tmp);
>> > + tmp = htons(fb_packet_size);
>> > + memcpy(packet, &tmp, sizeof(tmp));
>> > + packet += sizeof(tmp);
>> > + break;
>> > + case FASTBOOT_ERROR:
>> > + memcpy(packet, error_msg, strlen(error_msg));
>> > + packet += strlen(error_msg);
>> > + break;
>> > + case FASTBOOT_FASTBOOT:
>> > + if (!cmd_string) {
>> > + /* Parse command and send ack */
>> > + cmd_parameter = fastboot_data;
>
>> This seems unnecessary. There are only 4 cases handled, and of them
>> only download seems to be a command that happens more than once. And
>> in download, the first past through this parameter is saved internally
>> as bytes_expected.
>
>> > + cmd_string = strsep(&cmd_parameter, ":");
>> > + cmd_string = strdup(cmd_string);
>> > + if (cmd_parameter)
>> > + cmd_parameter = strdup(cmd_parameter);
>> > + } else if (!strcmp("getvar", cmd_string)) {
>> > + fb_getvar(response);
>
>> Seems like you should simply pass the "fastboot_data" as a parameter
>> here rather than using a global.
>
> I'm completely pulling this apart in a later patch. I wonder if I should
> really be folding some of these back into this - I was trying to maintain a
> clear line to the AOSP code which this came from.
What value do we get from that? Is there a merge technique you are
trying to enable to make pulling in future AOSP changes easier? Are
you trying to isolate your changes from AOSP for attribution purposes?
So far I'm of the opinion that the fixes to the existing code should
simply be there in the originating patch. Feel free to convince me
otherwise.
>> > +/**
>> > + * Copies image data from fastboot_data to CONFIG_FASTBOOT_BUF_ADDR.
>> > + * Writes to response.
>> > + *
>> > + * @param fastboot_data Pointer to received fastboot data
>> > + * @param fastboot_data_len Length of received fastboot data
>> > + * @param repsonse Pointer to fastboot response buffer
>> > + */
>> > +static void fb_download(char *fastboot_data, unsigned int
> fastboot_data_len,
>> > + char *response)
>> > +{
>> > + char *tmp;
>> > +
>> > + if (bytes_expected == 0) {
>> > + if (!cmd_parameter) {
>> > + write_fb_response("FAIL", "Expected command
> parameter",
>> > + response);
>> > + return;
>> > + }
>> > + bytes_expected = simple_strtoul(cmd_parameter, &tmp,
> 16);
>> > + if (bytes_expected == 0) {
>> > + write_fb_response("FAIL", "Expected nonzero
> image size",
>> > + response);
>> > + return;
>> > + }
>> > + }
>> > + if (fastboot_data_len == 0 && bytes_received == 0) {
>> > + /* Nothing to download yet. Response is of the form:
>> > + * [DATA|FAIL]$cmd_parameter
>> > + *
>> > + * where cmd_parameter is an 8 digit hexadecimal number
>> > + */
>> > + if (bytes_expected > CONFIG_FASTBOOT_BUF_SIZE)
>> > + write_fb_response("FAIL", cmd_parameter,
> response);
>> > + else
>> > + write_fb_response("DATA", cmd_parameter,
> response);
>> > + } else if (fastboot_data_len == 0 &&
>> > + (bytes_received >= bytes_expected)) {
>> > + /* Download complete. Respond with "OKAY" */
>> > + write_fb_response("OKAY", "", response);
>> > + image_size = bytes_received;
>> > + bytes_expected = 0;
>> > + bytes_received = 0;
>> > + } else {
>> > + if (fastboot_data_len == 0 ||
>> > + (bytes_received + fastboot_data_len) >
> bytes_expected) {
>> > + write_fb_response("FAIL",
>> > + "Received invalid data
> length",
>> > + response);
>> > + return;
>> > + }
>> > + /* Download data to CONFIG_FASTBOOT_BUF_ADDR */
>> > + memcpy((void *)CONFIG_FASTBOOT_BUF_ADDR +
> bytes_received,
>> > + fastboot_data, fastboot_data_len);
>
>> This is different than all the other approaches, which take the load
>> address as a command parameter. Is there a good reason to have it
>> baked in like this?
>
> Feels odd to me too, but this is just following the USB code. I'm thinking
> we should change the command so it's:
>
> fastboot ... [<load-address> [<size>]]
>
> And then default to the current values is they're not overridden?
>
>> > + bytes_received += fastboot_data_len;
>> > + }
>> > +}
>> > +
>> > +/**
>> > + * Writes the previously downloaded image to the partition indicated by
>> > + * cmd_parameter. Writes to response.
>> > + *
>> > + * @param repsonse Pointer to fastboot response buffer
>> > + */
>> > +static void fb_flash(char *response)
>> > +{
>> > + fb_mmc_flash_write(cmd_parameter, (void
> *)CONFIG_FASTBOOT_BUF_ADDR,
>> > + image_size, response);
>
>> It's peculiar that this hard-codes mmc.
>
> Fixed up in a later patch.
>
>> > +/**
>> > + * Boots into downloaded image.
>> > + */
>> > +static void boot_downloaded_image(void)
>> > +{
>> > + char kernel_addr[12];
>> > + char *fdt_addr = env_get("fdt_addr_r");
>> > + char *const bootm_args[] = {
>> > + "bootm", kernel_addr, "-", fdt_addr, NULL
>> > + };
>
>> It seems like this should be able to affected from the host side...
>> for instance choosing a config in an ITB.
>
>> How is the fdt supposed to be populated in the scenario at all?
>
> I strip this bit out in a later patch and replace it with either a simple
> bootm (which follows the USB code), or run fastbootcmd.
>
> Again, this is part of trying to land this patch as close as possible to
> the code which comes from AOSP.
>
> --
> Alex Kiernan
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list