Re: [PATCH 2/2] file-list: support special 'auto', 'block', 'nvmem' specifiers

[Sascha Hauer <sha@pengutronix.de>]

On Thu, Jun 08, 2023 at 09:24:18AM +0200, Ahmad Fatoum wrote:
> Best practice is for each board to populate $global.system.partitions
> or $global.fastboot.partitions with a string exporting its flashable
> devices in a descriptive manner, e.g. "/dev/mmc0(eMMC),/dev/mmc1(SD)".
> 
> This often goes into BSPs though, so upstream boards are left without
> default partitions, making use a bit cumbersome. Make this easier
> by providing three new magic specifiers:
> 
>   - nvmem: exports all registered NVMEM devices (e.g. EEPROMs, Fuse banks)
>   - block: exports all registered block devices (e.g. eMMC and SD)
>   - auto:  currently equivalent to "nvmem,block". May be extended
>            to raw MTD and UBI in future
> 
> This makes it easy to export devices on any board:
> 
>   usbgadget -A auto -b
> 
> or
> 
>   usbgadget -S auto,/tmp/fitimage(fitimage)c
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  Documentation/user/usb.rst     | 17 +++++++++++++++++
>  common/block.c                 | 16 ++++++++++++++++
>  common/file-list.c             | 29 +++++++++++++++++++++++++++--
>  drivers/nvmem/core.c           | 16 ++++++++++++++++
>  include/block.h                |  6 ++++++
>  include/linux/nvmem-consumer.h |  8 ++++++++
>  6 files changed, 90 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/user/usb.rst b/Documentation/user/usb.rst
> index f2f57ead98d4..6fed0f619b32 100644
> --- a/Documentation/user/usb.rst
> +++ b/Documentation/user/usb.rst
> @@ -73,6 +73,23 @@ Example:
>  
>    /dev/nand0.barebox.bb(barebox)sr,/kernel(kernel)rc
>  
> +Board code authors are encouraged to provide a default environment containing
> +partitions with descriptive names. For boards where this is not specified,
> +there exist a number of **partition** specifiers for automatically generating entries:
> +
> +* ``nvmem`` exports all registered NVMEM devices (e.g. EEPROMs, Fuse banks)

Blindly exporting NVMEM devices is not a good idea. As the description
says it's used for fuse banks. These are write-once and modifying them
is potentially dangerous. Also it's fastboot which means you can't even
read them before modifying them and there is no way to only partially
write them.

There might be good reasons to export some specific NVMEM device, but
then this should be explicitly exported and not by default.

> +* ``block`` exports all registered block devices (e.g. eMMC and SD)
> +* ``auto``  currently equivalent to ``nvmem,block``. May be extended to other flashable
> +            devices, like MTD or UBI volumes in future
> +
> +Example usage of exporting registered block devices, barebox update
> +handlers and a single file that is created on flashing:
> +
> +.. code-block:: sh
> +
> +     detect -a # optional. Detects everything, so auto can register it
> +     usbgadget -A auto,/tmp/fitimage(fitimage)c -b
> +
>  DFU
>  ^^^

[...]

> +static bool file_list_handle_spec(struct file_list *files, const char *spec)
> +{
> +	unsigned count = 0;
> +
> +	if (!strcmp(spec, "auto")) {
> +		count += file_list_add_blockdevs(files);
> +		count += file_list_add_nvmemdevs(files);
> +	} else if (!strcmp(spec, "block")) {
> +		count += file_list_add_blockdevs(files);
> +	} else if (!strcmp(spec, "nvmem")) {
> +		count += file_list_add_nvmemdevs(files);
> +	} else {
> +		return false;
> +	}

Since you are talking about future extensions you could parse "auto"
first and set a flag variable for it:

	bool auto = false;

	if (!strcmp(spec, "auto"))
		auto = true;
	if (!strcmp(spec, "block") || auto)
		count += file_list_add_blockdevs(files);
	if (!strcmp(spec, "nvmem") || auto)
		count += file_list_add_nvmemdevs(files);

That would make it a bit easier to integrate the future extensions into
the code.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |