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

[neil.armstrong at linaro.org]

On 22/05/2025 08:58, Mattijs Korpershoek wrote:
> Hi Tom,
> 
> On mer., mai 21, 2025 at 13:03, Tom Rini <trini at 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 at 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 at kernel.org> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, 06 May 2025 18:10:06 +0200, neil.armstrong at 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;
> 	}

It could be "" aswell ?

> 
>>
>>> @@ -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.

Yes 0 is a sentible value

> 
>> 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?


Sure

Thanks,
Neil

> 
>>
>> -- 
>> Tom