On Thu 17 Aug 18:15 PDT 2017, Chris Lew wrote:
> 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.
>
Welcome to LKML!
This looks quite reasonable, just some minor comments below.
> Signed-off-by: Chris Lew <clew@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/smem.c | 134 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 105 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index c28275be0038..fed2934d6bda 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -65,11 +65,20 @@
> /*
> * 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.
> + * a valid version as a sanity check.
> */
> #define SMEM_ITEM_VERSION 3
SMEM_ITEM_VERSION is now unused, which means that the comment should be
updated with respect to item 3 and the versions array of smem_header.
> #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.
[..]
> @@ -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).
>
> 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.
> +
> +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.
> + 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.
> + 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.
> 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.
> + default:
> dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
> return -EINVAL;
> }
Regards,
Bjorn
--
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
