Re: [PATCH 1/3] soc: qcom: smem: Support global partition

[Chris Lew]

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