Re: [PATCH RFT v4 3/3] fastboot: integrate block flashing back-end

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2749 bytes --]

On Thu, May 22, 2025 at 02:37:07PM +0200, Neil Armstrong wrote:

> From: Dmitrii Merkurev <dimorinny@google.com>
> 
> 1. Get partition info/size
> 2. Erase partition
> 3. Flash partition
> 4. BCB
> 
> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Tested-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/fastboot/Kconfig      | 29 +++++++++++++++++++++++++++++
>  drivers/fastboot/Makefile     |  1 +
>  drivers/fastboot/fb_command.c |  8 ++++++++
>  drivers/fastboot/fb_common.c  | 22 ++++++++++++++++++----
>  drivers/fastboot/fb_getvar.c  |  8 +++++++-
>  5 files changed, 63 insertions(+), 5 deletions(-)

I know this was posted before I replied with more feedback moments ago.

[snip]
> @@ -193,6 +197,31 @@ config FASTBOOT_MMC_USER_NAME
>  	  defined here.
>  	  The default target name for erasing EMMC_USER is "mmc0".
>  
> +config FASTBOOT_FLASH_BLOCK_INTERFACE_NAME
> +	string "Define FASTBOOT block interface name"
> +	depends on FASTBOOT_FLASH_BLOCK
> +	default ""
> +	help
> +	  The fastboot "flash" and "erase" commands support operations
> +	  on any Block device, this should specify the block device name
> +	  like ide, scsi, usb, sata, nvme, virtio, blkmap, mtd...
> +	  The mmc block device type can be used but most of the features
> +	  available in the FASTBOOT_MMC will be missing.
> +	  Consider using FASTBOOT_MMC on a MMC block device until all
> +	  features are migrated.

A default like "" in order to un-stick configs that are now here and
enabling the option is wrong. If we're enabling new functionality for
platforms, it needs to be configured correctly. This leads to building
code on platforms that won't be used on the platform so we've likely
added run-time bloat for no benefit.

> +config FASTBOOT_FLASH_BLOCK_DEVICE_ID
> +	int "Define FASTBOOT block device identifier"
> +	depends on FASTBOOT_FLASH_BLOCK
> +	default 0
> +	help
> +	  The fastboot "flash" and "erase" commands support operations
> +	  on any Block device, this should specify the block device
> +	  identifier on the system, as a number.
> +          The device identifier should be 0 for first device on the
> +	  interface type specified in FLASH_BLOCK_INTERFACE_NAME config,
> +	  1 the second, etc...

This help should be one paragraph and note something along the lines of:
- Device identifiers are numbered starting from 0.
- The most common case is to use the first controller.

And then yes, "default 0" is fine here because it is a reasonable
default when configuring the system to use the functionality.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]