Qualcomm SMEM V12 Support

PATCH Superseded Chris Lew 3 participants 11 messages Aug 18, 2017 – Aug 23, 2017

Subsystems (2)

Arm Qualcomm mailing list Arm Qualcomm support

lore

Share & tools

lore

Chris Lew

author

2017-08-18 01:15 UTC #0 lore raw

cover Qualcomm SMEM V12 Support

SMEM V12 was devised to make better use of the global SMEM region. The global heap region is formatted to be similar to a private partition. This allows the maximum number of smem items to increase. The maximum item number is written by the bootloader in a region after the table of contents. The number of hosts are increased for later chipsets.

This patchset depends on patch: Qualcomm SMEM cached item support

Chris Lew (3):
soc: qcom: smem: Support global partition
soc: qcom: smem: Support dynamic item limit
soc: qcom: smem: Increase the number of hosts

 drivers/soc/qcom/smem.c | 177 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 145 insertions(+), 32 deletions(-)

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Chris Lew

author

2017-08-18 01:15 UTC #1 lore raw

PATCH 1/3 soc: qcom: smem: Support global partition

SMEM V12 creates a global partition to allocate global smem items from instead of a global heap. The global partition has the same structure as a private partition.

Signed-off-by Chris Lew <[email protected]>

---
 drivers/soc/qcom/smem.c | 134 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 105 insertions(+), 29 deletions(-)

drivers/soc/qcom/smem.c +105 ~~-29~~

index c28275be0038..fed2934d6bda 100644
@@ -65,11 +65,20 @@
 /*
  * Item 3 of the global heap contains an array of versions for the various
  * software components in the SoC. We verify that the boot loader version is
- * what the expected version (SMEM_EXPECTED_VERSION) as a sanity check.
+ * a valid version as a sanity check.
  */
 #define SMEM_ITEM_VERSION	3
 #define  SMEM_MASTER_SBL_VERSION_INDEX	7
-#define  SMEM_EXPECTED_VERSION		11
+#define  SMEM_GLOBAL_HEAP_VERSION	11
+
+/*
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
+ */
+#define SMEM_GLOBAL_PART_VERSION	12
+#define SMEM_GLOBAL_HOST	0xfffe
 
 /*
  * The first 8 items are only to be allocated by the boot loader while

@@ -231,6 +240,8 @@ struct smem_region {
  * struct qcom_smem - device data for the smem device
  * @dev:	device pointer
  * @hwlock:	reference to a hwspinlock
+ * @global_partition:	pointer to global partition when in use
+ * @global_cacheline:	cacheline size for global partition
  * @partitions:	list of pointers to partitions affecting the current
  *		processor/host
  * @cacheline:	list of cacheline sizes for each host

@@ -242,6 +253,8 @@ struct qcom_smem {
 
 	struct hwspinlock *hwlock;
 
+	struct smem_partition_header *global_partition;
+	size_t global_cacheline;
 	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
 	size_t cacheline[SMEM_HOST_COUNT];

@@ -318,16 +331,14 @@ static void *cached_entry_to_item(struct smem_private_entry *e)
 #define HWSPINLOCK_TIMEOUT	1000
 
 static int qcom_smem_alloc_private(struct qcom_smem *smem,
-				   unsigned host,
+				   struct smem_partition_header *phdr,
 				   unsigned item,
 				   size_t size)
 {
-	struct smem_partition_header *phdr;
 	struct smem_private_entry *hdr, *end;
 	size_t alloc_size;
 	void *cached;
 
-	phdr = smem->partitions[host];
 	hdr = phdr_to_first_uncached_entry(phdr);
 	end = phdr_to_last_uncached_entry(phdr);
 	cached = phdr_to_last_cached_entry(phdr);

@@ -335,8 +346,8 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
 	while (hdr < end) {
 		if (hdr->canary != SMEM_PRIVATE_CANARY) {
 			dev_err(smem->dev,
-				"Found invalid canary in host %d partition\n",
-				host);
+				"Found invalid canary in hosts %d:%d partition\n",
+				phdr->host0, phdr->host1);
 			return -EINVAL;
 		}

@@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 {
 	unsigned long flags;
 	int ret;
+	struct smem_partition_header *phdr;
 
 	if (!__smem)
 		return -EPROBE_DEFER;

@@ -435,10 +447,15 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 	if (ret)
 		return ret;
 
-	if (host < SMEM_HOST_COUNT && __smem->partitions[host])
-		ret = qcom_smem_alloc_private(__smem, host, item, size);
-	else
+	if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
+		phdr = __smem->partitions[host];
+		ret = qcom_smem_alloc_private(__smem, phdr, item, size);
+	} else if (__smem->global_partition) {
+		phdr = __smem->global_partition;
+		ret = qcom_smem_alloc_private(__smem, phdr, item, size);
+	} else {
 		ret = qcom_smem_alloc_global(__smem, item, size);
+	}
 
 	hwspin_unlock_irqrestore(__smem->hwlock, &flags);

@@ -480,16 +497,12 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
 }
 
 static void *qcom_smem_get_private(struct qcom_smem *smem,
-				   unsigned host,
+				   struct smem_partition_header *phdr,
+				   size_t cacheline,
 				   unsigned item,
 				   size_t *size)
 {
-	struct smem_partition_header *phdr;
 	struct smem_private_entry *e, *end;
-	size_t cacheline;
-
-	phdr = smem->partitions[host];
-	cacheline = smem->cacheline[host];
 
 	e = phdr_to_first_uncached_entry(phdr);
 	end = phdr_to_last_uncached_entry(phdr);

@@ -532,7 +545,8 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
 	return ERR_PTR(-ENOENT);
 
 invalid_canary:
-	dev_err(smem->dev, "Found invalid canary in host %d partition\n", host);
+	dev_err(smem->dev, "Found invalid canary in hosts %d:%d partition\n",
+			phdr->host0, phdr->host1);
 
 	return ERR_PTR(-EINVAL);
 }

@@ -551,6 +565,8 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
 	unsigned long flags;
 	int ret;
 	void *ptr = ERR_PTR(-EPROBE_DEFER);
+	struct smem_partition_header *phdr;
+	size_t cacheln;
 
 	if (!__smem)
 		return ptr;

@@ -561,10 +577,17 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
 	if (ret)
 		return ERR_PTR(ret);
 
-	if (host < SMEM_HOST_COUNT && __smem->partitions[host])
-		ptr = qcom_smem_get_private(__smem, host, item, size);
-	else
+	if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
+		phdr = __smem->partitions[host];
+		cacheln = __smem->cacheline[host];
+		ptr = qcom_smem_get_private(__smem, phdr, cacheln, item, size);
+	} else if (__smem->global_partition) {
+		phdr = __smem->global_partition;
+		cacheln = __smem->global_cacheline;
+		ptr = qcom_smem_get_private(__smem, phdr, cacheln, item, size);
+	} else {
 		ptr = qcom_smem_get_global(__smem, item, size);
+	}
 
 	hwspin_unlock_irqrestore(__smem->hwlock, &flags);

@@ -593,6 +616,10 @@ int qcom_smem_get_free_space(unsigned host)
 		phdr = __smem->partitions[host];
 		ret = le32_to_cpu(phdr->offset_free_cached) -
 		      le32_to_cpu(phdr->offset_free_uncached);
+	} else if (__smem->global_partition) {
+		phdr = __smem->global_partition;
+		ret = le32_to_cpu(phdr->offset_free_cached) -
+		      le32_to_cpu(phdr->offset_free_uncached);
 	} else {
 		header = __smem->regions[0].virt_base;
 		ret = le32_to_cpu(header->available);

@@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host)
 
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
+	struct smem_header *header;
 	__le32 *versions;
-	size_t size;
 
-	versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
-	if (IS_ERR(versions)) {
-		dev_err(smem->dev, "Unable to read the version item\n");
-		return -ENOENT;
+	header = smem->regions[0].virt_base;
+	versions = header->version;
+
+	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+}
+
+static int qcom_smem_set_global_partition(struct qcom_smem *smem,
+				struct smem_ptable_entry *entry)
+{
+	struct smem_partition_header *header;
+	u32  host0, host1;
+
+	if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
+		dev_err(smem->dev, "Invalid entry for gloabl partition\n");
+		return -EINVAL;
 	}
 
-	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
-		dev_err(smem->dev, "Version item is too small\n");
+	if (smem->global_partition) {
+		dev_err(smem->dev, "Already found the global partition\n");
 		return -EINVAL;
 	}
+	header = smem->regions[0].virt_base + le32_to_cpu(entry->offset);
+	host0 = le16_to_cpu(header->host0);
+	host1 = le16_to_cpu(header->host1);
 
-	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+	if (memcmp(header->magic, SMEM_PART_MAGIC,
+		    sizeof(header->magic))) {
+		dev_err(smem->dev, "Gloal partition has invalid magic\n");
+		return -EINVAL;
+	}
+
+	if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) {
+		dev_err(smem->dev, "Global partition hosts are invalid\n");
+		return -EINVAL;
+	}
+
+	if (header->size != entry->size) {
+		dev_err(smem->dev, "Global partition has invalid size\n");
+		return -EINVAL;
+	}
+
+	if (le32_to_cpu(header->offset_free_uncached) >
+			le32_to_cpu(header->size)) {
+		dev_err(smem->dev,
+			"Global partition has invalid free pointer\n");
+		return -EINVAL;
+	}
+
+	smem->global_partition = header;
+	smem->global_cacheline = le32_to_cpu(entry->cacheline);
+
+	return 0;
 }
 
 static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,

@@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
 		host0 = le16_to_cpu(entry->host0);
 		host1 = le16_to_cpu(entry->host1);
 
+		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
+			if (qcom_smem_set_global_partition(smem, entry))
+				return -EINVAL;
+			continue;
+		}
+
 		if (host0 != local_host && host1 != local_host)
 			continue;

@@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
 	}
 
 	version = qcom_smem_get_sbl_version(smem);
-	if (version >> 16 != SMEM_EXPECTED_VERSION) {
+	switch (version >> 16) {
+	case SMEM_GLOBAL_PART_VERSION:
+	case SMEM_GLOBAL_HEAP_VERSION:
+	default:
 		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
 		return -EINVAL;
 	}

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Arun Kumar Neelakantam

2017-08-21 06:05 UTC #2 lore raw

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

drivers/soc/qcom/smem.c static int qcom_smem_probe(struct platform_device *pdev) · line 860

Quoted 8 lines · Chris Lew

drivers/soc/qcom/smem.c +3 -1

@@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
  	}
 
  	version = qcom_smem_get_sbl_version(smem);
-	if (version >> 16 != SMEM_EXPECTED_VERSION) {
+	switch (version >> 16) {
+	case SMEM_GLOBAL_PART_VERSION:
+	case SMEM_GLOBAL_HEAP_VERSION:

break statement is needed for supported versions

Quoted 4 lines

drivers/soc/qcom/smem.c +1 -0

+	default:
  		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
  		return -EINVAL;
  	}

Bjorn Andersson

2017-08-21 17:17 UTC #3 lore raw

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

In reply to Chris Lew

Quoted 4 lines · Chris Lew

SMEM V12 creates a global partition to allocate global smem items from instead of a global heap. The global partition has the same structure as a private partition.

Welcome to LKML!

This looks quite reasonable, just some minor comments below.

Quoted 10 lines

Signed-off-by Chris Lew <[email protected]>

---
 drivers/soc/qcom/smem.c | 134 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 105 insertions(+), 29 deletions(-)

drivers/soc/qcom/smem.c +0 -0

index c28275be0038..fed2934d6bda 100644

drivers/soc/qcom/smem.c line 70

Quoted 7 lines

drivers/soc/qcom/smem.c +1 -1

@@ -65,11 +65,20 @@
 /*
  * Item 3 of the global heap contains an array of versions for the various
  * software components in the SoC. We verify that the boot loader version is
- * what the expected version (SMEM_EXPECTED_VERSION) as a sanity check.
+ * a valid version as a sanity check.
  */
 #define SMEM_ITEM_VERSION	3

SMEM_ITEM_VERSION is now unused, which means that the comment should be updated with respect to item 3 and the versions array of smem_header.

drivers/soc/qcom/smem.c line 79

Quoted 10 lines

drivers/soc/qcom/smem.c +8 -1

 #define  SMEM_MASTER_SBL_VERSION_INDEX	7
-#define  SMEM_EXPECTED_VERSION		11
+#define  SMEM_GLOBAL_HEAP_VERSION	11
+
+/*
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
+ */

Please incorporate this paragraph in the larger comment at the top of the file, so we keep that up to date.

[..]

drivers/soc/qcom/smem.c int qcom_smem_alloc(unsigned host, unsigned item, size_t size) · line 433

Quoted 5 lines

drivers/soc/qcom/smem.c +1 -0

@@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 {
 	unsigned long flags;
 	int ret;
+	struct smem_partition_header *phdr;

Sorry for my OCD, but please maintain my reverse XMAS tree (put this declaration first, as it's the longest).

Quoted 3 lines

drivers/soc/qcom/smem.c +0 -0
 	if (!__smem)
 		return -EPROBE_DEFER;

[..]

Quoted 7 lines

drivers/soc/qcom/smem.c +1 -1

@@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host)
 
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
+	struct smem_header *header;
 	__le32 *versions;
-	size_t size;

drivers/soc/qcom/smem.c int qcom_smem_get_free_space(unsigned host) · line 641

Quoted 10 lines

drivers/soc/qcom/smem.c +5 -4

 
-	versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
-	if (IS_ERR(versions)) {
-		dev_err(smem->dev, "Unable to read the version item\n");
-		return -ENOENT;
+	header = smem->regions[0].virt_base;
+	versions = header->version;
+
+	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+}

I would prefer if you split the change of this function into its own patch, just to clarify that it's unrelated to the new version support.

Quoted 25 lines

drivers/soc/qcom/smem.c +17 -3

+
+static int qcom_smem_set_global_partition(struct qcom_smem *smem,
+				struct smem_ptable_entry *entry)
+{
+	struct smem_partition_header *header;
+	u32  host0, host1;
+
+	if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
+		dev_err(smem->dev, "Invalid entry for gloabl partition\n");
+		return -EINVAL;
 	}
 
-	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
-		dev_err(smem->dev, "Version item is too small\n");
+	if (smem->global_partition) {
+		dev_err(smem->dev, "Already found the global partition\n");
 		return -EINVAL;
 	}
+	header = smem->regions[0].virt_base + le32_to_cpu(entry->offset);
+	host0 = le16_to_cpu(header->host0);
+	host1 = le16_to_cpu(header->host1);
 
-	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+	if (memcmp(header->magic, SMEM_PART_MAGIC,
+		    sizeof(header->magic))) {

drivers/soc/qcom/smem.c int qcom_smem_get_free_space(unsigned host) · line 673

Quoted 10 lines

drivers/soc/qcom/smem.c +10 -0

+		dev_err(smem->dev, "Gloal partition has invalid magic\n");
+		return -EINVAL;
+	}
+
+	if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) {
+		dev_err(smem->dev, "Global partition hosts are invalid\n");
+		return -EINVAL;
+	}
+
+	if (header->size != entry->size) {

This happens to work, because they are both in the same endian. But please sprinkle some le32_to_cpu() here as well.

drivers/soc/qcom/smem.c int qcom_smem_get_free_space(unsigned host) · line 679

Quoted 6 lines

drivers/soc/qcom/smem.c +6 -0

+		dev_err(smem->dev, "Global partition has invalid size\n");
+		return -EINVAL;
+	}
+
+	if (le32_to_cpu(header->offset_free_uncached) >
+			le32_to_cpu(header->size)) {

Consider using local variables in favor of wrapping lines.

Quoted 13 lines

drivers/soc/qcom/smem.c +9 -0

+		dev_err(smem->dev,
+			"Global partition has invalid free pointer\n");
+		return -EINVAL;
+	}
+
+	smem->global_partition = header;
+	smem->global_cacheline = le32_to_cpu(entry->cacheline);
+
+	return 0;
 }
 
 static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,

drivers/soc/qcom/smem.c static int qcom_smem_enumerate_partitions(struct qcom_smem *smem, · line 722

Quoted 9 lines

drivers/soc/qcom/smem.c +6 -0

@@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
 		host0 = le16_to_cpu(entry->host0);
 		host1 = le16_to_cpu(entry->host1);
 
+		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
+			if (qcom_smem_set_global_partition(smem, entry))
+				return -EINVAL;
+			continue;
+		}
+

As you're not able to leverage any of the checks from this loop I think it's cleaner to duplicate the traversal of the partition table in both functions and call the "search for global partition" directly from probe - if the version indicates there should be one.

drivers/soc/qcom/smem.c static int qcom_smem_probe(struct platform_device *pdev) · line 860

Quoted 11 lines

drivers/soc/qcom/smem.c +3 -1

 		if (host0 != local_host && host1 != local_host)
 			continue;

@@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
 	}
 
 	version = qcom_smem_get_sbl_version(smem);
-	if (version >> 16 != SMEM_EXPECTED_VERSION) {
+	switch (version >> 16) {
+	case SMEM_GLOBAL_PART_VERSION:
+	case SMEM_GLOBAL_HEAP_VERSION:

As Arun pointed out, there should be a "break" here.

drivers/soc/qcom/smem.c static int qcom_smem_probe(struct platform_device *pdev) · line 864

Quoted 4 lines

drivers/soc/qcom/smem.c +1 -0

+	default:
 		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
 		return -EINVAL;
 	}

Regards,
Bjorn

Chris Lew

author

2017-08-23 00:28 UTC #4 lore raw

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

Hi Bjorn,

Thanks for the review.

Quoted 14 lines · Bjorn Andersson

drivers/soc/qcom/smem.c line 79

Quoted 10 lines

  #define  SMEM_MASTER_SBL_VERSION_INDEX	7

drivers/soc/qcom/smem.c +8 -1

-#define  SMEM_EXPECTED_VERSION		11
+#define  SMEM_GLOBAL_HEAP_VERSION	11
+
+/*
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
+ */

Please incorporate this paragraph in the larger comment at the top of the file, so we keep that up to date.

Quoted 1 line

[..]

Will do.

Quoted 9 lines

drivers/soc/qcom/smem.c int qcom_smem_alloc(unsigned host, unsigned item, size_t size) · line 433
Quoted 5 lines
drivers/soc/qcom/smem.c +1 -0
@@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
  {
  	unsigned long flags;
  	int ret;
+	struct smem_partition_header *phdr;
Sorry for my OCD, but please maintain my reverse XMAS tree (put this declaration first, as it's the longest).

Ok, will change.

Quoted 12 lines

Quoted 3 lines

drivers/soc/qcom/smem.c +0 -0

  	if (!__smem)
  		return -EPROBE_DEFER;

[..]

Quoted 8 lines

drivers/soc/qcom/smem.c +1 -1

@@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host)
 
  static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
  {
+	struct smem_header *header;
  	__le32 *versions;
-	size_t size;

Quoted 13 lines

drivers/soc/qcom/smem.c int qcom_smem_get_free_space(unsigned host) · line 641
Quoted 9 lines
drivers/soc/qcom/smem.c +5 -4
-	versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
-	if (IS_ERR(versions)) {
-		dev_err(smem->dev, "Unable to read the version item\n");
-		return -ENOENT;
+	header = smem->regions[0].virt_base;
+	versions = header->version;
+
+	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+}
I would prefer if you split the change of this function into its own patch, just to clarify that it's unrelated to the new version support.

Ok, will separate the change and adjust the descriptions accordingly.

Quoted 22 lines

drivers/soc/qcom/smem.c +15 -2

+
+static int qcom_smem_set_global_partition(struct qcom_smem *smem,
+				struct smem_ptable_entry *entry)
+{
+	struct smem_partition_header *header;
+	u32  host0, host1;
+
+	if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
+		dev_err(smem->dev, "Invalid entry for gloabl partition\n");
+		return -EINVAL;
  	}
 
-	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
-		dev_err(smem->dev, "Version item is too small\n");
+	if (smem->global_partition) {
+		dev_err(smem->dev, "Already found the global partition\n");
  		return -EINVAL;
  	}
+	header = smem->regions[0].virt_base + le32_to_cpu(entry->offset);
+	host0 = le16_to_cpu(header->host0);
+	host1 = le16_to_cpu(header->host1);

Quoted 17 lines

drivers/soc/qcom/smem.c int qcom_smem_get_free_space(unsigned host) · line 673
Quoted 13 lines
drivers/soc/qcom/smem.c +12 -1
-	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+	if (memcmp(header->magic, SMEM_PART_MAGIC,
+		    sizeof(header->magic))) {
+		dev_err(smem->dev, "Gloal partition has invalid magic\n");
+		return -EINVAL;
+	}
+
+	if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) {
+		dev_err(smem->dev, "Global partition hosts are invalid\n");
+		return -EINVAL;
+	}
+
+	if (header->size != entry->size) {
This happens to work, because they are both in the same endian. But please sprinkle some le32_to_cpu() here as well.

These checks mimic the sanity checks being done in enumerate_partitions. Should I create a patch to increase le32_to_cpu usage in qcom_smem_enumerate_partitions?

Quoted 9 lines

drivers/soc/qcom/smem.c int qcom_smem_get_free_space(unsigned host) · line 679
Quoted 6 lines
drivers/soc/qcom/smem.c +6 -0
+		dev_err(smem->dev, "Global partition has invalid size\n");
+		return -EINVAL;
+	}
+
+	if (le32_to_cpu(header->offset_free_uncached) >
+			le32_to_cpu(header->size)) {
Consider using local variables in favor of wrapping lines.

Ok, will do.

Quoted 16 lines

drivers/soc/qcom/smem.c +9 -0

+		dev_err(smem->dev,
+			"Global partition has invalid free pointer\n");
+		return -EINVAL;
+	}
+
+	smem->global_partition = header;
+	smem->global_cacheline = le32_to_cpu(entry->cacheline);
+
+	return 0;
  }
 
  static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,

@@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
  		host0 = le16_to_cpu(entry->host0);
  		host1 = le16_to_cpu(entry->host1);

Quoted 12 lines

drivers/soc/qcom/smem.c static int qcom_smem_enumerate_partitions(struct qcom_smem *smem, · line 722
Quoted 6 lines
drivers/soc/qcom/smem.c +6 -0
+		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
+			if (qcom_smem_set_global_partition(smem, entry))
+				return -EINVAL;
+			continue;
+		}
+
As you're not able to leverage any of the checks from this loop I think it's cleaner to duplicate the traversal of the partition table in both functions and call the "search for global partition" directly from probe - if the version indicates there should be one.

Ok, will set the global partition in the version case statement and error out of the probe if finding the global partition fails since it is not optional in the new version.

Quoted 6 lines

drivers/soc/qcom/smem.c +0 -0

  		if (host0 != local_host && host1 != local_host)
  			continue;

@@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
  	}

Quoted 8 lines

drivers/soc/qcom/smem.c static int qcom_smem_probe(struct platform_device *pdev) · line 860
Quoted 5 lines
drivers/soc/qcom/smem.c +3 -1
  	version = qcom_smem_get_sbl_version(smem);
-	if (version >> 16 != SMEM_EXPECTED_VERSION) {
+	switch (version >> 16) {
+	case SMEM_GLOBAL_PART_VERSION:
+	case SMEM_GLOBAL_HEAP_VERSION:
As Arun pointed out, there should be a "break" here.

Will change.

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Bjorn Andersson

2017-08-23 00:32 UTC #5 lore raw

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

Quoted 1 line · Chris Lew

On 8/21/2017 10:17 AM, Bjorn Andersson wrote:

[..]

Quoted 3 lines

drivers/soc/qcom/smem.c +3 -0

+static int qcom_smem_set_global_partition(struct qcom_smem *smem,
+				struct smem_ptable_entry *entry)
+{

[..]

Quoted 11 lines

Quoted 5 lines
drivers/soc/qcom/smem.c int qcom_smem_get_free_space(unsigned host) · line 673
Quoted 1 line
drivers/soc/qcom/smem.c +1 -0
+	if (header->size != entry->size) {
This happens to work, because they are both in the same endian. But please sprinkle some le32_to_cpu() here as well.
These checks mimic the sanity checks being done in enumerate_partitions. Should I create a patch to increase le32_to_cpu usage in qcom_smem_enumerate_partitions?

Oops, yeah please do, just for completeness sake.

[..]

Quoted 17 lines

Quoted 6 lines
Quoted 6 lines
drivers/soc/qcom/smem.c +3 -0
@@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
  		host0 = le16_to_cpu(entry->host0);
  		host1 = le16_to_cpu(entry->host1);
+		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
+			if (qcom_smem_set_global_partition(smem, entry))
+				return -EINVAL;
Quoted 9 lines
drivers/soc/qcom/smem.c static int qcom_smem_enumerate_partitions(struct qcom_smem *smem, · line 722
Quoted 3 lines
drivers/soc/qcom/smem.c +3 -0
+			continue;
+		}
+
As you're not able to leverage any of the checks from this loop I think it's cleaner to duplicate the traversal of the partition table in both functions and call the "search for global partition" directly from probe - if the version indicates there should be one.

Quoted 4 lines

Ok, will set the global partition in the version case statement and error out of the probe if finding the global partition fails since it is not optional in the new version.

Sounds good.

Regards,
Bjorn

Chris Lew

author

2017-08-18 01:15 UTC #6 lore raw

PATCH 2/3 soc: qcom: smem: Support dynamic item limit

In reply to Chris Lew

In V12 SMEM, SBL writes SMEM parameter information after the TOC. Use the SBL provided item count as the max item number.

Signed-off-by Chris Lew <[email protected]>

---
 drivers/soc/qcom/smem.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

drivers/soc/qcom/smem.c +39 -2

index fed2934d6bda..a51f4ba42173 100644
@@ -225,6 +225,24 @@ struct smem_private_entry {
 #define SMEM_PRIVATE_CANARY	0xa5a5
 
 /**
+ * struct smem_info - smem region info located after the table of contents
+ * @magic:	magic number, must be SMEM_INFO_MAGIC
+ * @size:	size of the smem region
+ * @base_addr:	base address of the smem region
+ * @reserved: for now reserved entry
+ * @num_items: highest accepted item number
+ */
+struct smem_info {
+	u8 magic[4];
+	__le32 size;
+	__le32 base_addr;
+	__le32 reserved;
+	__le32 num_items;
+};
+
+static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
+
+/**
  * struct smem_region - representation of a chunk of memory used for smem
  * @aux_base:	identifier of aux_mem base
  * @virt_base:	virtual base address of memory with this aux_mem identifier

@@ -245,6 +263,7 @@ struct smem_region {
  * @partitions:	list of pointers to partitions affecting the current
  *		processor/host
  * @cacheline:	list of cacheline sizes for each host
+ * @item_count: max accepted item number
  * @num_regions: number of @regions
  * @regions:	list of the memory regions defining the shared memory
  */

@@ -257,6 +276,7 @@ struct qcom_smem {
 	size_t global_cacheline;
 	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
 	size_t cacheline[SMEM_HOST_COUNT];
+	u32 item_count;
 
 	unsigned num_regions;
 	struct smem_region regions[0];

@@ -388,7 +408,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
 	struct smem_header *header;
 	struct smem_global_entry *entry;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
+	if (WARN_ON(item >= smem->item_count))
 		return -EINVAL;
 
 	header = smem->regions[0].virt_base;

@@ -473,7 +493,7 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
 	u32 aux_base;
 	unsigned i;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
+	if (WARN_ON(item >= smem->item_count))
 		return ERR_PTR(-EINVAL);
 
 	header = smem->regions[0].virt_base;

@@ -640,6 +660,19 @@ static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
 }
 
+static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
+{
+	struct smem_ptable *ptable;
+	struct smem_info *info;
+
+	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
+	info = (struct smem_info *)&ptable->entry[ptable->num_entries];
+	if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
+		return SMEM_ITEM_COUNT;
+
+	return le32_to_cpu(info->num_items);
+}
+
 static int qcom_smem_set_global_partition(struct qcom_smem *smem,
 				struct smem_ptable_entry *entry)
 {

@@ -857,7 +890,11 @@ static int qcom_smem_probe(struct platform_device *pdev)
 	version = qcom_smem_get_sbl_version(smem);
 	switch (version >> 16) {
 	case SMEM_GLOBAL_PART_VERSION:
+		smem->item_count = qcom_smem_get_dynamic_item(smem);
+		break;
 	case SMEM_GLOBAL_HEAP_VERSION:
+		smem->item_count = SMEM_ITEM_COUNT;
+		break;
 	default:
 		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
 		return -EINVAL;

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Arun Kumar Neelakantam

2017-08-21 08:57 UTC #7 lore raw

Re: [PATCH 2/3] soc: qcom: smem: Support dynamic item limit

Quoted 20 lines · Chris Lew

In V12 SMEM, SBL writes SMEM parameter information after the TOC. Use the SBL provided item count as the max item number.

Signed-off-by Chris Lew <[email protected]>

---
  drivers/soc/qcom/smem.c | 41 +++++++++++++++++++++++++++++++++++++++--
  1 file changed, 39 insertions(+), 2 deletions(-)

drivers/soc/qcom/smem.c +3 -0

index fed2934d6bda..a51f4ba42173 100644
@@ -225,6 +225,24 @@ struct smem_private_entry {
  #define SMEM_PRIVATE_CANARY	0xa5a5
 
  /**
+ * struct smem_info - smem region info located after the table of contents
+ * @magic:	magic number, must be SMEM_INFO_MAGIC
+ * @size:	size of the smem region

drivers/soc/qcom/smem.c struct smem_private_entry { · line 240

Quoted 10 lines

drivers/soc/qcom/smem.c +10 -0

+ * @base_addr:	base address of the smem region
+ * @reserved: for now reserved entry
+ * @num_items: highest accepted item number
+ */
+struct smem_info {
+	u8 magic[4];
+	__le32 size;
+	__le32 base_addr;
+	__le32 reserved;
+	__le32 num_items;

The num_items is first 16bits of reserved field, we dont have 32bit reserved and 32 bit for num_items

Quoted 25 lines

drivers/soc/qcom/smem.c +7 -0

+};
+
+static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
+
+/**
   * struct smem_region - representation of a chunk of memory used for smem
   * @aux_base:	identifier of aux_mem base
   * @virt_base:	virtual base address of memory with this aux_mem identifier

@@ -245,6 +263,7 @@ struct smem_region {
   * @partitions:	list of pointers to partitions affecting the current
   *		processor/host
   * @cacheline:	list of cacheline sizes for each host
+ * @item_count: max accepted item number
   * @num_regions: number of @regions
   * @regions:	list of the memory regions defining the shared memory
   */

@@ -257,6 +276,7 @@ struct qcom_smem {
  	size_t global_cacheline;
  	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
  	size_t cacheline[SMEM_HOST_COUNT];
+	u32 item_count;
 
  	unsigned num_regions;
  	struct smem_region regions[0];

drivers/soc/qcom/smem.c static int qcom_smem_alloc_global(struct qcom_smem *smem, · line 411

Quoted 5 lines

drivers/soc/qcom/smem.c +1 -1

@@ -388,7 +408,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
  	struct smem_header *header;
  	struct smem_global_entry *entry;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
+	if (WARN_ON(item >= smem->item_count))

can you please add this item_count check from partition alloc/get also if avoid any unknown clients usage of smem

Quoted 44 lines

drivers/soc/qcom/smem.c +18 -1

  		return -EINVAL;
 
  	header = smem->regions[0].virt_base;

@@ -473,7 +493,7 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
  	u32 aux_base;
  	unsigned i;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
+	if (WARN_ON(item >= smem->item_count))
  		return ERR_PTR(-EINVAL);
 
  	header = smem->regions[0].virt_base;

@@ -640,6 +660,19 @@ static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
  	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
  }
 
+static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
+{
+	struct smem_ptable *ptable;
+	struct smem_info *info;
+
+	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
+	info = (struct smem_info *)&ptable->entry[ptable->num_entries];
+	if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
+		return SMEM_ITEM_COUNT;
+
+	return le32_to_cpu(info->num_items);
+}
+
  static int qcom_smem_set_global_partition(struct qcom_smem *smem,
  				struct smem_ptable_entry *entry)
  {

@@ -857,7 +890,11 @@ static int qcom_smem_probe(struct platform_device *pdev)
  	version = qcom_smem_get_sbl_version(smem);
  	switch (version >> 16) {
  	case SMEM_GLOBAL_PART_VERSION:
+		smem->item_count = qcom_smem_get_dynamic_item(smem);
+		break;
  	case SMEM_GLOBAL_HEAP_VERSION:
+		smem->item_count = SMEM_ITEM_COUNT;
+		break;
  	default:
  		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
  		return -EINVAL;

Bjorn Andersson

2017-08-21 17:33 UTC #8 lore raw

Re: [PATCH 2/3] soc: qcom: smem: Support dynamic item limit

In reply to Chris Lew

Quoted 14 lines · Chris Lew

In V12 SMEM, SBL writes SMEM parameter information after the TOC. Use the SBL provided item count as the max item number.
Signed-off-by Chris Lew <[email protected]>
---
 drivers/soc/qcom/smem.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)
drivers/soc/qcom/smem.c +0 -0
index fed2934d6bda..a51f4ba42173 100644

drivers/soc/qcom/smem.c struct smem_private_entry { · line 233

Quoted 9 lines

drivers/soc/qcom/smem.c +6 -0

@@ -225,6 +225,24 @@ struct smem_private_entry {
 #define SMEM_PRIVATE_CANARY	0xa5a5
 
 /**
+ * struct smem_info - smem region info located after the table of contents
+ * @magic:	magic number, must be SMEM_INFO_MAGIC
+ * @size:	size of the smem region
+ * @base_addr:	base address of the smem region
+ * @reserved: for now reserved entry
+ * @num_items: highest accepted item number

Try to make the indentation of the descriptions a little bit more consistent.

drivers/soc/qcom/smem.c struct smem_private_entry { · line 238

Quoted 5 lines

drivers/soc/qcom/smem.c +5 -0

+ */
+struct smem_info {
+	u8 magic[4];
+	__le32 size;
+	__le32 base_addr;

May I ask why you keep the size and address of SMEM here? Didn't we just use that information to find this item?

Quoted 45 lines

drivers/soc/qcom/smem.c +11 -2

+	__le32 reserved;
+	__le32 num_items;
+};
+
+static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
+
+/**
  * struct smem_region - representation of a chunk of memory used for smem
  * @aux_base:	identifier of aux_mem base
  * @virt_base:	virtual base address of memory with this aux_mem identifier

@@ -245,6 +263,7 @@ struct smem_region {
  * @partitions:	list of pointers to partitions affecting the current
  *		processor/host
  * @cacheline:	list of cacheline sizes for each host
+ * @item_count: max accepted item number
  * @num_regions: number of @regions
  * @regions:	list of the memory regions defining the shared memory
  */

@@ -257,6 +276,7 @@ struct qcom_smem {
 	size_t global_cacheline;
 	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
 	size_t cacheline[SMEM_HOST_COUNT];
+	u32 item_count;
 
 	unsigned num_regions;
 	struct smem_region regions[0];

@@ -388,7 +408,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
 	struct smem_header *header;
 	struct smem_global_entry *entry;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
+	if (WARN_ON(item >= smem->item_count))
 		return -EINVAL;
 
 	header = smem->regions[0].virt_base;

@@ -473,7 +493,7 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
 	u32 aux_base;
 	unsigned i;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
+	if (WARN_ON(item >= smem->item_count))
 		return ERR_PTR(-EINVAL);
 
 	header = smem->regions[0].virt_base;

drivers/soc/qcom/smem.c static int qcom_smem_get_sbl_version(struct qcom_smem *smem) · line 668

Quoted 9 lines

drivers/soc/qcom/smem.c +6 -0

@@ -640,6 +660,19 @@ static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
 }
 
+static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
+{
+	struct smem_ptable *ptable;
+	struct smem_info *info;
+
+	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;

This should be expanded to check the magic and version of the ptable, but per my suggestion of duplicating this logic in patch 1 this should likely just be broken out into a "get pointer to valid ptable"-function.

drivers/soc/qcom/smem.c static int qcom_smem_get_sbl_version(struct qcom_smem *smem) · line 674

Quoted 6 lines

drivers/soc/qcom/smem.c +6 -0

+	info = (struct smem_info *)&ptable->entry[ptable->num_entries];
+	if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
+		return SMEM_ITEM_COUNT;
+
+	return le32_to_cpu(info->num_items);
+}

Other than that this looks good.

Regards,
Bjorn

Chris Lew

author

2017-08-18 01:15 UTC #9 lore raw

PATCH 3/3 soc: qcom: smem: Increase the number of hosts

In reply to Chris Lew

Increase the maximum number of hosts in a system to 10.

Signed-off-by Chris Lew <[email protected]>

---
 drivers/soc/qcom/smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/soc/qcom/smem.c +1 -1

index a51f4ba42173..4385f3b4bca9 100644
@@ -93,7 +93,7 @@
 #define SMEM_HOST_APPS		0
 
 /* Max number of processors/hosts in a system */
-#define SMEM_HOST_COUNT		9
+#define SMEM_HOST_COUNT		10
 
 /**
   * struct smem_proc_comm - proc_comm communication struct (legacy)

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Bjorn Andersson

Acked-by on v1 3/3

2017-08-21 17:34 UTC #10 lore raw

Re: [PATCH 3/3] soc: qcom: smem: Increase the number of hosts

Quoted 3 lines · Chris Lew

Increase the maximum number of hosts in a system to 10.
Signed-off-by Chris Lew <[email protected]>

Acked-by Bjorn Andersson <[email protected]>

Regards,
Bjorn

Quoted 21 lines

---
 drivers/soc/qcom/smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/soc/qcom/smem.c +1 -1

index a51f4ba42173..4385f3b4bca9 100644
@@ -93,7 +93,7 @@
 #define SMEM_HOST_APPS		0
 
 /* Max number of processors/hosts in a system */
-#define SMEM_HOST_COUNT		9
+#define SMEM_HOST_COUNT		10
 
 /**
   * struct smem_proc_comm - proc_comm communication struct (legacy)

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project