[PATCH v3 0/4] Enable env in SCSI

[PATCH v3 0/4] Enable env in SCSI

[Varadarajan Narayanan]

The qcs9100 based Ride platforms have UFS as their primary storage.
Hence add support to U-Boot env framework to be able to save and
retrieve the environment from UFS. The environment will be
saved/retrieved from the partition specified in the config option
CONFIG_SCSI_ENV_PART.

Also add an API to convert partition UUID string to block device
descriptor for UFS. This API will be used to get the block device
descriptor for the partition specified in CONFIG_SCSI_ENV_PART.

v3: s/scsi_get_blk/scsi_get_blk_by_uuid
    Fix argument name in scsi_get_blk_by_uuid

v2: Add part_get_info_by_uuid() similar to part_get_info_by_name()
    Use SCSI instead of UFS
    Use UUID for SCSI_ENV_PART instead of name

Varadarajan Narayanan (4):
  disk: part: implement generic function part_get_info_by_uuid()
  scsi: Implement get_blk() function
  env: Add support for storing env variables in SCSI devices
  configs: qcs9100: Enable env in SCSI

 configs/qcs9100_defconfig  |   5 +-
 disk/part.c                |  37 ++++++++++++
 drivers/scsi/scsi-uclass.c |  30 ++++++++++
 env/Kconfig                |  15 ++++-
 env/Makefile               |   1 +
 env/env.c                  |   3 +
 env/scsi.c                 | 115 +++++++++++++++++++++++++++++++++++++
 include/env_internal.h     |   1 +
 include/part.h             |  20 +++++++
 include/scsi.h             |  10 ++++
 10 files changed, 235 insertions(+), 2 deletions(-)
 create mode 100644 env/scsi.c

-- 
2.34.1

[PATCH v3 1/4] disk: part: implement generic function part_get_info_by_uuid()

[Varadarajan Narayanan]

Add function to search for a partition by UUID as partition
names may not be unique.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 disk/part.c    | 37 +++++++++++++++++++++++++++++++++++++
 include/part.h | 20 ++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/disk/part.c b/disk/part.c
index 303178161c0..abae79a55a7 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -698,6 +698,43 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
 	return -ENOENT;
 }
 
+int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
+			  struct disk_partition *info)
+{
+#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
+	struct part_driver *part_drv;
+	int ret;
+	int i;
+
+	part_drv = part_driver_lookup_type(desc);
+	if (!part_drv)
+		return -1;
+
+	if (!part_drv->get_info) {
+		log_debug("## Driver %s does not have the get_info() method\n",
+			  part_drv->name);
+		return -ENOSYS;
+	}
+
+	for (i = 1; i < part_drv->max_entries; i++) {
+		ret = part_drv->get_info(desc, i, info);
+		if (ret != 0) {
+			/*
+			 * Partition with this index can't be obtained, but
+			 * further partitions might be, so keep checking.
+			 */
+			continue;
+		}
+
+		if (!strncasecmp(uuid, disk_partition_uuid(info), UUID_STR_LEN)) {
+			/* matched */
+			return i;
+		}
+	}
+#endif
+	return -ENOENT;
+}
+
 /**
  * Get partition info from device number and partition name.
  *
diff --git a/include/part.h b/include/part.h
index fcb3c13dea4..3e6eb0ec33f 100644
--- a/include/part.h
+++ b/include/part.h
@@ -315,6 +315,20 @@ int blk_get_device_part_str(const char *ifname, const char *dev_part_str,
 int part_get_info_by_name(struct blk_desc *desc, const char *name,
 			  struct disk_partition *info);
 
+/**
+ * part_get_info_by_uuid() - Search for a partition by uuid
+ *                           among all available registered partitions
+ *
+ * @desc:	block device descriptor
+ * @uuid:	the specified table entry uuid
+ * @info:	the disk partition info
+ *
+ * Return: the partition number on match (starting on 1), -ENOENT on no match,
+ * otherwise error
+ */
+int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
+			  struct disk_partition *info);
+
 /**
  * part_get_info_by_dev_and_name_or_num() - Get partition info from dev number
  *					    and part name, or dev number and
@@ -386,6 +400,12 @@ static inline int part_get_info_by_name(struct blk_desc *desc, const char *name,
 	return -ENOENT;
 }
 
+static inline int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
+					struct disk_partition *info)
+{
+	return -ENOENT;
+}
+
 static inline int
 part_get_info_by_dev_and_name_or_num(const char *dev_iface,
 				     const char *dev_part_str,
-- 
2.34.1

[PATCH v3 2/4] scsi: Implement get_blk() function

[Varadarajan Narayanan]

Add a function to obtain the block device for SCSI.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v3: * s/scsi_get_blk/scsi_get_blk_by_uuid
    * s/partition_name/uuid
---
 drivers/scsi/scsi-uclass.c | 30 ++++++++++++++++++++++++++++++
 include/scsi.h             | 10 ++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/scsi/scsi-uclass.c b/drivers/scsi/scsi-uclass.c
index 1ee8236c05c..3eb6069649f 100644
--- a/drivers/scsi/scsi-uclass.c
+++ b/drivers/scsi/scsi-uclass.c
@@ -10,7 +10,9 @@
 
 #define LOG_CATEGORY UCLASS_SCSI
 
+#include <blk.h>
 #include <dm.h>
+#include <part.h>
 #include <scsi.h>
 
 int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
@@ -23,6 +25,34 @@ int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
 	return ops->exec(dev, pccb);
 }
 
+int scsi_get_blk_by_uuid(const char *uuid,
+			 struct blk_desc **blk_desc_ptr,
+			 struct disk_partition *part_info_ptr)
+{
+	static int is_scsi_scanned;
+	struct blk_desc *blk;
+	int i, ret;
+
+	if (!is_scsi_scanned) {
+		scsi_scan(false /* no verbose */);
+		is_scsi_scanned = 1;
+	}
+
+	for (i = 0; i < blk_find_max_devnum(UCLASS_SCSI) + 1; i++) {
+		ret = blk_get_desc(UCLASS_SCSI, i, &blk);
+		if (ret)
+			continue;
+
+		ret = part_get_info_by_uuid(blk, uuid, part_info_ptr);
+		if (ret > 0) {
+			*blk_desc_ptr = blk;
+			return 0;
+		}
+	}
+
+	return -1;
+}
+
 int scsi_bus_reset(struct udevice *dev)
 {
 	struct scsi_ops *ops = scsi_get_ops(dev);
diff --git a/include/scsi.h b/include/scsi.h
index ab53b47b58f..8d6c5116419 100644
--- a/include/scsi.h
+++ b/include/scsi.h
@@ -351,6 +351,16 @@ int scsi_scan(bool verbose);
  */
 int scsi_scan_dev(struct udevice *dev, bool verbose);
 
+/**
+ * scsi_get_blk_by_uuid() - Provides SCSI partition information.
+ *
+ * @uuid:		UUID of the partition for fetching its info
+ * @blk_desc_ptr:	Provides the blk descriptor
+ * @part_info_ptr:	Provides partition info
+ */
+int scsi_get_blk_by_uuid(const char *uuid, struct blk_desc **blk_desc_ptr,
+			 struct disk_partition *part_info_ptr);
+
 #define SCSI_IDENTIFY					0xC0  /* not used */
 
 /* Hardware errors  */
-- 
2.34.1

[PATCH v3 3/4] env: Add support for storing env variables in SCSI devices

[Varadarajan Narayanan]

Allow SCSI to be able to store environment variables.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v3: s/SCSI_ENV_PART/SCSI_ENV_PART_UUID
---
 env/Kconfig            |  15 +++++-
 env/Makefile           |   1 +
 env/env.c              |   3 ++
 env/scsi.c             | 115 +++++++++++++++++++++++++++++++++++++++++
 include/env_internal.h |   1 +
 5 files changed, 134 insertions(+), 1 deletion(-)
 create mode 100644 env/scsi.c

diff --git a/env/Kconfig b/env/Kconfig
index 9f5ec44601e..9081f7099b7 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -74,7 +74,7 @@ config ENV_IS_DEFAULT
 		     !ENV_IS_IN_MMC && !ENV_IS_IN_NAND && \
 		     !ENV_IS_IN_NVRAM && !ENV_IS_IN_ONENAND && \
 		     !ENV_IS_IN_REMOTE && !ENV_IS_IN_SPI_FLASH && \
-		     !ENV_IS_IN_UBI && !ENV_IS_IN_MTD
+		     !ENV_IS_IN_UBI && !ENV_IS_IN_MTD && !ENV_IS_IN_SCSI
 	select ENV_IS_NOWHERE
 
 config ENV_IS_NOWHERE
@@ -297,6 +297,13 @@ config ENV_IS_IN_NAND
 	  Currently, CONFIG_ENV_OFFSET_REDUND is not supported when
 	  using CONFIG_ENV_OFFSET_OOB.
 
+config ENV_IS_IN_SCSI
+	bool "Environment in an SCSI device"
+	depends on SCSI
+	help
+	  Define this if you have an SCSI device which you want to use for the
+	  environment.
+
 config ENV_RANGE
 	hex "Length of the region in which the environment can be written"
 	depends on ENV_IS_IN_NAND
@@ -731,6 +738,12 @@ config ENV_MMC_USE_DT
 	  The 2 defines CONFIG_ENV_OFFSET, CONFIG_ENV_OFFSET_REDUND
 	  are not used as fallback.
 
+config SCSI_ENV_PART_UUID
+	string "SCSI partition UUID for saving environment"
+	depends on ENV_IS_IN_SCSI
+	help
+	  UUID of the SCSI partition that you want to store the environment in.
+
 config USE_DEFAULT_ENV_FILE
 	bool "Create default environment from file"
 	help
diff --git a/env/Makefile b/env/Makefile
index 3b9c71d5681..d11b87702c1 100644
--- a/env/Makefile
+++ b/env/Makefile
@@ -28,5 +28,6 @@ obj-$(CONFIG_$(PHASE_)ENV_IS_IN_NAND) += nand.o
 obj-$(CONFIG_$(PHASE_)ENV_IS_IN_SPI_FLASH) += sf.o
 obj-$(CONFIG_$(PHASE_)ENV_IS_IN_MTD) += mtd.o
 obj-$(CONFIG_$(PHASE_)ENV_IS_IN_FLASH) += flash.o
+obj-$(CONFIG_$(PHASE_)ENV_IS_IN_SCSI) += scsi.o
 
 CFLAGS_embedded.o := -Wa,--no-warn -DENV_CRC=$(shell tools/envcrc 2>/dev/null)
diff --git a/env/env.c b/env/env.c
index dbaeedc3c3b..7a9c96b4078 100644
--- a/env/env.c
+++ b/env/env.c
@@ -46,6 +46,9 @@ static enum env_location env_locations[] = {
 #ifdef CONFIG_ENV_IS_IN_MMC
 	ENVL_MMC,
 #endif
+#ifdef CONFIG_ENV_IS_IN_SCSI
+	ENVL_SCSI,
+#endif
 #ifdef CONFIG_ENV_IS_IN_NAND
 	ENVL_NAND,
 #endif
diff --git a/env/scsi.c b/env/scsi.c
new file mode 100644
index 00000000000..207717e17b1
--- /dev/null
+++ b/env/scsi.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2008-2011 Freescale Semiconductor, Inc.
+ */
+
+/* #define DEBUG */
+
+#include <asm/global_data.h>
+
+#include <command.h>
+#include <env.h>
+#include <env_internal.h>
+#include <fdtdec.h>
+#include <linux/stddef.h>
+#include <malloc.h>
+#include <memalign.h>
+#include <part.h>
+#include <search.h>
+#include <scsi.h>
+#include <errno.h>
+#include <dm/ofnode.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+static env_t envbuf;
+
+struct env_scsi_info {
+	struct blk_desc *blk;
+	struct disk_partition part;
+	int count;
+};
+
+static struct env_scsi_info env_part;
+
+static inline struct env_scsi_info *env_scsi_get_part(void)
+{
+	struct env_scsi_info *ep = &env_part;
+
+	if (scsi_get_blk_by_uuid(CONFIG_SCSI_ENV_PART_UUID, &ep->blk, &ep->part))
+		return NULL;
+
+	ep->count = CONFIG_ENV_SIZE / ep->part.blksz;
+
+	return ep;
+}
+
+static int env_scsi_save(void)
+{
+	struct env_scsi_info *ep = env_scsi_get_part();
+	int ret;
+
+	if (!ep)
+		return -ENOENT;
+
+	ret = env_export(&envbuf);
+	if (ret)
+		return ret;
+
+	if (blk_dwrite(ep->blk, ep->part.start, ep->count, &envbuf) != ep->count)
+		return -EIO;
+
+	return 0;
+}
+
+static int env_scsi_erase(void)
+{
+	struct env_scsi_info *ep = env_scsi_get_part();
+
+	if (!ep)
+		return -ENOENT;
+
+	return (int)blk_derase(ep->blk, ep->part.start, ep->count);
+}
+
+#if defined(ENV_IS_EMBEDDED)
+static int env_scsi_load(void)
+{
+	return 0;
+}
+#else
+static int env_scsi_load(void)
+{
+	struct env_scsi_info *ep = env_scsi_get_part();
+	int ret;
+
+	if (!ep) {
+		env_set_default(CONFIG_SCSI_ENV_PART_UUID " partition not found", 0);
+		return -ENOENT;
+	}
+
+	if (blk_dread(ep->blk, ep->part.start, ep->count, &envbuf) != ep->count) {
+		env_set_default(CONFIG_SCSI_ENV_PART_UUID " partition read failed", 0);
+		return -EIO;
+	}
+
+	ret = env_import((char *)&envbuf, 1, H_EXTERNAL);
+	if (ret) {
+		debug("ENV import failed\n");
+		env_set_default("Cannot load environment", 0);
+	} else {
+		gd->env_addr = (ulong)envbuf.data;
+	}
+
+	return ret;
+}
+#endif
+
+U_BOOT_ENV_LOCATION(scsi) = {
+	.location	= ENVL_SCSI,
+	ENV_NAME("SCSI")
+	.load		= env_scsi_load,
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_XPL_BUILD)
+	.save		= env_save_ptr(env_scsi_save),
+	.erase		= ENV_ERASE_PTR(env_scsi_erase),
+#endif
+};
diff --git a/include/env_internal.h b/include/env_internal.h
index ee939ba4293..75b46d0bcb0 100644
--- a/include/env_internal.h
+++ b/include/env_internal.h
@@ -115,6 +115,7 @@ enum env_location {
 	ENVL_SPI_FLASH,
 	ENVL_MTD,
 	ENVL_UBI,
+	ENVL_SCSI,
 	ENVL_NOWHERE,
 
 	ENVL_COUNT,
-- 
2.34.1

[PATCH v3 4/4] configs: qcs9100: Enable env in SCSI

[Varadarajan Narayanan]

Enable CONFIG_ENV_IS_IN_SCSI to store environment variables in SCSI.
Set env variables partition UUID as seen in qcs9100 based boards.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v3: s/CONFIG_SCSI_ENV_PART/CONFIG_SCSI_ENV_PART_UUID
---
 configs/qcs9100_defconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/configs/qcs9100_defconfig b/configs/qcs9100_defconfig
index 10ff4d25398..cd48973599b 100644
--- a/configs/qcs9100_defconfig
+++ b/configs/qcs9100_defconfig
@@ -14,5 +14,8 @@ CONFIG_DEBUG_UART_CLOCK=14745600
 # Address where U-Boot will be loaded
 CONFIG_TEXT_BASE=0xaf000000
 CONFIG_REMAKE_ELF=y
-
 CONFIG_DEFAULT_DEVICE_TREE="qcom/qcs9100-ride-r3"
+CONFIG_ENV_IS_IN_SCSI=y
+CONFIG_SCSI_ENV_PART_UUID="71cb9cd0-acf1-b6cb-ad91-be9572fe11a9"
+# CONFIG_ENV_IS_DEFAULT is not set
+# CONFIG_ENV_IS_NOWHERE is not set
-- 
2.34.1

Re: [PATCH v3 4/4] configs: qcs9100: Enable env in SCSI

[Casey Connolly]

Hi Varadarajan,

On 5/7/25 13:28, Varadarajan Narayanan wrote:
> Enable CONFIG_ENV_IS_IN_SCSI to store environment variables in SCSI.
> Set env variables partition UUID as seen in qcs9100 based boards.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v3: s/CONFIG_SCSI_ENV_PART/CONFIG_SCSI_ENV_PART_UUID
> ---
>   configs/qcs9100_defconfig | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/configs/qcs9100_defconfig b/configs/qcs9100_defconfig
> index 10ff4d25398..cd48973599b 100644
> --- a/configs/qcs9100_defconfig
> +++ b/configs/qcs9100_defconfig
> @@ -14,5 +14,8 @@ CONFIG_DEBUG_UART_CLOCK=14745600
>   # Address where U-Boot will be loaded
>   CONFIG_TEXT_BASE=0xaf000000
>   CONFIG_REMAKE_ELF=y
> -
>   CONFIG_DEFAULT_DEVICE_TREE="qcom/qcs9100-ride-r3"
> +CONFIG_ENV_IS_IN_SCSI=y
> +CONFIG_SCSI_ENV_PART_UUID="71cb9cd0-acf1-b6cb-ad91-be9572fe11a9"

Could you also update the RDP documentation to describe firstly that env 
is loaded from a partition, and then reference this GUID and which 
partition it is.

As Tom mentioned before, doing it on FAT would make it much easier to 
tinker with, but I guess it's better to match what you're doing in 
production, just as long as it's documented...

Kind regards,> +# CONFIG_ENV_IS_DEFAULT is not set
> +# CONFIG_ENV_IS_NOWHERE is not set
-- 
Casey (she/they)

Re: [PATCH v3 1/4] disk: part: implement generic function part_get_info_by_uuid()

[Casey Connolly]

Hi Varadarajan,

On 5/7/25 13:28, Varadarajan Narayanan wrote:
> Add function to search for a partition by UUID as partition
> names may not be unique.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>   disk/part.c    | 37 +++++++++++++++++++++++++++++++++++++
>   include/part.h | 20 ++++++++++++++++++++
>   2 files changed, 57 insertions(+)
> 
> diff --git a/disk/part.c b/disk/part.c
> index 303178161c0..abae79a55a7 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -698,6 +698,43 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
>   	return -ENOENT;
>   }
>   
> +int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
> +			  struct disk_partition *info)
> +{
> +#if CONFIG_IS_ENABLED(PARTITION_UUIDS)

Please refer to checkpatch output (the b4 tool will run this for you 
automatically).

	if (!CONFIG_IS_ENABLED(PARTITION_UUIDS))
		return -ENOENT;

> +	struct part_driver *part_drv;
> +	int ret;
> +	int i;
> +
> +	part_drv = part_driver_lookup_type(desc);
> +	if (!part_drv)
> +		return -1;
> +
> +	if (!part_drv->get_info) {
> +		log_debug("## Driver %s does not have the get_info() method\n",
> +			  part_drv->name);
> +		return -ENOSYS;
> +	}
> +
> +	for (i = 1; i < part_drv->max_entries; i++) {
> +		ret = part_drv->get_info(desc, i, info);
> +		if (ret != 0) {
> +			/*
> +			 * Partition with this index can't be obtained, but
> +			 * further partitions might be, so keep checking.
> +			 */
> +			continue;

For part_efi (which is the only one relevant here I think?) you can 
check for -EPERM which signals the end of the partition list I believe. 
Then we aren't calling get_info() 128 times for every LUN...

Kind regards,> +		}
> +
> +		if (!strncasecmp(uuid, disk_partition_uuid(info), UUID_STR_LEN)) {
> +			/* matched */
> +			return i;
> +		}
> +	}
> +#endif
> +	return -ENOENT;
> +}
> +
>   /**
>    * Get partition info from device number and partition name.
>    *
> diff --git a/include/part.h b/include/part.h
> index fcb3c13dea4..3e6eb0ec33f 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -315,6 +315,20 @@ int blk_get_device_part_str(const char *ifname, const char *dev_part_str,
>   int part_get_info_by_name(struct blk_desc *desc, const char *name,
>   			  struct disk_partition *info);
>   
> +/**
> + * part_get_info_by_uuid() - Search for a partition by uuid
> + *                           among all available registered partitions
> + *
> + * @desc:	block device descriptor
> + * @uuid:	the specified table entry uuid
> + * @info:	the disk partition info
> + *
> + * Return: the partition number on match (starting on 1), -ENOENT on no match,
> + * otherwise error
> + */
> +int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
> +			  struct disk_partition *info);
> +
>   /**
>    * part_get_info_by_dev_and_name_or_num() - Get partition info from dev number
>    *					    and part name, or dev number and
> @@ -386,6 +400,12 @@ static inline int part_get_info_by_name(struct blk_desc *desc, const char *name,
>   	return -ENOENT;
>   }
>   
> +static inline int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
> +					struct disk_partition *info)
> +{
> +	return -ENOENT;
> +}
> +
>   static inline int
>   part_get_info_by_dev_and_name_or_num(const char *dev_iface,
>   				     const char *dev_part_str,

-- 
Casey (she/they)

Re: [PATCH v3 2/4] scsi: Implement get_blk() function

[Casey Connolly]

On 5/7/25 13:28, Varadarajan Narayanan wrote:
> Add a function to obtain the block device for SCSI.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v3: * s/scsi_get_blk/scsi_get_blk_by_uuid
>      * s/partition_name/uuid
> ---
>   drivers/scsi/scsi-uclass.c | 30 ++++++++++++++++++++++++++++++
>   include/scsi.h             | 10 ++++++++++
>   2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/scsi/scsi-uclass.c b/drivers/scsi/scsi-uclass.c
> index 1ee8236c05c..3eb6069649f 100644
> --- a/drivers/scsi/scsi-uclass.c
> +++ b/drivers/scsi/scsi-uclass.c
> @@ -10,7 +10,9 @@
>   
>   #define LOG_CATEGORY UCLASS_SCSI
>   
> +#include <blk.h>
>   #include <dm.h>
> +#include <part.h>
>   #include <scsi.h>
>   
>   int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
> @@ -23,6 +25,34 @@ int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
>   	return ops->exec(dev, pccb);
>   }
>   
> +int scsi_get_blk_by_uuid(const char *uuid,
> +			 struct blk_desc **blk_desc_ptr,
> +			 struct disk_partition *part_info_ptr)
> +{
> +	static int is_scsi_scanned;
> +	struct blk_desc *blk;
> +	int i, ret;
> +
> +	if (!is_scsi_scanned) {
> +		scsi_scan(false /* no verbose */);
> +		is_scsi_scanned = 1;

At this point I think we should just unconditionally scsi_scan() like we 
do with MMC... Tom (others?) any thoughts on this? Anyhow this is 
probably fine.

One thing to be extremely wary of is that we don't call scsi_scan() 
after initialising EFI driver handles because that will cause big issues 
(ran into this one before), since scsi_scan() will unbind and rebind 
drivers if it was already called. I don't think that's a problem with 
the current ordering of things.

> +	}
> +
> +	for (i = 0; i < blk_find_max_devnum(UCLASS_SCSI) + 1; i++) {
> +		ret = blk_get_desc(UCLASS_SCSI, i, &blk);
> +		if (ret)
> +			continue;

As before, please check for the end of the list and break here (-ENODEV 
is the magic value). Really it's bad practise to ignore ret, we should 
only ever continue if ret is -ENOENT, otherwise we should return the 
errorcode (and probably translate a -ENODEV to -ENOENT since that makes 
more sense here)

Kind regards,

> +
> +		ret = part_get_info_by_uuid(blk, uuid, part_info_ptr);
> +		if (ret > 0) {
> +			*blk_desc_ptr = blk;
> +			return 0;
> +		}
> +	}
> +
> +	return -1;

	return -ENOENT;

Kind regards,> +}
> +
>   int scsi_bus_reset(struct udevice *dev)
>   {
>   	struct scsi_ops *ops = scsi_get_ops(dev);
> diff --git a/include/scsi.h b/include/scsi.h
> index ab53b47b58f..8d6c5116419 100644
> --- a/include/scsi.h
> +++ b/include/scsi.h
> @@ -351,6 +351,16 @@ int scsi_scan(bool verbose);
>    */
>   int scsi_scan_dev(struct udevice *dev, bool verbose);
>   
> +/**
> + * scsi_get_blk_by_uuid() - Provides SCSI partition information.
> + *
> + * @uuid:		UUID of the partition for fetching its info
> + * @blk_desc_ptr:	Provides the blk descriptor
> + * @part_info_ptr:	Provides partition info
> + */
> +int scsi_get_blk_by_uuid(const char *uuid, struct blk_desc **blk_desc_ptr,
> +			 struct disk_partition *part_info_ptr);
> +
>   #define SCSI_IDENTIFY					0xC0  /* not used */
>   
>   /* Hardware errors  */
-- 
Casey (she/they)

Re: [PATCH v3 2/4] scsi: Implement get_blk() function

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

On Wed, May 07, 2025 at 01:57:11PM +0200, Casey Connolly wrote:
> 
> 
> On 5/7/25 13:28, Varadarajan Narayanan wrote:
> > Add a function to obtain the block device for SCSI.
> > 
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > v3: * s/scsi_get_blk/scsi_get_blk_by_uuid
> >      * s/partition_name/uuid
> > ---
> >   drivers/scsi/scsi-uclass.c | 30 ++++++++++++++++++++++++++++++
> >   include/scsi.h             | 10 ++++++++++
> >   2 files changed, 40 insertions(+)
> > 
> > diff --git a/drivers/scsi/scsi-uclass.c b/drivers/scsi/scsi-uclass.c
> > index 1ee8236c05c..3eb6069649f 100644
> > --- a/drivers/scsi/scsi-uclass.c
> > +++ b/drivers/scsi/scsi-uclass.c
> > @@ -10,7 +10,9 @@
> >   #define LOG_CATEGORY UCLASS_SCSI
> > +#include <blk.h>
> >   #include <dm.h>
> > +#include <part.h>
> >   #include <scsi.h>
> >   int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
> > @@ -23,6 +25,34 @@ int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
> >   	return ops->exec(dev, pccb);
> >   }
> > +int scsi_get_blk_by_uuid(const char *uuid,
> > +			 struct blk_desc **blk_desc_ptr,
> > +			 struct disk_partition *part_info_ptr)
> > +{
> > +	static int is_scsi_scanned;
> > +	struct blk_desc *blk;
> > +	int i, ret;
> > +
> > +	if (!is_scsi_scanned) {
> > +		scsi_scan(false /* no verbose */);
> > +		is_scsi_scanned = 1;
> 
> At this point I think we should just unconditionally scsi_scan() like we do
> with MMC... Tom (others?) any thoughts on this? Anyhow this is probably
> fine.

It's certainly something to talk about stand-alone, yeah.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 1/4] disk: part: implement generic function part_get_info_by_uuid()

[Varadarajan Narayanan]

On Wed, May 07, 2025 at 01:47:40PM +0200, Casey Connolly wrote:
> Hi Varadarajan,
>
> On 5/7/25 13:28, Varadarajan Narayanan wrote:
> > Add function to search for a partition by UUID as partition
> > names may not be unique.
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >   disk/part.c    | 37 +++++++++++++++++++++++++++++++++++++
> >   include/part.h | 20 ++++++++++++++++++++
> >   2 files changed, 57 insertions(+)
> >
> > diff --git a/disk/part.c b/disk/part.c
> > index 303178161c0..abae79a55a7 100644
> > --- a/disk/part.c
> > +++ b/disk/part.c
> > @@ -698,6 +698,43 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
> >   	return -ENOENT;
> >   }
> > +int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
> > +			  struct disk_partition *info)
> > +{
> > +#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
>
> Please refer to checkpatch output (the b4 tool will run this for you
> automatically).

Ok

> 	if (!CONFIG_IS_ENABLED(PARTITION_UUIDS))
> 		return -ENOENT;
>
> > +	struct part_driver *part_drv;
> > +	int ret;
> > +	int i;
> > +
> > +	part_drv = part_driver_lookup_type(desc);
> > +	if (!part_drv)
> > +		return -1;
> > +
> > +	if (!part_drv->get_info) {
> > +		log_debug("## Driver %s does not have the get_info() method\n",
> > +			  part_drv->name);
> > +		return -ENOSYS;
> > +	}
> > +
> > +	for (i = 1; i < part_drv->max_entries; i++) {
> > +		ret = part_drv->get_info(desc, i, info);
> > +		if (ret != 0) {
> > +			/*
> > +			 * Partition with this index can't be obtained, but
> > +			 * further partitions might be, so keep checking.
> > +			 */
> > +			continue;
>
> For part_efi (which is the only one relevant here I think?) you can check
> for -EPERM which signals the end of the partition list I believe. Then we
> aren't calling get_info() 128 times for every LUN...

This is a replica of part_get_info_by_name() - [1]. Not sure why it has
it that way and didn't want to miss corner cases (if any). Please let
me know if still you would prefer to change to check for -EPERM.

Thanks
Varada

1 - https://source.denx.de/u-boot/u-boot/-/blob/master/disk/part.c?ref_type=heads#L685

> +		}
> > +
> > +		if (!strncasecmp(uuid, disk_partition_uuid(info), UUID_STR_LEN)) {
> > +			/* matched */
> > +			return i;
> > +		}
> > +	}
> > +#endif
> > +	return -ENOENT;
> > +}
> > +
> >   /**
> >    * Get partition info from device number and partition name.
> >    *
> > diff --git a/include/part.h b/include/part.h
> > index fcb3c13dea4..3e6eb0ec33f 100644
> > --- a/include/part.h
> > +++ b/include/part.h
> > @@ -315,6 +315,20 @@ int blk_get_device_part_str(const char *ifname, const char *dev_part_str,
> >   int part_get_info_by_name(struct blk_desc *desc, const char *name,
> >   			  struct disk_partition *info);
> > +/**
> > + * part_get_info_by_uuid() - Search for a partition by uuid
> > + *                           among all available registered partitions
> > + *
> > + * @desc:	block device descriptor
> > + * @uuid:	the specified table entry uuid
> > + * @info:	the disk partition info
> > + *
> > + * Return: the partition number on match (starting on 1), -ENOENT on no match,
> > + * otherwise error
> > + */
> > +int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
> > +			  struct disk_partition *info);
> > +
> >   /**
> >    * part_get_info_by_dev_and_name_or_num() - Get partition info from dev number
> >    *					    and part name, or dev number and
> > @@ -386,6 +400,12 @@ static inline int part_get_info_by_name(struct blk_desc *desc, const char *name,
> >   	return -ENOENT;
> >   }
> > +static inline int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
> > +					struct disk_partition *info)
> > +{
> > +	return -ENOENT;
> > +}
> > +
> >   static inline int
> >   part_get_info_by_dev_and_name_or_num(const char *dev_iface,
> >   				     const char *dev_part_str,
>
> --
> Casey (she/they)
>

Re: [PATCH v3 1/4] disk: part: implement generic function part_get_info_by_uuid()

[Casey Connolly]

On 5/8/25 07:23, Varadarajan Narayanan wrote:
> On Wed, May 07, 2025 at 01:47:40PM +0200, Casey Connolly wrote:
>> Hi Varadarajan,
>>
>> On 5/7/25 13:28, Varadarajan Narayanan wrote:
>>> Add function to search for a partition by UUID as partition
>>> names may not be unique.
>>>
>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>>> ---
>>>    disk/part.c    | 37 +++++++++++++++++++++++++++++++++++++
>>>    include/part.h | 20 ++++++++++++++++++++
>>>    2 files changed, 57 insertions(+)
>>>
>>> diff --git a/disk/part.c b/disk/part.c
>>> index 303178161c0..abae79a55a7 100644
>>> --- a/disk/part.c
>>> +++ b/disk/part.c
>>> @@ -698,6 +698,43 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
>>>    	return -ENOENT;
>>>    }
>>> +int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
>>> +			  struct disk_partition *info)
>>> +{
>>> +#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
>>
>> Please refer to checkpatch output (the b4 tool will run this for you
>> automatically).
> 
> Ok
> 
>> 	if (!CONFIG_IS_ENABLED(PARTITION_UUIDS))
>> 		return -ENOENT;
>>
>>> +	struct part_driver *part_drv;
>>> +	int ret;
>>> +	int i;
>>> +
>>> +	part_drv = part_driver_lookup_type(desc);
>>> +	if (!part_drv)
>>> +		return -1;
>>> +
>>> +	if (!part_drv->get_info) {
>>> +		log_debug("## Driver %s does not have the get_info() method\n",
>>> +			  part_drv->name);
>>> +		return -ENOSYS;
>>> +	}
>>> +
>>> +	for (i = 1; i < part_drv->max_entries; i++) {
>>> +		ret = part_drv->get_info(desc, i, info);
>>> +		if (ret != 0) {
>>> +			/*
>>> +			 * Partition with this index can't be obtained, but
>>> +			 * further partitions might be, so keep checking.
>>> +			 */
>>> +			continue;
>>
>> For part_efi (which is the only one relevant here I think?) you can check
>> for -EPERM which signals the end of the partition list I believe. Then we
>> aren't calling get_info() 128 times for every LUN...
> 
> This is a replica of part_get_info_by_name() - [1]. Not sure why it has
> it that way and didn't want to miss corner cases (if any). Please let
> me know if still you would prefer to change to check for -EPERM.

Unfortunately the get_info callback doesn't document the return value :(

I think it would be nice to clean this up, and document a return value 
for get_info to save a bit of time but I guess this is fine with the API 
being so poorly defined :(

Kind regards,>
> Thanks
> Varada
> 
> 1 - https://source.denx.de/u-boot/u-boot/-/blob/master/disk/part.c?ref_type=heads#L685
> 
>> +		}
>>> +
>>> +		if (!strncasecmp(uuid, disk_partition_uuid(info), UUID_STR_LEN)) {
>>> +			/* matched */
>>> +			return i;
>>> +		}
>>> +	}
>>> +#endif
>>> +	return -ENOENT;
>>> +}
>>> +
>>>    /**
>>>     * Get partition info from device number and partition name.
>>>     *
>>> diff --git a/include/part.h b/include/part.h
>>> index fcb3c13dea4..3e6eb0ec33f 100644
>>> --- a/include/part.h
>>> +++ b/include/part.h
>>> @@ -315,6 +315,20 @@ int blk_get_device_part_str(const char *ifname, const char *dev_part_str,
>>>    int part_get_info_by_name(struct blk_desc *desc, const char *name,
>>>    			  struct disk_partition *info);
>>> +/**
>>> + * part_get_info_by_uuid() - Search for a partition by uuid
>>> + *                           among all available registered partitions
>>> + *
>>> + * @desc:	block device descriptor
>>> + * @uuid:	the specified table entry uuid
>>> + * @info:	the disk partition info
>>> + *
>>> + * Return: the partition number on match (starting on 1), -ENOENT on no match,
>>> + * otherwise error
>>> + */
>>> +int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
>>> +			  struct disk_partition *info);
>>> +
>>>    /**
>>>     * part_get_info_by_dev_and_name_or_num() - Get partition info from dev number
>>>     *					    and part name, or dev number and
>>> @@ -386,6 +400,12 @@ static inline int part_get_info_by_name(struct blk_desc *desc, const char *name,
>>>    	return -ENOENT;
>>>    }
>>> +static inline int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
>>> +					struct disk_partition *info)
>>> +{
>>> +	return -ENOENT;
>>> +}
>>> +
>>>    static inline int
>>>    part_get_info_by_dev_and_name_or_num(const char *dev_iface,
>>>    				     const char *dev_part_str,
>>
>> --
>> Casey (she/they)
>>
-- 
Casey (she/they)