| Message ID | 20240606-bootmeth-android-v1-2-0c69d4457cc5@baylibre.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | bootstd: Add Android support | expand |
Hi Mattijs, On Thu, Jun 6, 2024 at 2:24 PM Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote: > > When reading a boot image header, we may need to retrieve the header > version. > > Add a helper function for it. > > Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > --- > boot/image-android.c | 7 ++++++- > include/image.h | 7 +++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/boot/image-android.c b/boot/image-android.c > index ddd8ffd5e540..4f8fb51585eb 100644 > --- a/boot/image-android.c > +++ b/boot/image-android.c > @@ -185,7 +185,7 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr, > return false; > } > > - if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) { > + if (android_image_get_version(boot_hdr) > 2) { > if (!vendor_boot_hdr) { > printf("For boot header v3+ vendor boot image has to be provided\n"); > return false; > @@ -203,6 +203,11 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr, > return true; > } > > +u32 android_image_get_version(const void *hdr) > +{ > + return ((struct andr_boot_img_hdr_v0 *)hdr)->header_version; > +} > + > static ulong android_image_get_kernel_addr(struct andr_image_data *img_data) > { > /* > diff --git a/include/image.h b/include/image.h > index acffd17e0dfd..18e5ced5ab42 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -1963,6 +1963,13 @@ bool is_android_boot_image_header(const void *hdr); > */ > bool is_android_vendor_boot_image_header(const void *vendor_boot_img); > > +/** > + * android_image_get_version() - Retrieve the boot.img version > + * > + * Return: Android boot image header version. > + */ > +u32 android_image_get_version(const void *hdr); > + > /** > * get_abootimg_addr() - Get Android boot image address > * > > -- > 2.45.0 > why introduce a helper function if there is only one user of it? android_image_get_data() expects andr_boot_img_hdr_v0 anyway, as it has an explicit check for it in the very beginning (is_android_boot_image_header()). Have you considered adjusting android_image_get_data() declaration, and just use andr_boot_img_hdr_v0 *boot_hdr as first param instead (like it's done for example in android_boot_image_v0_v1_v2_parse_hdr()) and then rely on implicit cast when this function is used. this is of course all a matter of preference, just thinking out loud
Hi Igor, Thank you for the review. On lun., juin 10, 2024 at 11:20, Igor Opaniuk <igor.opaniuk@gmail.com> wrote: > Hi Mattijs, > > On Thu, Jun 6, 2024 at 2:24 PM Mattijs Korpershoek > <mkorpershoek@baylibre.com> wrote: >> >> When reading a boot image header, we may need to retrieve the header >> version. >> >> Add a helper function for it. >> >> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> >> --- >> boot/image-android.c | 7 ++++++- >> include/image.h | 7 +++++++ >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/boot/image-android.c b/boot/image-android.c >> index ddd8ffd5e540..4f8fb51585eb 100644 >> --- a/boot/image-android.c >> +++ b/boot/image-android.c >> @@ -185,7 +185,7 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr, >> return false; >> } >> >> - if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) { >> + if (android_image_get_version(boot_hdr) > 2) { >> if (!vendor_boot_hdr) { >> printf("For boot header v3+ vendor boot image has to be provided\n"); >> return false; >> @@ -203,6 +203,11 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr, >> return true; >> } >> >> +u32 android_image_get_version(const void *hdr) >> +{ >> + return ((struct andr_boot_img_hdr_v0 *)hdr)->header_version; >> +} >> + >> static ulong android_image_get_kernel_addr(struct andr_image_data *img_data) >> { >> /* >> diff --git a/include/image.h b/include/image.h >> index acffd17e0dfd..18e5ced5ab42 100644 >> --- a/include/image.h >> +++ b/include/image.h >> @@ -1963,6 +1963,13 @@ bool is_android_boot_image_header(const void *hdr); >> */ >> bool is_android_vendor_boot_image_header(const void *vendor_boot_img); >> >> +/** >> + * android_image_get_version() - Retrieve the boot.img version >> + * >> + * Return: Android boot image header version. >> + */ >> +u32 android_image_get_version(const void *hdr); >> + >> /** >> * get_abootimg_addr() - Get Android boot image address >> * >> >> -- >> 2.45.0 >> > why introduce a helper function if there is only one user of it? I added a second user of the helper function in patch 5/6. > > android_image_get_data() expects andr_boot_img_hdr_v0 anyway, > as it has an explicit check for it in the very beginning > (is_android_boot_image_header()). Right. > > Have you considered adjusting android_image_get_data() declaration, and just use > andr_boot_img_hdr_v0 *boot_hdr as first param instead (like it's done > for example in > android_boot_image_v0_v1_v2_parse_hdr()) and then rely on implicit > cast when this > function is used. > > this is of course all a matter of preference, just thinking out loud I've given this some more thought, and since I'm already using struct andr_boot_img_hdr_v0 in bootmeth_android/scan_boot_part(), I will drop this patch for v2. The helper seems indeed a bit useless given that we can use the struct's member for this. Thanks! > > -- > Best regards - Atentamente - Meilleures salutations > > Igor Opaniuk > > mailto: igor.opaniuk@gmail.com > skype: igor.opanyuk > https://www.linkedin.com/in/iopaniuk
diff --git a/boot/image-android.c b/boot/image-android.c index ddd8ffd5e540..4f8fb51585eb 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -185,7 +185,7 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr, return false; } - if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) { + if (android_image_get_version(boot_hdr) > 2) { if (!vendor_boot_hdr) { printf("For boot header v3+ vendor boot image has to be provided\n"); return false; @@ -203,6 +203,11 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr, return true; } +u32 android_image_get_version(const void *hdr) +{ + return ((struct andr_boot_img_hdr_v0 *)hdr)->header_version; +} + static ulong android_image_get_kernel_addr(struct andr_image_data *img_data) { /* diff --git a/include/image.h b/include/image.h index acffd17e0dfd..18e5ced5ab42 100644 --- a/include/image.h +++ b/include/image.h @@ -1963,6 +1963,13 @@ bool is_android_boot_image_header(const void *hdr); */ bool is_android_vendor_boot_image_header(const void *vendor_boot_img); +/** + * android_image_get_version() - Retrieve the boot.img version + * + * Return: Android boot image header version. + */ +u32 android_image_get_version(const void *hdr); + /** * get_abootimg_addr() - Get Android boot image address *
When reading a boot image header, we may need to retrieve the header version. Add a helper function for it. Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> --- boot/image-android.c | 7 ++++++- include/image.h | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-)