LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/6] soc: qcom: smem: bug fixes
@ 2018-04-10 22:25 Alex Elder
  2018-04-10 22:25 ` [PATCH 1/6] soc: qcom: smem: fix first cache entry calculation Alex Elder
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Alex Elder @ 2018-04-10 22:25 UTC (permalink / raw)
  To: andy.gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

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(-)

-- 
2.14.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/6] soc: qcom: smem: fix first cache entry calculation
  2018-04-10 22:25 [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
@ 2018-04-10 22:25 ` Alex Elder
  2018-04-25  4:35   ` Bjorn Andersson
  2018-04-10 22:25 ` [PATCH 2/6] soc: qcom: smem: return proper type for cached entry functions Alex Elder
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Alex Elder @ 2018-04-10 22:25 UTC (permalink / raw)
  To: andy.gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

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 <elder@linaro.org>
---
 drivers/soc/qcom/smem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 0b94d62fad2b..7f38c5e11440 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -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)
-- 
2.14.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/6] soc: qcom: smem: return proper type for cached entry functions
  2018-04-10 22:25 [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
  2018-04-10 22:25 ` [PATCH 1/6] soc: qcom: smem: fix first cache entry calculation Alex Elder
@ 2018-04-10 22:25 ` Alex Elder
  2018-04-25  4:39   ` Bjorn Andersson
  2018-04-10 22:25 ` [PATCH 3/6] soc: qcom: smem: byte swap values properly Alex Elder
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Alex Elder @ 2018-04-10 22:25 UTC (permalink / raw)
  To: andy.gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

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 <elder@linaro.org>
---
 drivers/soc/qcom/smem.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 7f38c5e11440..3102aa94aec2 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -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;
 
-- 
2.14.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/6] soc: qcom: smem: byte swap values properly
  2018-04-10 22:25 [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
  2018-04-10 22:25 ` [PATCH 1/6] soc: qcom: smem: fix first cache entry calculation Alex Elder
  2018-04-10 22:25 ` [PATCH 2/6] soc: qcom: smem: return proper type for cached entry functions Alex Elder
@ 2018-04-10 22:25 ` Alex Elder
  2018-04-25  4:44   ` Bjorn Andersson
  2018-04-10 22:25 ` [PATCH 4/6] soc: qcom: smem: fix off-by-one error in qcom_smem_alloc_private() Alex Elder
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Alex Elder @ 2018-04-10 22:25 UTC (permalink / raw)
  To: andy.gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

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 <elder@linaro.org>
---
 drivers/soc/qcom/smem.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 3102aa94aec2..82f0908b90e1 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -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);
 }
-- 
2.14.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/6] soc: qcom: smem: fix off-by-one error in qcom_smem_alloc_private()
  2018-04-10 22:25 [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
                   ` (2 preceding siblings ...)
  2018-04-10 22:25 ` [PATCH 3/6] soc: qcom: smem: byte swap values properly Alex Elder
@ 2018-04-10 22:25 ` Alex Elder
  2018-04-25  4:48   ` Bjorn Andersson
  2018-04-10 22:25 ` [PATCH 5/6] soc: qcom: smem: fix qcom_smem_set_global_partition() Alex Elder
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Alex Elder @ 2018-04-10 22:25 UTC (permalink / raw)
  To: andy.gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

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 <elder@linaro.org>
---
 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 82f0908b90e1..0ed263055988 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -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;
 	}
-- 
2.14.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 5/6] soc: qcom: smem: fix qcom_smem_set_global_partition()
  2018-04-10 22:25 [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
                   ` (3 preceding siblings ...)
  2018-04-10 22:25 ` [PATCH 4/6] soc: qcom: smem: fix off-by-one error in qcom_smem_alloc_private() Alex Elder
@ 2018-04-10 22:25 ` Alex Elder
  2018-04-25  4:50   ` Bjorn Andersson
  2018-04-10 22:25 ` [PATCH 6/6] soc: qcom: smem: check sooner in qcom_smem_set_global_partition() Alex Elder
  2018-04-20 12:11 ` [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
  6 siblings, 1 reply; 16+ messages in thread
From: Alex Elder @ 2018-04-10 22:25 UTC (permalink / raw)
  To: andy.gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

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 <elder@linaro.org>
---
 drivers/soc/qcom/smem.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 0ed263055988..6e42599b70d4 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -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;
 	}
-- 
2.14.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 6/6] soc: qcom: smem: check sooner in qcom_smem_set_global_partition()
  2018-04-10 22:25 [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
                   ` (4 preceding siblings ...)
  2018-04-10 22:25 ` [PATCH 5/6] soc: qcom: smem: fix qcom_smem_set_global_partition() Alex Elder
@ 2018-04-10 22:25 ` Alex Elder
  2018-04-25  4:52   ` Bjorn Andersson
  2018-04-20 12:11 ` [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
  6 siblings, 1 reply; 16+ messages in thread
From: Alex Elder @ 2018-04-10 22:25 UTC (permalink / raw)
  To: andy.gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

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 <elder@linaro.org>
---
 drivers/soc/qcom/smem.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 6e42599b70d4..7d9a43da5084 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -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);
-- 
2.14.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/6] soc: qcom: smem: bug fixes
  2018-04-10 22:25 [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
                   ` (5 preceding siblings ...)
  2018-04-10 22:25 ` [PATCH 6/6] soc: qcom: smem: check sooner in qcom_smem_set_global_partition() Alex Elder
@ 2018-04-20 12:11 ` Alex Elder
  2018-04-25  4:07   ` Andy Gross
  6 siblings, 1 reply; 16+ messages in thread
From: Alex Elder @ 2018-04-20 12:11 UTC (permalink / raw)
  To: andy.gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

On 04/10/2018 05:25 PM, Alex Elder wrote:
> 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

> 
> 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(-)
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/6] soc: qcom: smem: bug fixes
  2018-04-20 12:11 ` [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
@ 2018-04-25  4:07   ` Andy Gross
  2018-04-25 11:37     ` Alex Elder
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Gross @ 2018-04-25  4:07 UTC (permalink / raw)
  To: Alex Elder
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

On Fri, Apr 20, 2018 at 07:11:41AM -0500, Alex Elder wrote:
> On 04/10/2018 05:25 PM, Alex Elder wrote:
> > 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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/6] soc: qcom: smem: fix first cache entry calculation
  2018-04-10 22:25 ` [PATCH 1/6] soc: qcom: smem: fix first cache entry calculation Alex Elder
@ 2018-04-25  4:35   ` Bjorn Andersson
  0 siblings, 0 replies; 16+ messages in thread
From: Bjorn Andersson @ 2018-04-25  4:35 UTC (permalink / raw)
  To: Alex Elder
  Cc: andy.gross, clew, aneela, david.brown, linux-arm-msm, linux-soc,
	linux-kernel

On Tue 10 Apr 15:25 PDT 2018, Alex Elder wrote:

> 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 <elder@linaro.org>

Sorry, I didn't spot these on the mailing list.

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

Regards,
Bjorn

> ---
>  drivers/soc/qcom/smem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 0b94d62fad2b..7f38c5e11440 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -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)
> -- 
> 2.14.1
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] soc: qcom: smem: return proper type for cached entry functions
  2018-04-10 22:25 ` [PATCH 2/6] soc: qcom: smem: return proper type for cached entry functions Alex Elder
@ 2018-04-25  4:39   ` Bjorn Andersson
  0 siblings, 0 replies; 16+ messages in thread
From: Bjorn Andersson @ 2018-04-25  4:39 UTC (permalink / raw)
  To: Alex Elder
  Cc: andy.gross, clew, aneela, david.brown, linux-arm-msm, linux-soc,
	linux-kernel

On Tue 10 Apr 15:25 PDT 2018, Alex Elder wrote:

> 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 <elder@linaro.org>

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

Regards,
Bjorn

> ---
>  drivers/soc/qcom/smem.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 7f38c5e11440..3102aa94aec2 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -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;
>  
> -- 
> 2.14.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/6] soc: qcom: smem: byte swap values properly
  2018-04-10 22:25 ` [PATCH 3/6] soc: qcom: smem: byte swap values properly Alex Elder
@ 2018-04-25  4:44   ` Bjorn Andersson
  0 siblings, 0 replies; 16+ messages in thread
From: Bjorn Andersson @ 2018-04-25  4:44 UTC (permalink / raw)
  To: Alex Elder
  Cc: andy.gross, clew, aneela, david.brown, linux-arm-msm, linux-soc,
	linux-kernel

On Tue 10 Apr 15:25 PDT 2018, Alex Elder wrote:

> 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 <elder@linaro.org>

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

Regards,
Bjorn

> ---
>  drivers/soc/qcom/smem.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 3102aa94aec2..82f0908b90e1 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -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);
>  }
> -- 
> 2.14.1
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/6] soc: qcom: smem: fix off-by-one error in qcom_smem_alloc_private()
  2018-04-10 22:25 ` [PATCH 4/6] soc: qcom: smem: fix off-by-one error in qcom_smem_alloc_private() Alex Elder
@ 2018-04-25  4:48   ` Bjorn Andersson
  0 siblings, 0 replies; 16+ messages in thread
From: Bjorn Andersson @ 2018-04-25  4:48 UTC (permalink / raw)
  To: Alex Elder
  Cc: andy.gross, clew, aneela, david.brown, linux-arm-msm, linux-soc,
	linux-kernel

On Tue 10 Apr 15:25 PDT 2018, Alex Elder wrote:

> 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 <elder@linaro.org>

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

Regards,
Bjorn

> ---
>  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 82f0908b90e1..0ed263055988 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -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;
>  	}
> -- 
> 2.14.1
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/6] soc: qcom: smem: fix qcom_smem_set_global_partition()
  2018-04-10 22:25 ` [PATCH 5/6] soc: qcom: smem: fix qcom_smem_set_global_partition() Alex Elder
@ 2018-04-25  4:50   ` Bjorn Andersson
  0 siblings, 0 replies; 16+ messages in thread
From: Bjorn Andersson @ 2018-04-25  4:50 UTC (permalink / raw)
  To: Alex Elder
  Cc: andy.gross, clew, aneela, david.brown, linux-arm-msm, linux-soc,
	linux-kernel

On Tue 10 Apr 15:25 PDT 2018, Alex Elder wrote:

> 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 <elder@linaro.org>

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

Regards,
Bjorn

> ---
>  drivers/soc/qcom/smem.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 0ed263055988..6e42599b70d4 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -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;
>  	}
> -- 
> 2.14.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6] soc: qcom: smem: check sooner in qcom_smem_set_global_partition()
  2018-04-10 22:25 ` [PATCH 6/6] soc: qcom: smem: check sooner in qcom_smem_set_global_partition() Alex Elder
@ 2018-04-25  4:52   ` Bjorn Andersson
  0 siblings, 0 replies; 16+ messages in thread
From: Bjorn Andersson @ 2018-04-25  4:52 UTC (permalink / raw)
  To: Alex Elder
  Cc: andy.gross, clew, aneela, david.brown, linux-arm-msm, linux-soc,
	linux-kernel

On Tue 10 Apr 15:25 PDT 2018, Alex Elder wrote:

> 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 <elder@linaro.org>

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

Regards,
Bjorn

> ---
>  drivers/soc/qcom/smem.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 6e42599b70d4..7d9a43da5084 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -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);
> -- 
> 2.14.1
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/6] soc: qcom: smem: bug fixes
  2018-04-25  4:07   ` Andy Gross
@ 2018-04-25 11:37     ` Alex Elder
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Elder @ 2018-04-25 11:37 UTC (permalink / raw)
  To: Andy Gross
  Cc: clew, aneela, david.brown, linux-arm-msm, linux-soc, linux-kernel

On 04/24/2018 11:07 PM, Andy Gross wrote:
> On Fri, Apr 20, 2018 at 07:11:41AM -0500, Alex Elder wrote:
>> On 04/10/2018 05:25 PM, Alex Elder wrote:
>>> 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
> 
> 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.

Thanks a lot.  I have another set of patches based on these and I've been
sitting on them.  I don't want to inundate the list.

I see Bjorn has given his OK on all of them (thanks Bjorn!), so I'll send
out my new ones today.

					-Alex

> Andy
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-04-25 11:37 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-10 22:25 [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
2018-04-10 22:25 ` [PATCH 1/6] soc: qcom: smem: fix first cache entry calculation Alex Elder
2018-04-25  4:35   ` Bjorn Andersson
2018-04-10 22:25 ` [PATCH 2/6] soc: qcom: smem: return proper type for cached entry functions Alex Elder
2018-04-25  4:39   ` Bjorn Andersson
2018-04-10 22:25 ` [PATCH 3/6] soc: qcom: smem: byte swap values properly Alex Elder
2018-04-25  4:44   ` Bjorn Andersson
2018-04-10 22:25 ` [PATCH 4/6] soc: qcom: smem: fix off-by-one error in qcom_smem_alloc_private() Alex Elder
2018-04-25  4:48   ` Bjorn Andersson
2018-04-10 22:25 ` [PATCH 5/6] soc: qcom: smem: fix qcom_smem_set_global_partition() Alex Elder
2018-04-25  4:50   ` Bjorn Andersson
2018-04-10 22:25 ` [PATCH 6/6] soc: qcom: smem: check sooner in qcom_smem_set_global_partition() Alex Elder
2018-04-25  4:52   ` Bjorn Andersson
2018-04-20 12:11 ` [PATCH 0/6] soc: qcom: smem: bug fixes Alex Elder
2018-04-25  4:07   ` Andy Gross
2018-04-25 11:37     ` Alex Elder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).