Re: [PATCH v2 2/2] soc: qcom: Add Shared Memory Manager driver

[Jeffrey Hugo <jhugo@codeaurora.org>]

[..]

>>> +struct smem_header {
>>> +     struct smem_proc_comm proc_comm[4];
>>> +     u32 version[32];
>>> +     u32 initialized;
>>> +     u32 free_offset;
>>> +     u32 available;
>>> +     u32 reserved;
>>> +     struct smem_global_entry toc[];
>>
>> Was it intentional to not have "toc[512]"?
>>
>
> Not really, I can add it to make it clear that it's a fixed amount.

My personal preference would be to have it toc[512] since whenever I see 
an array with an empty size like this, my first thought is that it is a 
dynamic array which is not the case here.

[..]

>>> +/* Timeout (ms) for the trylock of remote spinlocks */
>>> +#define HWSPINLOCK_TIMEOUT   1000
>>
>> I'm curious what made you pick 1 second as a timeout value?
>>
>
> Sorry, I don't have even a tiny bit of science behind this number. I
> figured it's long enough to not have any false negatives and it's short
> enough to not be intrusive if some remote processor actually dies with
> the lock held.

Darn.  I've been pondering what value would be appropriate since 
reviewing the Hardware Spinlock framework, and had hoped you had already 
figured it out when I saw this.  My gut feeling agrees with your assessment.

[..]

>>> + *
>>> + * To be used by smem clients as a quick way to determine if any new
>>> + * allocations has been made.
>>> + */
>>> +int qcom_smem_get_free_space(unsigned host)
>>> +{
>>> +     struct smem_partition_header *phdr;
>>> +     struct smem_header *header;
>>> +     unsigned ret;
>>> +
>>> +     if (!__smem)
>>> +             return -EPROBE_DEFER;
>>> +
>>> +     if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
>>> +             phdr = __smem->partitions[host];
>>> +             ret = phdr->offset_free_uncached;
>>
>> Hmm.  This will work for the usecase that wants it, but its not really
>> correct based on how this function is described.  Could we fix it up so
>> that it actually returns the free space remaining?
>>
>
> Right, this is wrong.
>
> A potential issue with this api is if a remote processor has a partition
> but even so allocates smd channels from the global space, then checking
> for free space related to said host would not detect any updates.
>
> What is your allocation strategy related to this, would this cause an
> issue for us?

SMEM would allow that, but SMD wouldn't expect it.  SMD was one of the 
major reasons what the partitions came about - SMD is a point-to-point 
communication mechanism, so it doesn't make sense to allow C access to a 
SMD channel between A and B.

 From the SMD perspective, if the partition exists, it should be used. 
I would consider the scenario you propose to be an error and unsupported.

The one possible exception to this, is what the remote processors used 
to do for backwards compatibility for a time.  A remote processor would 
allocate the SMD channel from the partition, and also allocate it from 
the global space, but the global space entry would actually point to the 
allocation in the partition.  I only mention this scenario for 
completeness, since Linux is able to support the partitions, this 
scenario is not valid anymore.

> If so a better implementation would be to drop the argument from this
> function and just sum the free space from all the partitions. At the
> cost of a few extra runs through the channel scanner. What do you think?

I think its unnecessary, and considering that such a calculation would 
run for every interrupt, I'd like to avoid the extra cost.

-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project