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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 09.06.23 09:29, Sascha Hauer wrote:
> 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.

The few fuse bank drivers that can actually blow fuses have a device
variable that needs to be written to unlock write access.

But yes, the inability to write them partially makes the write
useless. I will just resend without nvmem.

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

Ok.

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