[PATCH 1/2] file_list: implement file_list_add_cdev_entry

[PATCH 1/2] file_list: implement file_list_add_cdev_entry

[Ahmad Fatoum]

Follow-up commit will add the two users that want to add a cdev,
so let's provide a common file_list_add_cdev_entry they can call.
The benefit of doing that in common/file-list.c is that we can avoid
an extra allocation for prefixing /dev.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/file-list.c  | 21 +++++++++++++++++----
 include/file-list.h |  5 +++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/common/file-list.c b/common/file-list.c
index 1dc7cd8266b1..5c7020111145 100644
--- a/common/file-list.c
+++ b/common/file-list.c
@@ -26,8 +26,8 @@ struct file_list_entry *file_list_entry_by_name(struct file_list *files, const c
 	return NULL;
 }
 
-int file_list_add_entry(struct file_list *files, const char *name, const char *filename,
-			unsigned long flags)
+static int __file_list_add_entry(struct file_list *files, char *name, char *filename,
+				 unsigned long flags)
 {
 	struct file_list_entry *entry;
 
@@ -37,8 +37,8 @@ int file_list_add_entry(struct file_list *files, const char *name, const char *f
 
 	entry = xzalloc(sizeof(*entry));
 
-	entry->name = xstrdup(name);
-	entry->filename = xstrdup(filename);
+	entry->name = name;
+	entry->filename = filename;
 	entry->flags = flags;
 
 	list_add_tail(&entry->list, &files->list);
@@ -46,6 +46,19 @@ int file_list_add_entry(struct file_list *files, const char *name, const char *f
 	return 0;
 }
 
+int file_list_add_entry(struct file_list *files, const char *name, const char *filename,
+			unsigned long flags)
+{
+	return __file_list_add_entry(files, xstrdup(name), xstrdup(filename), flags);
+}
+
+int file_list_add_cdev_entry(struct file_list *files, struct cdev *cdev,
+			     unsigned long flags)
+{
+	return __file_list_add_entry(files, xstrdup(cdev->name),
+				     xasprintf("/dev/%s", cdev->name), flags);
+}
+
 static int file_list_parse_one(struct file_list *files, const char *partstr, const char **endstr)
 {
 	int i = 0, state = PARSE_DEVICE;
diff --git a/include/file-list.h b/include/file-list.h
index 2b2003971f32..79190b0f191d 100644
--- a/include/file-list.h
+++ b/include/file-list.h
@@ -4,6 +4,8 @@
 
 #include <linux/list.h>
 
+struct cdev;
+
 #define FILE_LIST_FLAG_SAFE	(1 << 0)
 #define FILE_LIST_FLAG_READBACK	(1 << 1)
 #define FILE_LIST_FLAG_CREATE	(1 << 2)
@@ -30,6 +32,9 @@ void file_list_free(struct file_list *);
 int file_list_add_entry(struct file_list *files, const char *name, const char *filename,
 			unsigned long flags);
 
+int file_list_add_cdev_entry(struct file_list *files, struct cdev *cdev,
+			     unsigned long flags);
+
 struct file_list *file_list_new(void);
 struct file_list *file_list_dup(struct file_list *old);
 
-- 
2.39.2

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

[Ahmad Fatoum]

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)
+* ``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
 ^^^
 
diff --git a/common/block.c b/common/block.c
index f6eeb7f9c85f..3a4a9fb73149 100644
--- a/common/block.c
+++ b/common/block.c
@@ -11,6 +11,7 @@
 #include <linux/err.h>
 #include <linux/list.h>
 #include <dma.h>
+#include <file-list.h>
 
 #define BLOCKSIZE(blk)	(1 << blk->blockbits)
 
@@ -458,3 +459,18 @@ int block_write(struct block_device *blk, void *buf, sector_t block, blkcnt_t nu
 
 	return ret < 0 ? ret : 0;
 }
+
+unsigned file_list_add_blockdevs(struct file_list *files)
+{
+	struct block_device *blk;
+	unsigned count = 0;
+	int err;
+
+	list_for_each_entry(blk, &block_device_list, list) {
+		err = file_list_add_cdev_entry(files, &blk->cdev, 0);
+		if (!err)
+			count++;
+	}
+
+	return count;
+}
diff --git a/common/file-list.c b/common/file-list.c
index 5c7020111145..80ab108753e0 100644
--- a/common/file-list.c
+++ b/common/file-list.c
@@ -9,6 +9,8 @@
 #include <stringlist.h>
 #include <linux/err.h>
 #include <driver.h>
+#include <block.h>
+#include <linux/nvmem-consumer.h>
 
 #define PARSE_DEVICE	0
 #define PARSE_NAME	1
@@ -59,12 +61,32 @@ int file_list_add_cdev_entry(struct file_list *files, struct cdev *cdev,
 				     xasprintf("/dev/%s", cdev->name), flags);
 }
 
+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;
+	}
+
+	pr_debug("'%s' spcifier resulted in %u entries\n", spec, count);
+	return true;
+}
+
 static int file_list_parse_one(struct file_list *files, const char *partstr, const char **endstr)
 {
 	int i = 0, state = PARSE_DEVICE;
 	char filename[PATH_MAX];
 	char name[PATH_MAX];
 	unsigned long flags = 0;
+	bool special = false;
 
 	memset(filename, 0, sizeof(filename));
 	memset(name, 0, sizeof(name));
@@ -115,7 +137,10 @@ static int file_list_parse_one(struct file_list *files, const char *partstr, con
 		partstr++;
 	}
 
-	if (state != PARSE_FLAGS) {
+	if (state == PARSE_DEVICE)
+		special = file_list_handle_spec(files, filename);
+
+	if (!special && state != PARSE_FLAGS) {
 		pr_err("Missing ')'\n");
 		return -EINVAL;
 	}
@@ -124,7 +149,7 @@ static int file_list_parse_one(struct file_list *files, const char *partstr, con
 		partstr++;
 	*endstr = partstr;
 
-	return file_list_add_entry(files, name, filename, flags);
+	return special ? 0 : file_list_add_entry(files, name, filename, flags);
 }
 
 static const char *flags_to_str(int flags)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index e0110296f87b..6e5fb6f4f5a5 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -12,6 +12,7 @@
 #include <of.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/nvmem-provider.h>
+#include <file-list.h>
 
 struct nvmem_device {
 	const char		*name;
@@ -52,6 +53,21 @@ void nvmem_devices_print(void)
 	}
 }
 
+unsigned file_list_add_nvmemdevs(struct file_list *files)
+{
+	struct nvmem_device *dev;
+	unsigned count = 0;
+	int err;
+
+	list_for_each_entry(dev, &nvmem_devs, node) {
+		err = file_list_add_cdev_entry(files, &dev->cdev, 0);
+		if (!err)
+			count++;
+	}
+
+	return count;
+}
+
 static ssize_t nvmem_cdev_read(struct cdev *cdev, void *buf, size_t count,
 			       loff_t offset, unsigned long flags)
 {
diff --git a/include/block.h b/include/block.h
index da258f509b41..44037bd74c61 100644
--- a/include/block.h
+++ b/include/block.h
@@ -7,6 +7,7 @@
 #include <linux/types.h>
 
 struct block_device;
+struct file_list;
 
 struct block_device_ops {
 	int (*read)(struct block_device *, void *buf, sector_t block, blkcnt_t num_blocks);
@@ -51,11 +52,16 @@ static inline int block_flush(struct block_device *blk)
 
 #ifdef CONFIG_BLOCK
 struct block_device *cdev_get_block_device(const struct cdev *cdev);
+unsigned file_list_add_blockdevs(struct file_list *files);
 #else
 static inline struct block_device *cdev_get_block_device(const struct cdev *cdev)
 {
 	return NULL;
 }
+static inline unsigned file_list_add_blockdevs(struct file_list *files)
+{
+	return 0;
+}
 #endif
 
 static inline bool cdev_is_block_device(const struct cdev *cdev)
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 397c4c29dafd..f215851b3936 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -17,6 +17,7 @@ struct device_node;
 /* consumer cookie */
 struct nvmem_cell;
 struct nvmem_device;
+struct file_list;
 
 struct nvmem_cell_info {
 	const char		*name;
@@ -53,6 +54,8 @@ int nvmem_device_cell_write(struct nvmem_device *nvmem,
 
 void nvmem_devices_print(void);
 
+unsigned file_list_add_nvmemdevs(struct file_list *files);
+
 #else
 
 static inline struct nvmem_cell *nvmem_cell_get(struct device *dev,
@@ -128,6 +131,11 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
 	return -EOPNOTSUPP;
 }
 
+static inline unsigned file_list_add_nvmemdevs(struct file_list *files)
+{
+	return 0;
+}
+
 #endif /* CONFIG_NVMEM */
 
 #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
-- 
2.39.2

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

[Sascha Hauer]

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 |

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

[Ahmad Fatoum]

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 |

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

[Sascha Hauer]

On Fri, Jun 09, 2023 at 09:37:56AM +0200, Ahmad Fatoum wrote:
> 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.

Side note: the fastboot protocol would even allow to do partial writes,
but I don't think there's a way to make use of them with the fastboot
tool.

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 |