Hi Svyatoslav,

Thank you for the patch.

On Wed, Sep 30, 2026 at 10:55, Svyatoslav Ryhel <[email protected]> wrote:

> From: Ion Agorria <[email protected]>
>
> This allows boards to intercept fastboot flash/erase and modify/change
> behavior according to board specifics like AES encryption or RSA signing.
> Add board_fastboot_get_part_info to provide info as fastboot tool queries
> during flash/erase.
>
> Signed-off-by: Ion Agorria <[email protected]>
> Signed-off-by: Svyatoslav Ryhel <[email protected]>

This patch does not apply on main
commit fed92546960a ("Gitlab CI: sage: Update for labgrid 26.0 behavior")

I'll try to review it anyways, but please consider resubmitting it after 
rebasing.

> ---
>  drivers/fastboot/fb_command.c | 20 ++++++++++++++++++++
>  drivers/fastboot/fb_getvar.c  | 12 ++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> index 18d86988f4c..284d771fe31 100644
> --- a/drivers/fastboot/fb_command.c
> +++ b/drivers/fastboot/fb_command.c
> @@ -17,6 +17,17 @@
>  #include <vsprintf.h>
>  #include <linux/printk.h>
>  

Please document the function. Especially the return code so that
board-specific implementations know what to do.

> +__weak int board_fastboot_flash(char *cmd_parameter, void *download_buffer,
> +                             u32 download_bytes, char *response)
> +{
> +     return 0;

Why 0?

Can we please use a more meaningful error code here? -ENOTSUPP for example.

> +}
> +
> +__weak int board_fastboot_erase(char *cmd_parameter, char *response)
> +{
> +     return 0;

Same.

> +}
> +
>  /**
>   * image_size - final fastboot image size
>   */
> @@ -339,6 +350,11 @@ void fastboot_data_complete(char *response)
>   */
>  static void __maybe_unused flash(char *cmd_parameter, char *response)
>  {
> +     if (IS_ENABLED(CONFIG_FASTBOOT_FLASH) &&
> +         board_fastboot_flash(cmd_parameter, fastboot_buf_addr,
> +                              image_size, response))
> +             return;
> +
>       if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_BLOCK))
>               fastboot_block_flash_write(cmd_parameter, fastboot_buf_addr,
>                                          image_size, response);
> @@ -367,6 +383,10 @@ static void __maybe_unused flash(char *cmd_parameter, 
> char *response)
>   */
>  static void __maybe_unused erase(char *cmd_parameter, char *response)
>  {
> +     if (IS_ENABLED(CONFIG_FASTBOOT_FLASH) &&
> +         board_fastboot_erase(cmd_parameter, response))
> +             return;
> +
>       if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_BLOCK))
>               fastboot_block_erase(cmd_parameter, response);
>  
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index e8aa0e09aa6..af65fca548f 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -29,6 +29,12 @@ static void getvar_partition_type(char *part_name, char 
> *response);
>  static void getvar_partition_size(char *part_name, char *response);
>  static void getvar_is_userspace(char *var_parameter, char *response);
>  
> +__weak int board_fastboot_get_part_info(const char *part_name, char 
> *response,
> +                                     size_t *size)
> +{
> +     return 1;

Why 1?

What happens if a board_fastboot_get_part_info() returns 1 here?

Also if we look at the documentation of part_get_info_by_name():
"""
 * part_get_info_by_name() - Search for a partition by name
 *                           among all available registered partitions
 *
 * @desc:       block device descriptor
 * @name:       the specified table entry name
 * @info:       returns the disk partition info
 *
 * Return: the partition number on match (starting on 1), -1 on no match,
 * otherwise error
 */
int part_get_info_by_name(struct blk_desc *desc, const char *name,
                          struct disk_partition *info);
"""

So 1 can be confusing.

Can't we return something like -ENOTSUPP instead?
That makes it more clear that this is a stub operation that can be
re-implemented by each board.

> +}
> +
>  static const struct {
>       const char *variable;
>       bool list;
> @@ -116,6 +122,12 @@ static int getvar_get_part_info(const char *part_name, 
> char *response,
>       struct disk_partition disk_part;
>       struct part_info *part_info;
>  
> +     if (IS_ENABLED(CONFIG_FASTBOOT_FLASH)) {
> +             r = board_fastboot_get_part_info(part_name, response, size);
> +             if (r != 1)
> +                     return r;
> +     }
> +
>       if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_BLOCK)) {
>               r = fastboot_block_get_part_info(part_name, &dev_desc, 
> &disk_part,
>                                                response);
> -- 
> 2.53.0

Reply via email to