Hi Bjorn,
Thanks for the review.
On 8/21/2017 10:17 AM, Bjorn Andersson wrote:
#define SMEM_MASTER_SBL_VERSION_INDEX 7
-#define SMEM_EXPECTED_VERSION 11
+#define SMEM_GLOBAL_HEAP_VERSION 11
+
+/*
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
+ */
Please incorporate this paragraph in the larger comment at the top of
the file, so we keep that up to date.
[..]
Will do.
@@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
{
unsigned long flags;
int ret;
+ struct smem_partition_header *phdr;
Sorry for my OCD, but please maintain my reverse XMAS tree (put this
declaration first, as it's the longest).
Ok, will change.
if (!__smem)
return -EPROBE_DEFER;
[..]
@@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host)
static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
{
+ struct smem_header *header;
__le32 *versions;
- size_t size;
- versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
- if (IS_ERR(versions)) {
- dev_err(smem->dev, "Unable to read the version item\n");
- return -ENOENT;
+ header = smem->regions[0].virt_base;
+ versions = header->version;
+
+ return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+}
I would prefer if you split the change of this function into its own
patch, just to clarify that it's unrelated to the new version support.
Ok, will separate the change and adjust the descriptions accordingly.
+
+static int qcom_smem_set_global_partition(struct qcom_smem *smem,
+ struct smem_ptable_entry *entry)
+{
+ struct smem_partition_header *header;
+ u32 host0, host1;
+
+ if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
+ dev_err(smem->dev, "Invalid entry for gloabl partition\n");
+ return -EINVAL;
}
- if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
- dev_err(smem->dev, "Version item is too small\n");
+ if (smem->global_partition) {
+ dev_err(smem->dev, "Already found the global partition\n");
return -EINVAL;
}
+ header = smem->regions[0].virt_base + le32_to_cpu(entry->offset);
+ host0 = le16_to_cpu(header->host0);
+ host1 = le16_to_cpu(header->host1);
- return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+ if (memcmp(header->magic, SMEM_PART_MAGIC,
+ sizeof(header->magic))) {
+ dev_err(smem->dev, "Gloal partition has invalid magic\n");
+ return -EINVAL;
+ }
+
+ if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) {
+ dev_err(smem->dev, "Global partition hosts are invalid\n");
+ return -EINVAL;
+ }
+
+ if (header->size != entry->size) {
This happens to work, because they are both in the same endian. But
please sprinkle some le32_to_cpu() here as well.
These checks mimic the sanity checks being done in enumerate_partitions.
Should I create a patch to increase le32_to_cpu usage in
qcom_smem_enumerate_partitions?
+ dev_err(smem->dev, "Global partition has invalid size\n");
+ return -EINVAL;
+ }
+
+ if (le32_to_cpu(header->offset_free_uncached) >
+ le32_to_cpu(header->size)) {
Consider using local variables in favor of wrapping lines.
Ok, will do.
+ dev_err(smem->dev,
+ "Global partition has invalid free pointer\n");
+ return -EINVAL;
+ }
+
+ smem->global_partition = header;
+ smem->global_cacheline = le32_to_cpu(entry->cacheline);
+
+ return 0;
}
static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
@@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
host0 = le16_to_cpu(entry->host0);
host1 = le16_to_cpu(entry->host1);
+ if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
+ if (qcom_smem_set_global_partition(smem, entry))
+ return -EINVAL;
+ continue;
+ }
+
As you're not able to leverage any of the checks from this loop I think
it's cleaner to duplicate the traversal of the partition table in both
functions and call the "search for global partition" directly from
probe - if the version indicates there should be one.
Ok, will set the global partition in the version case statement and
error out of the probe if finding the global partition fails since it is
not optional in the new version.
if (host0 != local_host && host1 != local_host)
continue;
@@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
}
version = qcom_smem_get_sbl_version(smem);
- if (version >> 16 != SMEM_EXPECTED_VERSION) {
+ switch (version >> 16) {
+ case SMEM_GLOBAL_PART_VERSION:
+ case SMEM_GLOBAL_HEAP_VERSION:
As Arun pointed out, there should be a "break" here.
Will change.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
