[PATCH 0/2] Add support to extract the image versions beyond the first 32 images

[PATCH 0/2] Add support to extract the image versions beyond the first 32 images

[Kathiravan Thirumoorthy]

SMEM item 667 contains the image version details beyond the first 32 images.
Add support for the same. While at it, introduce the new API
qcom_smem_validate_item() and use it before getting the image version
from item 667 to avoid the invalid item warning.

IPQ platforms doesn't have the SMEM item 667. So I couldn't validate the
image version details from SMEM item 667.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Kathiravan Thirumoorthy (2):
      soc: qcom: smem: introduce qcom_smem_validate_item API
      soc: qcom: socinfo: add support to extract more than 32 image versions

 drivers/soc/qcom/smem.c       | 16 +++++++++++++--
 drivers/soc/qcom/socinfo.c    | 46 +++++++++++++++++++++++++++++++++++--------
 include/linux/soc/qcom/smem.h |  1 +
 3 files changed, 53 insertions(+), 10 deletions(-)
---
base-commit: 131f3d9446a6075192cdd91f197989d98302faa6
change-id: 20251028-image-crm-part2-1f6bbefea2d8

Best regards,
-- 
Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>

[PATCH 1/2] soc: qcom: smem: introduce qcom_smem_validate_item API

[Kathiravan Thirumoorthy]

When a SMEM item is allocated or retrieved, sanity check on the SMEM item
is performed and backtrace is printed if the SMEM item is invalid.

Image version table in SMEM contains version details for the first 32
images. Beyond that, another SMEM item 667 is being used, which may not
be defined in all the platforms. So directly retrieving the SMEM item 667,
throws the warning as invalid item number.

To handle such cases, introduce a new API to validate the SMEM item before
processing it. While at it, make use of this API in the SMEM driver where
possible.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
 drivers/soc/qcom/smem.c       | 16 ++++++++++++++--
 include/linux/soc/qcom/smem.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index c4c45f15dca4fb14f97df4ad494c1189e4f098bd..8a0a832f1e9915b2177a0fe08298ffe8a779e516 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -396,6 +396,18 @@ bool qcom_smem_is_available(void)
 }
 EXPORT_SYMBOL_GPL(qcom_smem_is_available);
 
+/**
+ * qcom_smem_validate_item() - Check if SMEM item is within the limit
+ * @item:	SMEM item to validate
+ *
+ * Return: true if SMEM item is valid, false otherwise.
+ */
+bool qcom_smem_validate_item(unsigned item)
+{
+	return item < __smem->item_count;
+}
+EXPORT_SYMBOL_GPL(qcom_smem_validate_item);
+
 static int qcom_smem_alloc_private(struct qcom_smem *smem,
 				   struct smem_partition *part,
 				   unsigned item,
@@ -517,7 +529,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 		return -EINVAL;
 	}
 
-	if (WARN_ON(item >= __smem->item_count))
+	if (WARN_ON(!qcom_smem_validate_item(item)))
 		return -EINVAL;
 
 	ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
@@ -690,7 +702,7 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
 	if (!__smem)
 		return ptr;
 
-	if (WARN_ON(item >= __smem->item_count))
+	if (WARN_ON(!qcom_smem_validate_item(item)))
 		return ERR_PTR(-EINVAL);
 
 	if (host < SMEM_HOST_COUNT && __smem->partitions[host].virt_base) {
diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
index f946e3beca215548ac56dbf779138d05479712f5..05891532d530a25747afb8dc96ad4ba668598197 100644
--- a/include/linux/soc/qcom/smem.h
+++ b/include/linux/soc/qcom/smem.h
@@ -5,6 +5,7 @@
 #define QCOM_SMEM_HOST_ANY -1
 
 bool qcom_smem_is_available(void);
+bool qcom_smem_validate_item(unsigned item);
 int qcom_smem_alloc(unsigned host, unsigned item, size_t size);
 void *qcom_smem_get(unsigned host, unsigned item, size_t *size);
 

-- 
2.34.1

[PATCH 2/2] soc: qcom: socinfo: add support to extract more than 32 image versions

[Kathiravan Thirumoorthy]

SMEM_IMAGE_VERSION_TABLE contains the version of the first 32 images.
Add images beyond that and read these from SMEM_IMAGE_VERSION_TABLE_2.

Not all platforms define the SMEM item number 667, in that case
qcom_smem_get() will throw the invalid item warning. To avoid that,
validate the SMEM item before fetching the version details.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
 drivers/soc/qcom/socinfo.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 4fd09e2bfd021424b9489cd29eec29dc7c7a16d3..f832ae36942b10f68f0c3304f98d946796e8d1bd 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -67,7 +67,17 @@
 #define SMEM_IMAGE_TABLE_GEARVM_INDEX	29
 #define SMEM_IMAGE_TABLE_UEFI_INDEX	30
 #define SMEM_IMAGE_TABLE_CDSP3_INDEX	31
+#define SMEM_IMAGE_TABLE_AUDIOPD_ADSP1_INDEX	32
+#define SMEM_IMAGE_TABLE_AUDIOPD_ADSP2_INDEX	33
+#define SMEM_IMAGE_TABLE_DCP_INDEX	34
+#define SMEM_IMAGE_TABLE_OOBS_INDEX	35
+#define SMEM_IMAGE_TABLE_OOBNS_INDEX	36
+#define SMEM_IMAGE_TABLE_DEVCFG_INDEX	37
+#define SMEM_IMAGE_TABLE_BTPD_INDEX	38
+#define SMEM_IMAGE_TABLE_QECP_INDEX	39
+
 #define SMEM_IMAGE_VERSION_TABLE       469
+#define SMEM_IMAGE_VERSION_TABLE_2	667
 
 /*
  * SMEM Image table names
@@ -79,13 +89,18 @@ static const char *const socinfo_image_names[] = {
 	[SMEM_IMAGE_TABLE_APPSBL_INDEX] = "appsbl",
 	[SMEM_IMAGE_TABLE_APPS_INDEX] = "apps",
 	[SMEM_IMAGE_TABLE_AUDIOPD_INDEX] = "audiopd",
+	[SMEM_IMAGE_TABLE_AUDIOPD_ADSP1_INDEX] = "audiopd_adsp1",
+	[SMEM_IMAGE_TABLE_AUDIOPD_ADSP2_INDEX] = "audiopd_adsp2",
 	[SMEM_IMAGE_TABLE_BOOT_INDEX] = "boot",
+	[SMEM_IMAGE_TABLE_BTPD_INDEX] = "btpd",
 	[SMEM_IMAGE_TABLE_CDSP1_INDEX] = "cdsp1",
 	[SMEM_IMAGE_TABLE_CDSP2_INDEX] = "cdsp2",
 	[SMEM_IMAGE_TABLE_CDSP3_INDEX] = "cdsp3",
 	[SMEM_IMAGE_TABLE_CDSP_INDEX] = "cdsp",
 	[SMEM_IMAGE_TABLE_CHARGERPD_INDEX] = "chargerpd",
 	[SMEM_IMAGE_TABLE_CNSS_INDEX] = "cnss",
+	[SMEM_IMAGE_TABLE_DCP_INDEX] = "dcp",
+	[SMEM_IMAGE_TABLE_DEVCFG_INDEX] = "devcfg",
 	[SMEM_IMAGE_TABLE_DSPS_INDEX] = "dsps",
 	[SMEM_IMAGE_TABLE_GEARVM_INDEX] = "gearvm",
 	[SMEM_IMAGE_TABLE_GPDSP1_INDEX] = "gpdsp1",
@@ -95,6 +110,9 @@ static const char *const socinfo_image_names[] = {
 	[SMEM_IMAGE_TABLE_NPU_INDEX] = "npu",
 	[SMEM_IMAGE_TABLE_OEMPD_INDEX] = "oempd",
 	[SMEM_IMAGE_TABLE_OISPD_INDEX] = "oispd",
+	[SMEM_IMAGE_TABLE_OOBNS_INDEX] = "oobns",
+	[SMEM_IMAGE_TABLE_OOBS_INDEX] = "oobs",
+	[SMEM_IMAGE_TABLE_QECP_INDEX] = "qecp",
 	[SMEM_IMAGE_TABLE_RPM_INDEX] = "rpm",
 	[SMEM_IMAGE_TABLE_SDI_INDEX] = "sdi",
 	[SMEM_IMAGE_TABLE_SENSORPD_INDEX] = "sensorpd",
@@ -644,7 +662,7 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
 	struct smem_image_version *versions;
 	struct dentry *dentry;
 	size_t size;
-	int i;
+	int i, j;
 	unsigned int num_pmics;
 	unsigned int pmic_array_offset;
 
@@ -788,20 +806,32 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
 		break;
 	}
 
-	versions = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_IMAGE_VERSION_TABLE,
-				 &size);
-
-	for (i = 0; i < ARRAY_SIZE(socinfo_image_names); i++) {
+	for (i = 0, j = 0; i < ARRAY_SIZE(socinfo_image_names); i++, j++) {
 		if (!socinfo_image_names[i])
 			continue;
 
+		if (i == 0) {
+			versions = qcom_smem_get(QCOM_SMEM_HOST_ANY,
+						 SMEM_IMAGE_VERSION_TABLE,
+						 &size);
+		}
+		if (i == 32) {
+			if (!qcom_smem_validate_item(SMEM_IMAGE_VERSION_TABLE_2))
+				break;
+
+			j = 0;
+			versions = qcom_smem_get(QCOM_SMEM_HOST_ANY,
+						 SMEM_IMAGE_VERSION_TABLE_2,
+						 &size);
+		}
+
 		dentry = debugfs_create_dir(socinfo_image_names[i],
 					    qcom_socinfo->dbg_root);
-		debugfs_create_file("name", 0444, dentry, &versions[i],
+		debugfs_create_file("name", 0444, dentry, &versions[j],
 				    &qcom_image_name_ops);
-		debugfs_create_file("variant", 0444, dentry, &versions[i],
+		debugfs_create_file("variant", 0444, dentry, &versions[j],
 				    &qcom_image_variant_ops);
-		debugfs_create_file("oem", 0444, dentry, &versions[i],
+		debugfs_create_file("oem", 0444, dentry, &versions[j],
 				    &qcom_image_oem_ops);
 	}
 }

-- 
2.34.1

Re: [PATCH 1/2] soc: qcom: smem: introduce qcom_smem_validate_item API

[Bjorn Andersson]

On Thu, Oct 30, 2025 at 03:07:48PM +0530, Kathiravan Thirumoorthy wrote:
> When a SMEM item is allocated or retrieved, sanity check on the SMEM item
> is performed and backtrace is printed if the SMEM item is invalid.
> 

That sounds overly defensive...

> Image version table in SMEM contains version details for the first 32
> images. Beyond that, another SMEM item 667 is being used, which may not
> be defined in all the platforms. So directly retrieving the SMEM item 667,
> throws the warning as invalid item number.
> 
> To handle such cases, introduce a new API to validate the SMEM item before
> processing it. While at it, make use of this API in the SMEM driver where
> possible.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/smem.c       | 16 ++++++++++++++--
>  include/linux/soc/qcom/smem.h |  1 +
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index c4c45f15dca4fb14f97df4ad494c1189e4f098bd..8a0a832f1e9915b2177a0fe08298ffe8a779e516 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -396,6 +396,18 @@ bool qcom_smem_is_available(void)
>  }
>  EXPORT_SYMBOL_GPL(qcom_smem_is_available);
>  
> +/**
> + * qcom_smem_validate_item() - Check if SMEM item is within the limit

If nothing else, this contradicts the comment by SMEM_ITEM_COUNT.

> + * @item:	SMEM item to validate
> + *
> + * Return: true if SMEM item is valid, false otherwise.
> + */
> +bool qcom_smem_validate_item(unsigned item)
> +{
> +	return item < __smem->item_count;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_validate_item);
> +
>  static int qcom_smem_alloc_private(struct qcom_smem *smem,
>  				   struct smem_partition *part,
>  				   unsigned item,
> @@ -517,7 +529,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
>  		return -EINVAL;
>  	}
>  
> -	if (WARN_ON(item >= __smem->item_count))
> +	if (WARN_ON(!qcom_smem_validate_item(item)))

When we're using a version 11 (global heap, with toc indexed by the item
number) the SMEM_ITEM_COUNT actually matters, but when we use version 12
the items are stored in linked lists, so the only limit I can see is
that the item needs to be max 16 bit.

I think we should push this check down to qcom_smem_alloc_global().

And have a sanity check for item in qcom_smem_alloc_private() and
qcom_smem_get_private() to avoid truncation errors.

>  		return -EINVAL;
>  
>  	ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
> @@ -690,7 +702,7 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
>  	if (!__smem)
>  		return ptr;
>  
> -	if (WARN_ON(item >= __smem->item_count))
> +	if (WARN_ON(!qcom_smem_validate_item(item)))

I think we should push this check down to qcom_smem_get_global()

I guess we'd still hit your problem on version 11 platforms if we keep
the WARN_ON(), but I don't know why that's reason for throwing a splat
in the log. Let's drop the WARN_ON() as well.

>  		return ERR_PTR(-EINVAL);
>  
>  	if (host < SMEM_HOST_COUNT && __smem->partitions[host].virt_base) {
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index f946e3beca215548ac56dbf779138d05479712f5..05891532d530a25747afb8dc96ad4ba668598197 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -5,6 +5,7 @@
>  #define QCOM_SMEM_HOST_ANY -1
>  
>  bool qcom_smem_is_available(void);
> +bool qcom_smem_validate_item(unsigned item);

This makes the API clunky for no real reason, let's avoid that.


Adding Chris, in case I'm overlooking something here.

Regards,
Bjorn

>  int qcom_smem_alloc(unsigned host, unsigned item, size_t size);
>  void *qcom_smem_get(unsigned host, unsigned item, size_t *size);
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] soc: qcom: socinfo: add support to extract more than 32 image versions

[Bjorn Andersson]

On Thu, Oct 30, 2025 at 03:07:49PM +0530, Kathiravan Thirumoorthy wrote:
> SMEM_IMAGE_VERSION_TABLE contains the version of the first 32 images.
> Add images beyond that and read these from SMEM_IMAGE_VERSION_TABLE_2.
> 
> Not all platforms define the SMEM item number 667, in that case
> qcom_smem_get() will throw the invalid item warning. To avoid that,
> validate the SMEM item before fetching the version details.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/socinfo.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> index 4fd09e2bfd021424b9489cd29eec29dc7c7a16d3..f832ae36942b10f68f0c3304f98d946796e8d1bd 100644
> --- a/drivers/soc/qcom/socinfo.c
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -67,7 +67,17 @@
>  #define SMEM_IMAGE_TABLE_GEARVM_INDEX	29
>  #define SMEM_IMAGE_TABLE_UEFI_INDEX	30
>  #define SMEM_IMAGE_TABLE_CDSP3_INDEX	31
> +#define SMEM_IMAGE_TABLE_AUDIOPD_ADSP1_INDEX	32
> +#define SMEM_IMAGE_TABLE_AUDIOPD_ADSP2_INDEX	33
> +#define SMEM_IMAGE_TABLE_DCP_INDEX	34
> +#define SMEM_IMAGE_TABLE_OOBS_INDEX	35
> +#define SMEM_IMAGE_TABLE_OOBNS_INDEX	36
> +#define SMEM_IMAGE_TABLE_DEVCFG_INDEX	37
> +#define SMEM_IMAGE_TABLE_BTPD_INDEX	38
> +#define SMEM_IMAGE_TABLE_QECP_INDEX	39
> +
>  #define SMEM_IMAGE_VERSION_TABLE       469
> +#define SMEM_IMAGE_VERSION_TABLE_2	667
>  
>  /*
>   * SMEM Image table names
> @@ -79,13 +89,18 @@ static const char *const socinfo_image_names[] = {
>  	[SMEM_IMAGE_TABLE_APPSBL_INDEX] = "appsbl",
>  	[SMEM_IMAGE_TABLE_APPS_INDEX] = "apps",
>  	[SMEM_IMAGE_TABLE_AUDIOPD_INDEX] = "audiopd",
> +	[SMEM_IMAGE_TABLE_AUDIOPD_ADSP1_INDEX] = "audiopd_adsp1",
> +	[SMEM_IMAGE_TABLE_AUDIOPD_ADSP2_INDEX] = "audiopd_adsp2",
>  	[SMEM_IMAGE_TABLE_BOOT_INDEX] = "boot",
> +	[SMEM_IMAGE_TABLE_BTPD_INDEX] = "btpd",
>  	[SMEM_IMAGE_TABLE_CDSP1_INDEX] = "cdsp1",
>  	[SMEM_IMAGE_TABLE_CDSP2_INDEX] = "cdsp2",
>  	[SMEM_IMAGE_TABLE_CDSP3_INDEX] = "cdsp3",
>  	[SMEM_IMAGE_TABLE_CDSP_INDEX] = "cdsp",
>  	[SMEM_IMAGE_TABLE_CHARGERPD_INDEX] = "chargerpd",
>  	[SMEM_IMAGE_TABLE_CNSS_INDEX] = "cnss",
> +	[SMEM_IMAGE_TABLE_DCP_INDEX] = "dcp",
> +	[SMEM_IMAGE_TABLE_DEVCFG_INDEX] = "devcfg",
>  	[SMEM_IMAGE_TABLE_DSPS_INDEX] = "dsps",
>  	[SMEM_IMAGE_TABLE_GEARVM_INDEX] = "gearvm",
>  	[SMEM_IMAGE_TABLE_GPDSP1_INDEX] = "gpdsp1",
> @@ -95,6 +110,9 @@ static const char *const socinfo_image_names[] = {
>  	[SMEM_IMAGE_TABLE_NPU_INDEX] = "npu",
>  	[SMEM_IMAGE_TABLE_OEMPD_INDEX] = "oempd",
>  	[SMEM_IMAGE_TABLE_OISPD_INDEX] = "oispd",
> +	[SMEM_IMAGE_TABLE_OOBNS_INDEX] = "oobns",
> +	[SMEM_IMAGE_TABLE_OOBS_INDEX] = "oobs",
> +	[SMEM_IMAGE_TABLE_QECP_INDEX] = "qecp",
>  	[SMEM_IMAGE_TABLE_RPM_INDEX] = "rpm",
>  	[SMEM_IMAGE_TABLE_SDI_INDEX] = "sdi",
>  	[SMEM_IMAGE_TABLE_SENSORPD_INDEX] = "sensorpd",
> @@ -644,7 +662,7 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
>  	struct smem_image_version *versions;
>  	struct dentry *dentry;
>  	size_t size;
> -	int i;
> +	int i, j;
>  	unsigned int num_pmics;
>  	unsigned int pmic_array_offset;
>  
> @@ -788,20 +806,32 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
>  		break;
>  	}
>  
> -	versions = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_IMAGE_VERSION_TABLE,
> -				 &size);
> -
> -	for (i = 0; i < ARRAY_SIZE(socinfo_image_names); i++) {
> +	for (i = 0, j = 0; i < ARRAY_SIZE(socinfo_image_names); i++, j++) {
>  		if (!socinfo_image_names[i])
>  			continue;
>  
> +		if (i == 0) {
> +			versions = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> +						 SMEM_IMAGE_VERSION_TABLE,
> +						 &size);
> +		}
> +		if (i == 32) {

Probably nicer to do } else if (...) { here...

> +			if (!qcom_smem_validate_item(SMEM_IMAGE_VERSION_TABLE_2))

Let's see if we can clean up patch 1 and get rid of this.
Other than that, this patch looks good.

Regards,
Bjorn

> +				break;
> +
> +			j = 0;
> +			versions = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> +						 SMEM_IMAGE_VERSION_TABLE_2,
> +						 &size);
> +		}
> +
>  		dentry = debugfs_create_dir(socinfo_image_names[i],
>  					    qcom_socinfo->dbg_root);
> -		debugfs_create_file("name", 0444, dentry, &versions[i],
> +		debugfs_create_file("name", 0444, dentry, &versions[j],
>  				    &qcom_image_name_ops);
> -		debugfs_create_file("variant", 0444, dentry, &versions[i],
> +		debugfs_create_file("variant", 0444, dentry, &versions[j],
>  				    &qcom_image_variant_ops);
> -		debugfs_create_file("oem", 0444, dentry, &versions[i],
> +		debugfs_create_file("oem", 0444, dentry, &versions[j],
>  				    &qcom_image_oem_ops);
>  	}
>  }
> 
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] soc: qcom: smem: introduce qcom_smem_validate_item API

[Christopher Lew]

On Thu, Oct 30, 2025 at 9:48 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Oct 30, 2025 at 03:07:48PM +0530, Kathiravan Thirumoorthy wrote:
> > When a SMEM item is allocated or retrieved, sanity check on the SMEM item
> > is performed and backtrace is printed if the SMEM item is invalid.
> >
>
> That sounds overly defensive...
>

Discussed with Bjorn a bit offline and we couldn't come up with a
really good reason to keep the WARN_ON() check.
Dropping WARN_ON() and returning an error back from qcom_smem_get()
that clients can act on should suffice.

> > Image version table in SMEM contains version details for the first 32
> > images. Beyond that, another SMEM item 667 is being used, which may not
> > be defined in all the platforms. So directly retrieving the SMEM item 667,
> > throws the warning as invalid item number.
> >
> > To handle such cases, introduce a new API to validate the SMEM item before
> > processing it. While at it, make use of this API in the SMEM driver where
> > possible.
> >
> > Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> > ---
> >  drivers/soc/qcom/smem.c       | 16 ++++++++++++++--
> >  include/linux/soc/qcom/smem.h |  1 +
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > index c4c45f15dca4fb14f97df4ad494c1189e4f098bd..8a0a832f1e9915b2177a0fe08298ffe8a779e516 100644
> > --- a/drivers/soc/qcom/smem.c
> > +++ b/drivers/soc/qcom/smem.c
> > @@ -396,6 +396,18 @@ bool qcom_smem_is_available(void)
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_smem_is_available);
> >
> > +/**
> > + * qcom_smem_validate_item() - Check if SMEM item is within the limit
>
> If nothing else, this contradicts the comment by SMEM_ITEM_COUNT.
>
> > + * @item:    SMEM item to validate
> > + *
> > + * Return: true if SMEM item is valid, false otherwise.
> > + */
> > +bool qcom_smem_validate_item(unsigned item)
> > +{
> > +     return item < __smem->item_count;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_smem_validate_item);
> > +
> >  static int qcom_smem_alloc_private(struct qcom_smem *smem,
> >                                  struct smem_partition *part,
> >                                  unsigned item,
> > @@ -517,7 +529,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
> >               return -EINVAL;
> >       }
> >
> > -     if (WARN_ON(item >= __smem->item_count))
> > +     if (WARN_ON(!qcom_smem_validate_item(item)))
>
> When we're using a version 11 (global heap, with toc indexed by the item
> number) the SMEM_ITEM_COUNT actually matters, but when we use version 12
> the items are stored in linked lists, so the only limit I can see is
> that the item needs to be max 16 bit.
>
> I think we should push this check down to qcom_smem_alloc_global().
>
> And have a sanity check for item in qcom_smem_alloc_private() and
> qcom_smem_get_private() to avoid truncation errors.
>

For alloc, I think we should adhere to the platform defined max
item_count. I'm not sure if the remote hosts validate the entries
against item_count while they are iterating through the items during
their implementation of qcom_smem_get().

> >               return -EINVAL;
> >
> >       ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
> > @@ -690,7 +702,7 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
> >       if (!__smem)
> >               return ptr;
> >
> > -     if (WARN_ON(item >= __smem->item_count))
> > +     if (WARN_ON(!qcom_smem_validate_item(item)))
>
> I think we should push this check down to qcom_smem_get_global()
>
> I guess we'd still hit your problem on version 11 platforms if we keep
> the WARN_ON(), but I don't know why that's reason for throwing a splat
> in the log. Let's drop the WARN_ON() as well.
>

I think it's worth keeping the item_count check here because it acts
as a quick short circuit out of the search loop for absurd values in
qcom_smem_get_private().

Thanks,
Chris

> >               return ERR_PTR(-EINVAL);
> >
> >       if (host < SMEM_HOST_COUNT && __smem->partitions[host].virt_base) {
> > diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> > index f946e3beca215548ac56dbf779138d05479712f5..05891532d530a25747afb8dc96ad4ba668598197 100644
> > --- a/include/linux/soc/qcom/smem.h
> > +++ b/include/linux/soc/qcom/smem.h
> > @@ -5,6 +5,7 @@
> >  #define QCOM_SMEM_HOST_ANY -1
> >
> >  bool qcom_smem_is_available(void);
> > +bool qcom_smem_validate_item(unsigned item);
>
> This makes the API clunky for no real reason, let's avoid that.
>
>
> Adding Chris, in case I'm overlooking something here.
>
> Regards,
> Bjorn
>
> >  int qcom_smem_alloc(unsigned host, unsigned item, size_t size);
> >  void *qcom_smem_get(unsigned host, unsigned item, size_t *size);
> >
> >
> > --
> > 2.34.1
> >

Re: [PATCH 1/2] soc: qcom: smem: introduce qcom_smem_validate_item API

[Kathiravan Thirumoorthy]

On 10/31/2025 6:10 AM, Christopher Lew wrote:
> On Thu, Oct 30, 2025 at 9:48 AM Bjorn Andersson <andersson@kernel.org> wrote:
>> On Thu, Oct 30, 2025 at 03:07:48PM +0530, Kathiravan Thirumoorthy wrote:
>>> When a SMEM item is allocated or retrieved, sanity check on the SMEM item
>>> is performed and backtrace is printed if the SMEM item is invalid.
>>>
>> That sounds overly defensive...
>>
> Discussed with Bjorn a bit offline and we couldn't come up with a
> really good reason to keep the WARN_ON() check.
> Dropping WARN_ON() and returning an error back from qcom_smem_get()
> that clients can act on should suffice.


Thanks Bjorn and Chris for the comments and the suggestions. Based on 
the below feedback from Chris, I understand that we can just drop the 
WARN_ON() on both places. I will submit the V2 with that address.


>
>>> Image version table in SMEM contains version details for the first 32
>>> images. Beyond that, another SMEM item 667 is being used, which may not
>>> be defined in all the platforms. So directly retrieving the SMEM item 667,
>>> throws the warning as invalid item number.
>>>
>>> To handle such cases, introduce a new API to validate the SMEM item before
>>> processing it. While at it, make use of this API in the SMEM driver where
>>> possible.
>>>
>>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>>> ---
>>>   drivers/soc/qcom/smem.c       | 16 ++++++++++++++--
>>>   include/linux/soc/qcom/smem.h |  1 +
>>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>>> index c4c45f15dca4fb14f97df4ad494c1189e4f098bd..8a0a832f1e9915b2177a0fe08298ffe8a779e516 100644
>>> --- a/drivers/soc/qcom/smem.c
>>> +++ b/drivers/soc/qcom/smem.c
>>> @@ -396,6 +396,18 @@ bool qcom_smem_is_available(void)
>>>   }
>>>   EXPORT_SYMBOL_GPL(qcom_smem_is_available);
>>>
>>> +/**
>>> + * qcom_smem_validate_item() - Check if SMEM item is within the limit
>> If nothing else, this contradicts the comment by SMEM_ITEM_COUNT.
>>
>>> + * @item:    SMEM item to validate
>>> + *
>>> + * Return: true if SMEM item is valid, false otherwise.
>>> + */
>>> +bool qcom_smem_validate_item(unsigned item)
>>> +{
>>> +     return item < __smem->item_count;
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_smem_validate_item);
>>> +
>>>   static int qcom_smem_alloc_private(struct qcom_smem *smem,
>>>                                   struct smem_partition *part,
>>>                                   unsigned item,
>>> @@ -517,7 +529,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
>>>                return -EINVAL;
>>>        }
>>>
>>> -     if (WARN_ON(item >= __smem->item_count))
>>> +     if (WARN_ON(!qcom_smem_validate_item(item)))
>> When we're using a version 11 (global heap, with toc indexed by the item
>> number) the SMEM_ITEM_COUNT actually matters, but when we use version 12
>> the items are stored in linked lists, so the only limit I can see is
>> that the item needs to be max 16 bit.
>>
>> I think we should push this check down to qcom_smem_alloc_global().
>>
>> And have a sanity check for item in qcom_smem_alloc_private() and
>> qcom_smem_get_private() to avoid truncation errors.
>>
> For alloc, I think we should adhere to the platform defined max
> item_count. I'm not sure if the remote hosts validate the entries
> against item_count while they are iterating through the items during
> their implementation of qcom_smem_get().
>
>>>                return -EINVAL;
>>>
>>>        ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
>>> @@ -690,7 +702,7 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
>>>        if (!__smem)
>>>                return ptr;
>>>
>>> -     if (WARN_ON(item >= __smem->item_count))
>>> +     if (WARN_ON(!qcom_smem_validate_item(item)))
>> I think we should push this check down to qcom_smem_get_global()
>>
>> I guess we'd still hit your problem on version 11 platforms if we keep
>> the WARN_ON(), but I don't know why that's reason for throwing a splat
>> in the log. Let's drop the WARN_ON() as well.
>>
> I think it's worth keeping the item_count check here because it acts
> as a quick short circuit out of the search loop for absurd values in
> qcom_smem_get_private().
>
> Thanks,
> Chris
>
>>>                return ERR_PTR(-EINVAL);
>>>
>>>        if (host < SMEM_HOST_COUNT && __smem->partitions[host].virt_base) {
>>> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
>>> index f946e3beca215548ac56dbf779138d05479712f5..05891532d530a25747afb8dc96ad4ba668598197 100644
>>> --- a/include/linux/soc/qcom/smem.h
>>> +++ b/include/linux/soc/qcom/smem.h
>>> @@ -5,6 +5,7 @@
>>>   #define QCOM_SMEM_HOST_ANY -1
>>>
>>>   bool qcom_smem_is_available(void);
>>> +bool qcom_smem_validate_item(unsigned item);
>> This makes the API clunky for no real reason, let's avoid that.
>>
>>
>> Adding Chris, in case I'm overlooking something here.
>>
>> Regards,
>> Bjorn
>>
>>>   int qcom_smem_alloc(unsigned host, unsigned item, size_t size);
>>>   void *qcom_smem_get(unsigned host, unsigned item, size_t *size);
>>>
>>>
>>> --
>>> 2.34.1
>>>

Re: [PATCH 2/2] soc: qcom: socinfo: add support to extract more than 32 image versions

[Kathiravan Thirumoorthy]

On 10/30/2025 10:41 PM, Bjorn Andersson wrote:
> On Thu, Oct 30, 2025 at 03:07:49PM +0530, Kathiravan Thirumoorthy wrote:
>> SMEM_IMAGE_VERSION_TABLE contains the version of the first 32 images.
>> Add images beyond that and read these from SMEM_IMAGE_VERSION_TABLE_2.
>>
>> Not all platforms define the SMEM item number 667, in that case
>> qcom_smem_get() will throw the invalid item warning. To avoid that,
>> validate the SMEM item before fetching the version details.
>>
>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>> ---
>>   drivers/soc/qcom/socinfo.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> index 4fd09e2bfd021424b9489cd29eec29dc7c7a16d3..f832ae36942b10f68f0c3304f98d946796e8d1bd 100644
>> --- a/drivers/soc/qcom/socinfo.c
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -67,7 +67,17 @@
>>   #define SMEM_IMAGE_TABLE_GEARVM_INDEX	29
>>   #define SMEM_IMAGE_TABLE_UEFI_INDEX	30
>>   #define SMEM_IMAGE_TABLE_CDSP3_INDEX	31
>> +#define SMEM_IMAGE_TABLE_AUDIOPD_ADSP1_INDEX	32
>> +#define SMEM_IMAGE_TABLE_AUDIOPD_ADSP2_INDEX	33
>> +#define SMEM_IMAGE_TABLE_DCP_INDEX	34
>> +#define SMEM_IMAGE_TABLE_OOBS_INDEX	35
>> +#define SMEM_IMAGE_TABLE_OOBNS_INDEX	36
>> +#define SMEM_IMAGE_TABLE_DEVCFG_INDEX	37
>> +#define SMEM_IMAGE_TABLE_BTPD_INDEX	38
>> +#define SMEM_IMAGE_TABLE_QECP_INDEX	39
>> +
>>   #define SMEM_IMAGE_VERSION_TABLE       469
>> +#define SMEM_IMAGE_VERSION_TABLE_2	667
>>   
>>   /*
>>    * SMEM Image table names
>> @@ -79,13 +89,18 @@ static const char *const socinfo_image_names[] = {
>>   	[SMEM_IMAGE_TABLE_APPSBL_INDEX] = "appsbl",
>>   	[SMEM_IMAGE_TABLE_APPS_INDEX] = "apps",
>>   	[SMEM_IMAGE_TABLE_AUDIOPD_INDEX] = "audiopd",
>> +	[SMEM_IMAGE_TABLE_AUDIOPD_ADSP1_INDEX] = "audiopd_adsp1",
>> +	[SMEM_IMAGE_TABLE_AUDIOPD_ADSP2_INDEX] = "audiopd_adsp2",
>>   	[SMEM_IMAGE_TABLE_BOOT_INDEX] = "boot",
>> +	[SMEM_IMAGE_TABLE_BTPD_INDEX] = "btpd",
>>   	[SMEM_IMAGE_TABLE_CDSP1_INDEX] = "cdsp1",
>>   	[SMEM_IMAGE_TABLE_CDSP2_INDEX] = "cdsp2",
>>   	[SMEM_IMAGE_TABLE_CDSP3_INDEX] = "cdsp3",
>>   	[SMEM_IMAGE_TABLE_CDSP_INDEX] = "cdsp",
>>   	[SMEM_IMAGE_TABLE_CHARGERPD_INDEX] = "chargerpd",
>>   	[SMEM_IMAGE_TABLE_CNSS_INDEX] = "cnss",
>> +	[SMEM_IMAGE_TABLE_DCP_INDEX] = "dcp",
>> +	[SMEM_IMAGE_TABLE_DEVCFG_INDEX] = "devcfg",
>>   	[SMEM_IMAGE_TABLE_DSPS_INDEX] = "dsps",
>>   	[SMEM_IMAGE_TABLE_GEARVM_INDEX] = "gearvm",
>>   	[SMEM_IMAGE_TABLE_GPDSP1_INDEX] = "gpdsp1",
>> @@ -95,6 +110,9 @@ static const char *const socinfo_image_names[] = {
>>   	[SMEM_IMAGE_TABLE_NPU_INDEX] = "npu",
>>   	[SMEM_IMAGE_TABLE_OEMPD_INDEX] = "oempd",
>>   	[SMEM_IMAGE_TABLE_OISPD_INDEX] = "oispd",
>> +	[SMEM_IMAGE_TABLE_OOBNS_INDEX] = "oobns",
>> +	[SMEM_IMAGE_TABLE_OOBS_INDEX] = "oobs",
>> +	[SMEM_IMAGE_TABLE_QECP_INDEX] = "qecp",
>>   	[SMEM_IMAGE_TABLE_RPM_INDEX] = "rpm",
>>   	[SMEM_IMAGE_TABLE_SDI_INDEX] = "sdi",
>>   	[SMEM_IMAGE_TABLE_SENSORPD_INDEX] = "sensorpd",
>> @@ -644,7 +662,7 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
>>   	struct smem_image_version *versions;
>>   	struct dentry *dentry;
>>   	size_t size;
>> -	int i;
>> +	int i, j;
>>   	unsigned int num_pmics;
>>   	unsigned int pmic_array_offset;
>>   
>> @@ -788,20 +806,32 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo,
>>   		break;
>>   	}
>>   
>> -	versions = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_IMAGE_VERSION_TABLE,
>> -				 &size);
>> -
>> -	for (i = 0; i < ARRAY_SIZE(socinfo_image_names); i++) {
>> +	for (i = 0, j = 0; i < ARRAY_SIZE(socinfo_image_names); i++, j++) {
>>   		if (!socinfo_image_names[i])
>>   			continue;
>>   
>> +		if (i == 0) {
>> +			versions = qcom_smem_get(QCOM_SMEM_HOST_ANY,
>> +						 SMEM_IMAGE_VERSION_TABLE,
>> +						 &size);
>> +		}
>> +		if (i == 32) {
> Probably nicer to do } else if (...) { here...


Ack.


>
>> +			if (!qcom_smem_validate_item(SMEM_IMAGE_VERSION_TABLE_2))
> Let's see if we can clean up patch 1 and get rid of this.
> Other than that, this patch looks good.


Yeah, will drop this one and add the check for "versions".


>
> Regards,
> Bjorn
>
>> +				break;
>> +
>> +			j = 0;
>> +			versions = qcom_smem_get(QCOM_SMEM_HOST_ANY,
>> +						 SMEM_IMAGE_VERSION_TABLE_2,
>> +						 &size);
>> +		}
>> +
>>   		dentry = debugfs_create_dir(socinfo_image_names[i],
>>   					    qcom_socinfo->dbg_root);
>> -		debugfs_create_file("name", 0444, dentry, &versions[i],
>> +		debugfs_create_file("name", 0444, dentry, &versions[j],
>>   				    &qcom_image_name_ops);
>> -		debugfs_create_file("variant", 0444, dentry, &versions[i],
>> +		debugfs_create_file("variant", 0444, dentry, &versions[j],
>>   				    &qcom_image_variant_ops);
>> -		debugfs_create_file("oem", 0444, dentry, &versions[i],
>> +		debugfs_create_file("oem", 0444, dentry, &versions[j],
>>   				    &qcom_image_oem_ops);
>>   	}
>>   }
>>
>> -- 
>> 2.34.1
>>