soc: qcom: smem: bug fixes

Cached items are found at the high end of an smem partition. A cached item's shared memory precedes the private entry structure that describes it.

The address of the structure describing the first cached item should be returned by phdr_to_first_cached_entry(). However the function calculates the start address using the wrong structure size.

Fix this by computing the first item's entry structure address by subtracting the size of a private entry structure rather than a partition header structure.

Signed-off-by Alex Elder <[email protected]>

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

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

index 0b94d62fad2b..7f38c5e11440 100644
@@ -292,8 +292,9 @@ static void *phdr_to_first_cached_entry(struct smem_partition_header *phdr,
 					size_t cacheline)
 {
 	void *p = phdr;
+	struct smem_private_entry *e;
 
-	return p + le32_to_cpu(phdr->size) - ALIGN(sizeof(*phdr), cacheline);
+	return p + le32_to_cpu(phdr->size) - ALIGN(sizeof(*e), cacheline);
 }
 
 static void *phdr_to_last_cached_entry(struct smem_partition_header *phdr)

Signature · 1 line

2.14.1

What phdr_to_last_uncached_entry() returns is the address of the start of the free space following all allocated uncached entries. It really doesn't refer to an actual (initialized) private entry
structure. Similarly phdr_to_last_cached_entry() returns the
address of the end of free space, preceding the last allocated cache entry. Change both functions' return type to be pointer to void to reflect this.

Meanwhile, phdr_to_first_cached_entry() really *does* point to a private entry structure, so change its return type to reflect this fact.

Signed-off-by Alex Elder <[email protected]>

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

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

index 7f38c5e11440..3102aa94aec2 100644
@@ -280,7 +280,7 @@ struct qcom_smem {
 	struct smem_region regions[0];
 };
 
-static struct smem_private_entry *
+static void *
 phdr_to_last_uncached_entry(struct smem_partition_header *phdr)
 {
 	void *p = phdr;

@@ -288,7 +288,8 @@ phdr_to_last_uncached_entry(struct smem_partition_header *phdr)
 	return p + le32_to_cpu(phdr->offset_free_uncached);
 }
 
-static void *phdr_to_first_cached_entry(struct smem_partition_header *phdr,
+static struct smem_private_entry *
+phdr_to_first_cached_entry(struct smem_partition_header *phdr,
 					size_t cacheline)
 {
 	void *p = phdr;

@@ -297,7 +298,8 @@ static void *phdr_to_first_cached_entry(struct smem_partition_header *phdr,
 	return p + le32_to_cpu(phdr->size) - ALIGN(sizeof(*e), cacheline);
 }
 
-static void *phdr_to_last_cached_entry(struct smem_partition_header *phdr)
+static void *
+phdr_to_last_cached_entry(struct smem_partition_header *phdr)
 {
 	void *p = phdr;
 

Signature · 6 lines

2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Two places report an error when a partition header is found to not contain the right canary value. The error messages do not properly byte swap the host ids. Fix this, and adjust the format specificier to match the 16-bit unsigned data type.

Move the error handling for a bad canary value to the end of qcom_smem_alloc_private(). This avoids some long lines, and reduces the distraction of handling this unexpected problem.

Signed-off-by Alex Elder <[email protected]>

---
 drivers/soc/qcom/smem.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

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

index 3102aa94aec2..82f0908b90e1 100644
@@ -365,13 +365,8 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
 	cached = phdr_to_last_cached_entry(phdr);
 
 	while (hdr < end) {
-		if (hdr->canary != SMEM_PRIVATE_CANARY) {
-			dev_err(smem->dev,
-				"Found invalid canary in hosts %d:%d partition\n",
-				phdr->host0, phdr->host1);
-			return -EINVAL;
-		}
-
+		if (hdr->canary != SMEM_PRIVATE_CANARY)
+			goto bad_canary;
 		if (le16_to_cpu(hdr->item) == item)
 			return -EEXIST;
 

@@ -400,6 +395,11 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
 	le32_add_cpu(&phdr->offset_free_uncached, alloc_size);
 
 	return 0;
+bad_canary:
+	dev_err(smem->dev, "Found invalid canary in hosts %hu:%hu partition\n",
+		le16_to_cpu(phdr->host0), le16_to_cpu(phdr->host1));
+
+	return -EINVAL;
 }
 
 static int qcom_smem_alloc_global(struct qcom_smem *smem,

@@ -563,8 +563,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 hosts %d:%d partition\n",
-			phdr->host0, phdr->host1);
+	dev_err(smem->dev, "Found invalid canary in hosts %hu:%hu partition\n",
+			le16_to_cpu(phdr->host0), le16_to_cpu(phdr->host1));
 
 	return ERR_PTR(-EINVAL);
 }

Signature · 1 line

2.14.1

It's OK if the space for a newly-allocated uncached entry actually touches the free cached space boundary. It's only a problem if it would cross it.

Signed-off-by Alex Elder <[email protected]>

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

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

index 82f0908b90e1..0ed263055988 100644
@@ -375,7 +375,7 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
 
 	/* Check that we don't grow into the cached region */
 	alloc_size = sizeof(*hdr) + ALIGN(size, 8);
-	if ((void *)hdr + alloc_size >= cached) {
+	if ((void *)hdr + alloc_size > cached) {
 		dev_err(smem->dev, "Out of memory\n");
 		return -ENOSPC;
 	}

Signature · 1 line

2.14.1

If there is at least one entry in the partition table, but no global entry, the qcom_smem_set_global_partition() should return an error just like it does if there are no partition table entries.

It turns out the function still returns an error in this case, but it waits to do so until it has mistakenly treated the last entry in the table as if it were the global entry found.

Fix the function to return immediately if no global entry is found in the table.

Signed-off-by Alex Elder <[email protected]>

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

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

index 0ed263055988..6e42599b70d4 100644
@@ -698,9 +698,10 @@ static u32 qcom_smem_get_item_count(struct qcom_smem *smem)
 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_entry *entry;
 	struct smem_ptable *ptable;
 	u32 host0, host1, size;
+	bool found = false;
 	int i;
 
 	ptable = qcom_smem_get_ptable(smem);

@@ -712,11 +713,13 @@ static int qcom_smem_set_global_partition(struct qcom_smem *smem)
 		host0 = le16_to_cpu(entry->host0);
 		host1 = le16_to_cpu(entry->host1);
 
-		if (host0 == SMEM_GLOBAL_HOST && host0 == host1)
+		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
+			found = true;
 			break;
+		}
 	}
 
-	if (!entry) {
+	if (!found) {
 		dev_err(smem->dev, "Missing entry for global partition\n");
 		return -EINVAL;
 	}

Signature · 6 lines

2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

There's no sense in scanning the partition table again if we know the global partition has already been discovered. Check for a non-null global_partition pointer in qcom_smem_set_global_partition() immediately.

Signed-off-by Alex Elder <[email protected]>

---
 drivers/soc/qcom/smem.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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

index 6e42599b70d4..7d9a43da5084 100644
@@ -364,6 +364,11 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
 	end = phdr_to_last_uncached_entry(phdr);
 	cached = phdr_to_last_cached_entry(phdr);
 
+	if (smem->global_partition) {
+		dev_err(smem->dev, "Already found the global partition\n");
+		return -EINVAL;
+	}
+
 	while (hdr < end) {
 		if (hdr->canary != SMEM_PRIVATE_CANARY)
 			goto bad_canary;

@@ -729,11 +734,6 @@ static int qcom_smem_set_global_partition(struct qcom_smem *smem)
 		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);

Signature · 1 line

2.14.1

This series contains mostly bug fixes to the Qualcomm shared memory ("smem") code. Although the bugs are real, they have most likely never been observed.

-Alex

Alex Elder (6):
soc: qcom: smem: fix first cache entry calculation
soc: qcom: smem: return proper type for cached entry functions
soc: qcom: smem: byte swap values properly
soc: qcom: smem: fix off-by-one error in qcom_smem_alloc_private()
soc: qcom: smem: fix qcom_smem_set_global_partition()
soc: qcom: smem: check sooner in qcom_smem_set_global_partition()

 drivers/soc/qcom/smem.c | 50 +++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

Quoted 5 lines · Alex Elder

This series contains mostly bug fixes to the Qualcomm shared memory ("smem") code. Although the bugs are real, they have most likely never been observed.

-Alex

Ping? -Alex

Quoted 5 lines · Alex Elder

Quoted 4 lines · Alex Elder

This series contains mostly bug fixes to the Qualcomm shared memory ("smem") code. Although the bugs are real, they have most likely never been observed.

Quoted 3 lines

Quoted 1 line

-Alex

Ping? -Alex

It's only been a couple of weeks =P That said, I looked over all of these and they look reasonable. I'd like Bjorn to take a look as well.

Andy