Re: [PATCH RFT v3 0/3] fastboot: add support for generic block flashing

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Tom,

On mer., mai 21, 2025 at 13:03, Tom Rini <trini@konsulko.com> wrote:

> On Wed, May 21, 2025 at 08:52:41PM +0200, Mattijs Korpershoek wrote:
>> Hi Tom,
>> 
>> On mer., mai 21, 2025 at 09:12, Tom Rini <trini@konsulko.com> wrote:
>> 
>> > On Wed, May 21, 2025 at 04:49:35PM +0200, Mattijs Korpershoek wrote:
>> >> Hi Neil,
>> >> 
>> >> On mar., mai 20, 2025 at 13:35, Mattijs Korpershoek <mkorpershoek@kernel.org> wrote:
>> >> 
>> >> > Hi,
>> >> >
>> >> > On Tue, 06 May 2025 18:10:06 +0200, neil.armstrong@linaro.org wrote:
>> >> >> This serie permits using any block device as target
>> >> >> for fastboot by moving the generic block logic into
>> >> >> a common set of helpers and also use them as generic
>> >> >> backend.
>> >> >> 
>> >> >> The erase logic has been extended to support software
>> >> >> erase since only 2 block drivers exposes the erase
>> >> >> operation.
>> >> >> 
>> >> >> [...]
>> >> >
>> >> > Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu-next)
>> >> >
>> >> > [1/3] fastboot: blk: introduce fastboot block flashing support
>> >> >       https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/88239b5bb04bea2b58f7bf4c3ea72cf832de818c
>> >> > [2/3] fastboot: blk: switch emmc to use the block helpers
>> >> >       https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/25ab5c32c28b9f25fb193f726f239d75af3c365a
>> >> > [3/3] fastboot: integrate block flashing back-end
>> >> >       https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/a885bd8c969e25d03bf406207d89b1145c9490fb
>> >> 
>> >> It seems this series cause CI to fail:
>> >> https://source.denx.de/u-boot/custodians/u-boot-dfu/-/pipelines/26238
>> >> 
>> >> Without the patches applied, it passes:
>> >> https://source.denx.de/u-boot/custodians/u-boot-dfu/-/pipelines/26235
>> >> 
>> >> Do you have any idea what is going wrong?
>> >> I could not find anything obvious by skimming through the logs.
>> >
>> > It's a Kconfig problem then. Some platform is prompting for a value (not
>> > a y/n) and there's no default.
>> 
>> You are correct. Thank you for the suggestion.
>> 
>> I've applied the following diff to 3/3:
>> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
>> index 68967abb751e..fdf34a6abe1e 100644
>> --- a/drivers/fastboot/Kconfig
>> +++ b/drivers/fastboot/Kconfig
>> @@ -200,6 +200,7 @@ config FASTBOOT_MMC_USER_NAME
>>  config FASTBOOT_FLASH_BLOCK_INTERFACE_NAME
>>         string "Define FASTBOOT block interface name"
>>         depends on FASTBOOT_FLASH_BLOCK
>> +       default "none"
>>         help
>>           The fastboot "flash" and "erase" commands support operations
>>           on any Block device, this should specify the block device name
>
> Assuming that the code will see "none" and handle the error correctly,
> OK. But we really should have a configured true value here, yes?

The code does not handle any special cases.
If FASTBOOT_FLASH_BLOCK_INTERFACE_NAME is "none", we will call:

blk_get_dev("none", 0);

Which will then be handled via:

	if (!dev_desc) {
		fastboot_fail("no such device", response);
		return -ENODEV;
	}

>
>> @@ -212,6 +213,7 @@ config FASTBOOT_FLASH_BLOCK_INTERFACE_NAME
>>  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
>
> I do not like this at first. If FASTBOOT_FLASH_BLOCK_DEVICE_ID is set,
> there should be a valid ID set too yes? Potentially worse, is 0 a valid
> option here? If so, is that likely to be a real and common one? In that

Yes, 0 is a valid option here. Think of this symbol as a similar one to
FASTBOOT_FLASH_MMC_DEV however it's for generic block device type
(virtio, scsi, ...)

I'd think that 0 is typically the most common value, since it's the
first block controller of a specific type.

> case, we should also be updating the help text to make sure it's clear
> what the normal range is I think.

Ok. That's a bit too much for me to do in a fixup.

Neil, can you send a v4?

>
> -- 
> Tom