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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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(-)
>
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
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 <[email protected]>
Sorry, I didn't spot these on the mailing list.
Reviewed-by: Bjorn Andersson <[email protected]>
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
>
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 <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
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
>
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 <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
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
>
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 <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
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
>
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
>