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

[Bjorn Andersson]

On Thu 23 Apr 14:18 PDT 2015, Jeffrey Hugo wrote:

> This is different, but I rather quite like its simplicity.  Some nits
> and requested clarifications below....
> 

Thanks.

> On 4/11/2015 5:32 PM, Bjorn Andersson wrote:
> > The Shared Memory Manager driver implements an interface for allocating
> > and accessing items in the memory area shared among all of the
> > processors in a Qualcomm platform.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxx>
[..]
> > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
[..]
> > +
> > +/*
> > + * The Qualcomm shared memory system is a allocate only heap structure that
> 
> "an allocate"
> 

Thanks

[..]
> > +/**
> > +  * struct smem_proc_comm - proc_comm communication struct (legacy)
> > +  * @command:        current command to be executed
> > +  * @status: status of the currently requested command
> > +  * @params: parameters to the command
> > +  */
> 
> Comment block does not appear to be correctly lined up.  Lines after the
> "/**" should be lined up with the first *, not the second *.
> 

Thanks

[..]
> > +/**
> > + * struct smem_header - header found in beginning of primary smem region
> > + * @proc_comm:               proc_comm communication interface (legacy)
> > + * @version:         array of versions for the various subsystems
> > + * @initialized:     boolean to indicate that smem is initialized
> > + * @free_offset:     index of the first unallocated byte in smem
> > + * @available:               number of bytes available for allocation
> > + * @reserved:                reserved field, must be 0
> > + * toc:                      array of references to items
> 
> @toc
> 

Thanks

[..]
> > +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.

[..]
> > +/*
> > + * 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.
> > + */
> > +#define SMEM_ITEM_VERSION    3
> > +#define  SMEM_MASTER_SBL_VERSION_INDEX       7
> > +#define  SMEM_EXPECTED_VERSION               11
> 
> Any particular reason why some of these defines lead with 2 spaces, and
> some with 1 space?
> 

It's intentional to indicate the index is a "child" of the item. Maybe I
should just revise the naming of these, I will give it some more though.

[..]
> > +/* Max number of processors/hosts in a system */
> > +#define SMEM_HOST_COUNT              7
> 
> Go ahead and make this 9.  Downstream just added 2 Hosts.
> 

Thanks, will do.

[..]
> > +/* 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.

[..]
> > +static int qcom_smem_alloc_private(struct qcom_smem *smem,
> > +                                unsigned host,
> > +                                unsigned item,
> > +                                size_t size)
> > +{
> > +     struct smem_partition_header *phdr;
> > +     struct smem_private_entry *hdr;
> > +     size_t alloc_size;
> > +     void *p;
> > +
> > +     /* We're not going to find it if there's no matching partition */
> > +     if (host >= SMEM_HOST_COUNT || !smem->partitions[host])
> > +             return -ENOENT;
> > +
> > +     phdr = smem->partitions[host];
> > +
> > +     p = (void *)phdr + sizeof(*phdr);
> > +     while (p < (void *)phdr + phdr->offset_free_uncached) {
> > +             hdr = p;
> > +
> > +             if (hdr->canary != SMEM_PRIVATE_CANARY) {
> > +                     dev_err(smem->dev,
> > +                             "Found invalid canary in host %d partition\n",
> > +                             host);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             if (hdr->item == item)
> > +                     return -EEXIST;
> > +
> > +             p += sizeof(*hdr) + hdr->padding_hdr + hdr->size;
> > +     }
> 
> Why not just use qcom_smem_get_private() instead of duplicating the
> algorithm here?
> 

Excellent question, I'll do so :)

[..]
> > +/**
> > + * qcom_smem_alloc - allocate space for a smem item
> 
> qcom_smem_alloc()
> 

Thanks

[..]
> > +/**
> > + * qcom_smem_get - resolve ptr of size of a smem item
> 
> qcom_smem_get()
> 

Thanks

[..]
> > +/**
> > + * qcom_smem_get_free_space - retrieve amont of free space in a partition
> 
> "qcom_smem_get_free_space()"
> "ammount"
> 

Thanks

> > + * @host:    the remote processor identifing a partition, or -1
> 
> "identifying'
> 

Thanks

> > + *
> > + * 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?


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?

[..]
> > +static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
> > +                                       unsigned local_host)
> > +{
> > +     struct smem_partition_header *header;
> > +     struct smem_ptable_entry *entry;
> > +     struct smem_ptable *ptable;
> > +     unsigned remote_host;
> > +     int i;
> > +
> > +     ptable = smem->regions[0].virt_base + smem->regions[0].size - 4 * 1024;
> 
> SZ_4K?
> 

Sure thing.

> > +     if (ptable->magic != SMEM_PTABLE_MAGIC)
> > +             return 0;
> > +
> > +     if (ptable->version != 1) {
> > +             dev_err(smem->dev,
> > +                     "Unsupported partition header version %d\n",
> > +                     ptable->version);
> > +             return -EINVAL;
> > +     }
> > +
> > +     for (i = 0; i < ptable->num_entries; i++) {
> > +             entry = &ptable->entry[i];
> > +
> > +             if (entry->host0 != local_host && entry->host1 != local_host)
> > +                     continue;
> > +
> > +             if (!entry->offset)
> > +                     continue;
> > +
> > +             if (!entry->size)
> > +                     continue;
> > +
> > +             if (entry->host0 == local_host)
> > +                     remote_host = entry->host1;
> > +             else
> > +                     remote_host = entry->host0;
> 
> What about checking to see if the remote_host is valid?  Otherwise the
> following if check can exceed the end of the partitions array.
> 

The only case I can think of would be on new platforms where you've
incremented the number of hosts, but that's probably good enough
motivation. I'll add another check.

> > +
> > +             if (smem->partitions[remote_host]) {
> > +                     dev_err(smem->dev,
> > +                             "Already found a partition for host %d\n",
> > +                             remote_host);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             header = smem->regions[0].virt_base + entry->offset;
> > +
> > +             if (header->magic != SMEM_PART_MAGIC) {
> > +                     dev_err(smem->dev,
> > +                             "Partition %d has invalid magic\n", i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             if (header->host0 != local_host && header->host1 != local_host) {
> > +                     dev_err(smem->dev,
> > +                             "Partition %d hosts are invalid\n", i);
> > +                     return -EINVAL;
> > +             }
> 
> Should we really continue on here?  This is unlikely to happen, but if
> it does, either the bootloader badly glitched, or we've detected memory
> corruption.  Either way, it doesn't seem like the system is going to be
> stable.
> 

Generally I don't like calling panic(), but as you're saying we will hit
these cases during boot and they are catastrophic. I'll give it some
thought, maybe they are better of as a series of BUG_ON().

> > +
> > +             if (header->host0 != remote_host && header->host1 != remote_host) {
> > +                     dev_err(smem->dev,
> > +                             "Partition %d hosts are invalid\n", i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             if (header->size != entry->size) {
> > +                     dev_err(smem->dev,
> > +                             "Partition %d has invalid size\n", i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             if (header->offset_free_uncached > header->size) {
> > +                     dev_err(smem->dev,
> > +                             "Partition %d has invalid free pointer\n", i);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             smem->partitions[remote_host] = header;
> > +     }
> > +
> > +     return 0;
> > +}
[..]
> > diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
[..]
> > +struct device_node;
> > +struct qcom_smem;
> 
> Am I missing something?  I'm not seeing a reason to forward declare these.
> 

No, these are leftovers from development. Sorry about that.

Thanks for the review Jeffrey!

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/