Re: [flashrom] [PATCH] Change image building in layout code

[David Hendricks]

On Thu, Oct 28, 2010 at 10:29 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Index: flashrom-layout_exclude_unspecified_regions/layout.c
>
> --- flashrom-layout_exclude_unspecified_regions/layout.c        (revision
> 1217)
> +++ flashrom-layout_exclude_unspecified_regions/layout.c        (working
> copy)
> @@ -22,6 +22,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <ctype.h>
> +#include <limits.h>
>  #include "flash.h"
>  #include "programmer.h"
>
> @@ -205,33 +206,62 @@
>        return -1;
>  }
>
> -int handle_romentries(struct flashchip *flash, uint8_t *oldcontents,
> uint8_t *newcontents)
> +int find_next_included_romentry(unsigned int start)
>  {
>        int i;
>   unsigned int best_start = INT_MAX;

Minor nit: Should that be UINT_MAX? Alternatively, maybe ~0 would work
without including limits.h.

Up to you.

On Thu, Oct 28, 2010 at 10:29 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
>   int best_entry = -1;
>
>   // This function does not save flash write cycles.
>   //
>   // Also it does not cope with overlapping rom layout
>   // sections.
>   // example:
>   // 00000000:00008fff gfxrom
>   // 00009000:0003ffff normal
>   // 00040000:0007ffff fallback
>   // 00000000:0007ffff all
>   //
>   // If you'd specify -i all the included flag of all other
>   // sections is still 0, so no changes will be made to the
>   // flash. Same thing if you specify -i normal -i all only
>   // normal will be updated and the rest will be kept.
>   /* First come, first serve for overlapping regions. */
>  for (i = 0; i < romimages; i++) {
>           if (rom_entries[i].included)
>           if (!rom_entries[i].included)
>                  continue;
>           /* Already past the current entry? */
>           if (start > rom_entries[i].end)
>                   continue;
>           /* Inside the current entry? */
>           if (start >= rom_entries[i].start)
>                   return i;
>           /* Entry begins after start. */
>           if (best_start > rom_entries[i].start) {
>                   best_start = rom_entries[i].start;
>                   best_entry = i;
>           }
>   }
>   return best_entry;
>
> +}
>           memcpy(newcontents + rom_entries[i].start,
>                  oldcontents + rom_entries[i].start,
>                  rom_entries[i].end - rom_entries[i].start + 1);
>
> +int handle_romentries(struct flashchip *flash, uint8_t *oldcontents,
> uint8_t *newcontents)
> +{
>   unsigned int start = 0;
>   int entry;
>   unsigned int size = flash->total_size * 1024;
>   /* If no layout file was specified or the layout file was empty,
>
> assume
>    * that the user wants to flash the complete new image.
>    */
>   if (!romimages)
>           return 0;
>   /* Non-included romentries are ignored.
>    * The union of all included romentries is used from the new image.
>    */
>   while (start < size) {
>           entry = find_next_included_romentry(start);
>           /* No more romentries for remaining region? */
>           if (entry < 0) {
>                   memcpy(newcontents + start, oldcontents + start,
>                          size - start);
>                   break;
>           }
>           if (rom_entries[entry].start > start)
>                   memcpy(newcontents + start, oldcontents + start,
>                          rom_entries[entry].start - start);
>           /* Skip to location after current romentry. */
>           start = rom_entries[entry].end + 1;
>           /* Catch overflow. */
>           if (!start)
>                   break;
>  }
>  return 0;
>
> }

Looks good to me. I've also tested this successfully, so:

Acked-by: David Hendricks