Date: Mon, 21 Aug 2017 10:17:33 -0700
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Chris Lew <clew@codeaurora.org>
Cc: andy.gross@linaro.org, david.brown@linaro.org, aneela@codeaurora.org,
        linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] soc: qcom: smem: Support global partition
Message-ID: <20170821171733.GU29306@minitux>
References: <1503018948-26629-1-git-send-email-clew@codeaurora.org>
 <1503018948-26629-2-git-send-email-clew@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1503018948-26629-2-git-send-email-clew@codeaurora.org>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5700
Lines: 178

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@codeaurora.org>
> ---
>  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