[PATCH v2 0/5] Qualcomm SMEM V12 Support

[PATCH v2 0/5] Qualcomm SMEM V12 Support

[Chris Lew]

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 (5):
  soc: qcom: smem: Use le32_to_cpu for partition size comparison
  soc: qcom: smem: Read version by using the smem header
  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 | 248 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 195 insertions(+), 53 deletions(-)

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

[PATCH v2 1/5] soc: qcom: smem: Use le32_to_cpu for partition size comparison

[Chris Lew]

Endianness can vary in the system, add le32_to_cpu when comparing
size values from smem.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---

Changes since v1:
- New change

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

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index c28275be0038..db04c45d4132 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -698,7 +698,7 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
 			return -EINVAL;
 		}
 
-		if (header->size != entry->size) {
+		if (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {
 			dev_err(smem->dev,
 				"Partition %d has invalid size\n", i);
 			return -EINVAL;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/5] soc: qcom: smem: Read version by using the smem header

[Chris Lew]

The SMEM header structure includes the version information.
Read the version directly from the header instead of getting
an item from the global heap.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---

Changes since v1:
- Remove unused smem item version macro
- Move smem get version change to separate commit

 drivers/soc/qcom/smem.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index db04c45d4132..540322ae409e 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -63,13 +63,12 @@
  */
 
 /*
- * 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.
+ * The version member of the smem header contains an array of versions for the
+ * various software components in the SoC. We verify that the boot loader
+ * version is 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_MASTER_SBL_VERSION_INDEX	7
+#define SMEM_EXPECTED_VERSION		11
 
 /*
  * The first 8 items are only to be allocated by the boot loader while
@@ -604,19 +603,11 @@ 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;
-	}
 
-	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
-		dev_err(smem->dev, "Version item is too small\n");
-		return -EINVAL;
-	}
+	header = smem->regions[0].virt_base;
+	versions = header->version;
 
 	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/5] soc: qcom: smem: Support global partition

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

Signed-off-by: Chris Lew <clew@codeaurora.org>
---

Changes since v1:
- Move V12 descriptions to top comment
- Add cacheline support to global partition
- Add ptable get helper function
- Move global partition init to version check

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

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 540322ae409e..88d5ec663304 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -55,6 +55,10 @@
  * is hence the region between the cached and non-cached offsets. The header of
  * cached items comes after the data.
  *
+ * 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.
  *
  * To synchronize allocations in the shared memory heaps a remote spinlock must
  * be held - currently lock number 3 of the sfpb or tcsr is used for this on all
@@ -68,7 +72,8 @@
  * version is a valid version as a sanity check.
  */
 #define SMEM_MASTER_SBL_VERSION_INDEX	7
-#define SMEM_EXPECTED_VERSION		11
+#define SMEM_GLOBAL_HEAP_VERSION	11
+#define SMEM_GLOBAL_PART_VERSION	12
 
 /*
  * The first 8 items are only to be allocated by the boot loader while
@@ -82,6 +87,9 @@
 /* Processor/host identifier for the application processor */
 #define SMEM_HOST_APPS		0
 
+/* Processor/host identifier for the global partition */
+#define SMEM_GLOBAL_HOST	0xfffe
+
 /* Max number of processors/hosts in a system */
 #define SMEM_HOST_COUNT		9
 
@@ -230,6 +238,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
@@ -241,6 +251,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];
 
@@ -317,16 +329,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);
@@ -334,8 +344,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;
 		}
 
@@ -373,8 +383,8 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
 				  unsigned item,
 				  size_t size)
 {
-	struct smem_header *header;
 	struct smem_global_entry *entry;
+	struct smem_header *header;
 
 	if (WARN_ON(item >= SMEM_ITEM_COUNT))
 		return -EINVAL;
@@ -416,6 +426,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
  */
 int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 {
+	struct smem_partition_header *phdr;
 	unsigned long flags;
 	int ret;
 
@@ -434,10 +445,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);
 
@@ -479,16 +495,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);
@@ -531,7 +543,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);
 }
@@ -547,7 +560,9 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
  */
 void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
 {
+	struct smem_partition_header *phdr;
 	unsigned long flags;
+	size_t cacheln;
 	int ret;
 	void *ptr = ERR_PTR(-EPROBE_DEFER);
 
@@ -560,10 +575,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);
 
@@ -592,6 +614,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);
@@ -612,27 +638,106 @@ static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
 }
 
-static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
-					  unsigned local_host)
+static struct smem_ptable *qcom_smem_get_ptable(struct qcom_smem *smem)
 {
-	struct smem_partition_header *header;
-	struct smem_ptable_entry *entry;
 	struct smem_ptable *ptable;
-	unsigned remote_host;
-	u32 version, host0, host1;
-	int i;
+	u32 version;
 
 	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
 	if (memcmp(ptable->magic, SMEM_PTABLE_MAGIC, sizeof(ptable->magic)))
-		return 0;
+		return NULL;
 
 	version = le32_to_cpu(ptable->version);
 	if (version != 1) {
 		dev_err(smem->dev,
 			"Unsupported partition header version %d\n", version);
+		return ERR_PTR(-EINVAL);
+	}
+	return ptable;
+}
+
+static int qcom_smem_set_global_partition(struct qcom_smem *smem)
+{
+	struct smem_partition_header *header;
+	struct smem_ptable_entry *entry = NULL;
+	struct smem_ptable *ptable;
+	u32 host0, host1, size;
+	int i;
+
+	ptable = qcom_smem_get_ptable(smem);
+	if (IS_ERR_OR_NULL(ptable))
+		return -EINVAL;
+
+	for (i = 0; i < le32_to_cpu(ptable->num_entries); i++) {
+		entry = &ptable->entry[i];
+		host0 = le16_to_cpu(entry->host0);
+		host1 = le16_to_cpu(entry->host1);
+
+		if (host0 == SMEM_GLOBAL_HOST && host0 == host1)
+			break;
+	}
+
+	if (!entry) {
+		dev_err(smem->dev, "Missing entry for global partition\n");
+		return -EINVAL;
+	}
+
+	if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
+		dev_err(smem->dev, "Invalid entry for global partition\n");
+		return -EINVAL;
+	}
+
+	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);
+
+	if (memcmp(header->magic, SMEM_PART_MAGIC, sizeof(header->magic))) {
+		dev_err(smem->dev, "Global 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 (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {
+		dev_err(smem->dev, "Global partition has invalid size\n");
 		return -EINVAL;
 	}
 
+	size = le32_to_cpu(header->offset_free_uncached);
+	if (size > 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,
+					  unsigned int local_host)
+{
+	struct smem_partition_header *header;
+	struct smem_ptable_entry *entry;
+	struct smem_ptable *ptable;
+	unsigned int remote_host;
+	u32 host0, host1;
+	int i;
+
+	ptable = qcom_smem_get_ptable(smem);
+	if (IS_ERR_OR_NULL(ptable))
+		return PTR_ERR(ptable);
+
 	for (i = 0; i < le32_to_cpu(ptable->num_entries); i++) {
 		entry = &ptable->entry[i];
 		host0 = le16_to_cpu(entry->host0);
@@ -773,7 +878,14 @@ 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:
+		ret = qcom_smem_set_global_partition(smem);
+		if (ret < 0)
+			return ret;
+	case SMEM_GLOBAL_HEAP_VERSION:
+		break;
+	default:
 		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
 		return -EINVAL;
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/5] soc: qcom: smem: Support dynamic item limit

[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 <clew@codeaurora.org>
---

Changes since v1:
- Change num_items to __le16 from __le32
- Move max smem item warning to generic get/alloc functions
- Use get ptable helper function

 drivers/soc/qcom/smem.c | 51 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 88d5ec663304..2f3b1e1a8904 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -223,6 +223,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;
+	__le16 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
@@ -243,6 +261,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
  */
@@ -255,6 +274,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];
@@ -386,9 +406,6 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
 	struct smem_global_entry *entry;
 	struct smem_header *header;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
-		return -EINVAL;
-
 	header = smem->regions[0].virt_base;
 	entry = &header->toc[item];
 	if (entry->allocated)
@@ -439,6 +456,9 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 		return -EINVAL;
 	}
 
+	if (WARN_ON(item >= __smem->item_count))
+		return -EINVAL;
+
 	ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
 					  HWSPINLOCK_TIMEOUT,
 					  &flags);
@@ -471,9 +491,6 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
 	u32 aux_base;
 	unsigned i;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
-		return ERR_PTR(-EINVAL);
-
 	header = smem->regions[0].virt_base;
 	entry = &header->toc[item];
 	if (!entry->allocated)
@@ -569,6 +586,9 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
 	if (!__smem)
 		return ptr;
 
+	if (WARN_ON(item >= __smem->item_count))
+		return ERR_PTR(-EINVAL);
+
 	ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
 					  HWSPINLOCK_TIMEOUT,
 					  &flags);
@@ -656,6 +676,22 @@ static struct smem_ptable *qcom_smem_get_ptable(struct qcom_smem *smem)
 	return ptable;
 }
 
+static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
+{
+	struct smem_ptable *ptable;
+	struct smem_info *info;
+
+	ptable = qcom_smem_get_ptable(smem);
+	if (IS_ERR_OR_NULL(ptable))
+		return SMEM_ITEM_COUNT;
+
+	info = (struct smem_info *)&ptable->entry[ptable->num_entries];
+	if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
+		return SMEM_ITEM_COUNT;
+
+	return le16_to_cpu(info->num_items);
+}
+
 static int qcom_smem_set_global_partition(struct qcom_smem *smem)
 {
 	struct smem_partition_header *header;
@@ -883,7 +919,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
 		ret = qcom_smem_set_global_partition(smem);
 		if (ret < 0)
 			return ret;
+		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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 5/5] soc: qcom: smem: Increase the number of hosts

[Chris Lew]

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

Signed-off-by: Chris Lew <clew@codeaurora.org>
---

Changes since v1:
- None

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

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 2f3b1e1a8904..086f31b6c584 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -91,7 +91,7 @@
 #define SMEM_GLOBAL_HOST	0xfffe
 
 /* 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)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 1/5] soc: qcom: smem: Use le32_to_cpu for partition size comparison

[Bjorn Andersson]

On Thu 14 Sep 14:24 PDT 2017, Chris Lew wrote:

> Endianness can vary in the system, add le32_to_cpu when comparing
> size values from smem.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Changes since v1:
> - New change
> 
>  drivers/soc/qcom/smem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index c28275be0038..db04c45d4132 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -698,7 +698,7 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
>  			return -EINVAL;
>  		}
>  
> -		if (header->size != entry->size) {
> +		if (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {
>  			dev_err(smem->dev,
>  				"Partition %d has invalid size\n", i);
>  			return -EINVAL;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2 2/5] soc: qcom: smem: Read version by using the smem header

[Bjorn Andersson]

On Thu 14 Sep 14:24 PDT 2017, Chris Lew wrote:

> The SMEM header structure includes the version information.
> Read the version directly from the header instead of getting
> an item from the global heap.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Changes since v1:
> - Remove unused smem item version macro
> - Move smem get version change to separate commit
> 
>  drivers/soc/qcom/smem.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index db04c45d4132..540322ae409e 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -63,13 +63,12 @@
>   */
>  
>  /*
> - * 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.
> + * The version member of the smem header contains an array of versions for the
> + * various software components in the SoC. We verify that the boot loader
> + * version is 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_MASTER_SBL_VERSION_INDEX	7
> +#define SMEM_EXPECTED_VERSION		11
>  
>  /*
>   * The first 8 items are only to be allocated by the boot loader while
> @@ -604,19 +603,11 @@ 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;
> -	}
>  
> -	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
> -		dev_err(smem->dev, "Version item is too small\n");
> -		return -EINVAL;
> -	}
> +	header = smem->regions[0].virt_base;
> +	versions = header->version;
>  
>  	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

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

[Bjorn Andersson]

On Thu 14 Sep 14:25 PDT 2017, Chris Lew wrote:

[..]
> +static struct smem_ptable *qcom_smem_get_ptable(struct qcom_smem *smem)
>  {
> -	struct smem_partition_header *header;
> -	struct smem_ptable_entry *entry;
>  	struct smem_ptable *ptable;
> -	unsigned remote_host;
> -	u32 version, host0, host1;
> -	int i;
> +	u32 version;
>  
>  	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
>  	if (memcmp(ptable->magic, SMEM_PTABLE_MAGIC, sizeof(ptable->magic)))
> -		return 0;
> +		return NULL;
>  
>  	version = le32_to_cpu(ptable->version);
>  	if (version != 1) {
>  		dev_err(smem->dev,
>  			"Unsupported partition header version %d\n", version);
> +		return ERR_PTR(-EINVAL);

In the calling places NULL and -EINVAL are both treated as -EINVAL, so I
think it's better to just return NULL here as well as check for !ptable
in callers.

Regards,
Bjorn

Re: [PATCH v2 1/5] soc: qcom: smem: Use le32_to_cpu for partition size comparison

[Stephen Boyd]

On 09/14, Chris Lew wrote:
> Endianness can vary in the system, add le32_to_cpu when comparing
> size values from smem.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> ---

Fixes: tag?

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

Re: [PATCH v2 1/5] soc: qcom: smem: Use le32_to_cpu for partition size comparison

[Stephen Boyd]

On 09/14, Chris Lew wrote:
> Endianness can vary in the system, add le32_to_cpu when comparing
> size values from smem.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> ---
> 
> Changes since v1:
> - New change
> 
>  drivers/soc/qcom/smem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index c28275be0038..db04c45d4132 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -698,7 +698,7 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
>  			return -EINVAL;
>  		}
>  
> -		if (header->size != entry->size) {
> +		if (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {

Also, it doesn't really matter. We're comparing two numbers with
the same endianness, so comparing them for equality before or
after swapping makes no difference. Sparse also (correctly)
doesn't complain here, because adding the conversion is not
necessary. Drop this patch?

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

Re: [PATCH v2 4/5] soc: qcom: smem: Support dynamic item limit

[Bjorn Andersson]

On Thu 14 Sep 14:25 PDT 2017, Chris Lew wrote:

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

Please wrap your commit messages at 72 chars (50 for the subject).

[..]
> +static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)

This naming is too similar to the functions for looking up items, please
rename it to "qcom_smem_get_item_count()".

Regards,
Bjorn

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

[Bjorn Andersson]

On Thu 14 Sep 14:25 PDT 2017, Chris Lew wrote:

> Increase the maximum number of hosts in a system to 10.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> Changes since v1:
> - None
> 
>  drivers/soc/qcom/smem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 2f3b1e1a8904..086f31b6c584 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -91,7 +91,7 @@
>  #define SMEM_GLOBAL_HOST	0xfffe
>  
>  /* 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)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v2 1/5] soc: qcom: smem: Use le32_to_cpu for partition size comparison

[Chris Lew]

On 9/15/2017 11:39 AM, Stephen Boyd wrote:
> On 09/14, Chris Lew wrote:
>> Endianness can vary in the system, add le32_to_cpu when comparing
>> size values from smem.
>>
>> Signed-off-by: Chris Lew <clew@codeaurora.org>
>> ---
>>
>> Changes since v1:
>> - New change
>>
>>   drivers/soc/qcom/smem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>> index c28275be0038..db04c45d4132 100644
>> --- a/drivers/soc/qcom/smem.c
>> +++ b/drivers/soc/qcom/smem.c
>> @@ -698,7 +698,7 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
>>   			return -EINVAL;
>>   		}
>>   
>> -		if (header->size != entry->size) {
>> +		if (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {
> 
> Also, it doesn't really matter. We're comparing two numbers with
> the same endianness, so comparing them for equality before or
> after swapping makes no difference. Sparse also (correctly)
> doesn't complain here, because adding the conversion is not
> necessary. Drop this patch?
> 

Hey Bjorn, should we remove this patch? You had flagged this comparison 
in the first version of the global partition changes.

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

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

[Chris Lew]

On 9/15/2017 11:33 AM, Bjorn Andersson wrote:
> On Thu 14 Sep 14:25 PDT 2017, Chris Lew wrote:
> 
> [..]
>> +static struct smem_ptable *qcom_smem_get_ptable(struct qcom_smem *smem)
>>   {
>> -	struct smem_partition_header *header;
>> -	struct smem_ptable_entry *entry;
>>   	struct smem_ptable *ptable;
>> -	unsigned remote_host;
>> -	u32 version, host0, host1;
>> -	int i;
>> +	u32 version;
>>   
>>   	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
>>   	if (memcmp(ptable->magic, SMEM_PTABLE_MAGIC, sizeof(ptable->magic)))
>> -		return 0;
>> +		return NULL;
>>   
>>   	version = le32_to_cpu(ptable->version);
>>   	if (version != 1) {
>>   		dev_err(smem->dev,
>>   			"Unsupported partition header version %d\n", version);
>> +		return ERR_PTR(-EINVAL);
> 
> In the calling places NULL and -EINVAL are both treated as -EINVAL, so I
> think it's better to just return NULL here as well as check for !ptable
> in callers.
> 
> Regards,
> Bjorn
> 

qcom_smem_enumerate_partitions allowed the partition table to be 
optional before. I want to keep that behavior for V11 where smem might 
only have the global heap. The probe will continue with a NULL/0 return 
from qcom_get_ptable/qcom_smem_enumerate_partitions.

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

Re: [PATCH v2 1/5] soc: qcom: smem: Use le32_to_cpu for partition size comparison

[Bjorn Andersson]

On Fri 15 Sep 13:53 PDT 2017, Chris Lew wrote:

> 
> 
> On 9/15/2017 11:39 AM, Stephen Boyd wrote:
> > On 09/14, Chris Lew wrote:
> > > Endianness can vary in the system, add le32_to_cpu when comparing
> > > size values from smem.
> > > 
> > > Signed-off-by: Chris Lew <clew@codeaurora.org>
> > > ---
> > > 
> > > Changes since v1:
> > > - New change
> > > 
> > >   drivers/soc/qcom/smem.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > index c28275be0038..db04c45d4132 100644
> > > --- a/drivers/soc/qcom/smem.c
> > > +++ b/drivers/soc/qcom/smem.c
> > > @@ -698,7 +698,7 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
> > >   			return -EINVAL;
> > >   		}
> > > -		if (header->size != entry->size) {
> > > +		if (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {
> > 
> > Also, it doesn't really matter. We're comparing two numbers with
> > the same endianness, so comparing them for equality before or
> > after swapping makes no difference. Sparse also (correctly)
> > doesn't complain here, because adding the conversion is not
> > necessary. Drop this patch?
> > 
> 
> Hey Bjorn, should we remove this patch? You had flagged this comparison in
> the first version of the global partition changes.
> 

As Stephen says; it's (technically) unnecessary.

But I think we should take this patch anyways, for the sake of saving
future readers of having to think about why there's one case in the
entire file where we don't do this translation.

Regards,
Bjorn

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

[Bjorn Andersson]

On Fri 15 Sep 14:02 PDT 2017, Chris Lew wrote:

> 
> 
> On 9/15/2017 11:33 AM, Bjorn Andersson wrote:
> > On Thu 14 Sep 14:25 PDT 2017, Chris Lew wrote:
> > 
> > [..]
> > > +static struct smem_ptable *qcom_smem_get_ptable(struct qcom_smem *smem)
> > >   {
> > > -	struct smem_partition_header *header;
> > > -	struct smem_ptable_entry *entry;
> > >   	struct smem_ptable *ptable;
> > > -	unsigned remote_host;
> > > -	u32 version, host0, host1;
> > > -	int i;
> > > +	u32 version;
> > >   	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
> > >   	if (memcmp(ptable->magic, SMEM_PTABLE_MAGIC, sizeof(ptable->magic)))
> > > -		return 0;
> > > +		return NULL;
> > >   	version = le32_to_cpu(ptable->version);
> > >   	if (version != 1) {
> > >   		dev_err(smem->dev,
> > >   			"Unsupported partition header version %d\n", version);
> > > +		return ERR_PTR(-EINVAL);
> > 
> > In the calling places NULL and -EINVAL are both treated as -EINVAL, so I
> > think it's better to just return NULL here as well as check for !ptable
> > in callers.
> > 
> > Regards,
> > Bjorn
> > 
> 
> qcom_smem_enumerate_partitions allowed the partition table to be optional
> before. I want to keep that behavior for V11 where smem might only have the
> global heap. The probe will continue with a NULL/0 return from
> qcom_get_ptable/qcom_smem_enumerate_partitions.
> 

You're right, I missed that detail.

Which allows me to argue that I do not feel that testing with
IS_ERR_OR_NULL() to let a 0 pass through to the caller is not obvious.

I think it's better if you replace the NULL with ERR_PTR(-ENOENT), which
you test and forward in both callers and then for the one caller where
partitions are optional you check if ret < 0 && ret != -ENOENT.

(Alternatively you check for NULL in enumerate_partitions() and return 0
with a nice comment saying that everything is okay)

Regards,
Bjorn