Hi Evan,
Thanks for reviewing!
On 06/26/2018 11:57 PM, Evan Green wrote:
> Hi Georgi. Thanks for the new spin of this.
>
> On Wed, Jun 20, 2018 at 5:11 AM Georgi Djakov <georgi.djakov@xxxxxxxxxx> wrote:
>>
>> This patch introduce a new API to get requirements and configure the
>> interconnect buses across the entire chipset to fit with the current
>> demand.
>>
>> The API is using a consumer/provider-based model, where the providers are
>> the interconnect buses and the consumers could be various drivers.
>> The consumers request interconnect resources (path) between endpoints and
>> set the desired constraints on this data flow path. The providers receive
>> requests from consumers and aggregate these requests for all master-slave
>> pairs on that path. Then the providers configure each participating in the
>> topology node according to the requested data flow path, physical links and
>> constraints. The topology could be complicated and multi-tiered and is SoC
>> specific.
[..]
>> +
>> +static struct icc_node *node_find(const int id)
>> +{
>> + struct icc_node *node;
>> +
>> + mutex_lock(&icc_lock);
>> + node = idr_find(&icc_idr, id);
>> + mutex_unlock(&icc_lock);
>
> I wonder if this is too low of a level to be dealing with the lock. I
> notice that everywhere you use this function, you afterwards
> immediately grab the lock and do more stuff. Maybe this function
> should have a comment saying it assumes the lock is already held, and
> then you can grab the lock in the callers, since you're doing that
> anyway.
Ok, will try to do better next time.
>> +
>> + return node;
>> +}
>> +
>> +static struct icc_path *path_allocate(struct icc_node *dst, ssize_t num_nodes)
>> +{
>> + struct icc_node *node = dst;
>> + struct icc_path *path;
>> + size_t i;
>> +
>> + path = kzalloc(sizeof(*path) + num_nodes * sizeof(*path->reqs),
>> + GFP_KERNEL);
>> + if (!path)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + path->num_nodes = num_nodes;
>> +
>> + for (i = 0; i < num_nodes; i++) {
>> + hlist_add_head(&path->reqs[i].req_node, &node->req_list);
>> +
>> + path->reqs[i].node = node;
>> + /* reference to previous node was saved during path traversal */
>> + node = node->reverse;
>> + }
>> +
>> + return path;
>> +}
>> +
>> +static struct icc_path *path_find(struct device *dev, struct icc_node *src,
>> + struct icc_node *dst)
>> +{
>
> I personally prefer a comment somewhere indicating that this function
> assumes icc_lock is already held. Not sure if that's conventional or
> not.
>
Right, Rob and Matthias have also provided useful feedback on this! Thanks!
[..]
>> + /* reset the traversed state */
>> + list_for_each_entry(provider, &icc_provider_list, provider_list) {
>> + list_for_each_entry(n, &provider->nodes, node_list)
>> + if (n->is_traversed)
>> + n->is_traversed = false;
>
> Remove the conditional, just set is_traversed to false.
>
Agree.
>> + }
>> +
>> + if (found) {
>> + struct icc_path *path = path_allocate(dst, depth);
>
> Is the path supposed to include the source? For instance, if the dst
> were a neighbor, depth would be one, so only dst would be in the path.
> It seems like it might be worthwhile to have the source in there too.
Agree that it would be logical to include the complete path. Will fix
the depth.
>> +
>> + if (IS_ERR(path))
>> + return path;
>> +
>> + /* initialize the path */
>> + for (i = 0; i < path->num_nodes; i++) {
>> + node = path->reqs[i].node;
>> + path->reqs[i].dev = dev;
>> + node->provider->users++;
>
> Should this loop live inside path_allocate? I'm unsure, but maybe at
> least path->reqs[i].dev = dev, since it feels like standard
> initialization of the path.
>
Ok, will move it and also change the function name from path_allocate()
to path_init().
>> +
>> +/**
>> + * icc_put() - release the reference to the icc_path
>> + * @path: interconnect path
>> + *
>> + * Use this function to release the constraints on a path when the path is
>> + * no longer needed. The constraints will be re-aggregated.
>> + */
>> +void icc_put(struct icc_path *path)
>> +{
>> + struct icc_node *node;
>> + size_t i;
>> + int ret;
>> +
>> + if (!path || WARN_ON_ONCE(IS_ERR(path)))
>
> Why only once?
>
Will change to WARN_ON.
[..]
>> +void icc_node_remove(int id)
>> +{
>> + struct icc_node *node;
>> +
>> + node = node_find(id);
>> + if (node) {
>> + mutex_lock(&icc_lock);
>> + idr_remove(&icc_idr, node->id);
>
> Should we throw a warning if there are any paths that go through this
> node (ie req_list is non-empty)?
>
Sounds good, will do it.
>> + mutex_unlock(&icc_lock);
>> + }
>> +
>> + kfree(node);
>> +}
>> +EXPORT_SYMBOL_GPL(icc_node_remove);
[..]
>> +/**
>> + * icc_link_remove() - remove a link between two nodes
>> + * @src: pointer to source node
>> + * @dst: pointer to destination node
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int icc_link_remove(struct icc_node *src, struct icc_node *dst)
>> +{
>> + struct icc_node **new;
>> + int ret = 0;
>> + int i, j;
>> +
>> + if (IS_ERR_OR_NULL(src))
>> + return PTR_ERR(src);
>> +
>> + if (IS_ERR_OR_NULL(dst))
>> + return PTR_ERR(dst);
>
> I wonder if we should return a fixed error in these cases like
> -EINVAL, rather than handing through whatever crazy value is in
> src/dst.
Ok, agree.
>> +
>> + mutex_lock(&icc_lock);
>> +
>> + new = krealloc(src->links,
>> + (src->num_links - 1) * sizeof(*src->links),
>> + GFP_KERNEL);
>> + if (!new) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + for (i = 0, j = 0; j < src->num_links; j++) {
>> + if (src->links[j] == dst)
>> + continue;
>> +
>> + new[i++] = src->links[j];
>> + }
>> +
>> + src->links = new;
>> + src->num_links--;
>
> My understanding is that once you call realloc and it succeeds, you
> must assume your old memory is gone and your new memory is only as big
> as the new size you request. So you shouldn't call krealloc until
> you've fixed the array up. Is the order of the links array important?
> If not, you could just take the element at the end and stick it in the
> slot that's being deleted. Then decrease the size and do your realloc.
Sorry, this was obviously untested. Your suggestions is good.
[..]
>> +
>> +/**
>> + * icc_provider_del() - delete previously added interconnect provider
>> + * @icc_provider: the interconnect provider that will be removed from topology
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int icc_provider_del(struct icc_provider *provider)
>> +{
>> + mutex_lock(&icc_lock);
>> + if (provider->users) {
>> + pr_warn("interconnect provider still has %d users\n",
>> + provider->users);
>> + mutex_unlock(&icc_lock);
>> + return -EBUSY;
>> + }
>> +
>> + if (!list_empty_careful(&provider->nodes)) {
>> + pr_warn("interconnect provider still has nodes\n");
>> + mutex_unlock(&icc_lock);
>> + return -EEXIST;
>
> How come you're returning different error codes for these two cases?
> The error in both cases is effectively "you failed to clean up after
> yourself", so maybe EBUSY makes sense for both of them. The pr_warn
> helps to differentiate between the two for debugging.
Ok.
>> + }
>> +
>> + list_del(&provider->provider_list);
>> + mutex_unlock(&icc_lock);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_provider_del);
>> +
[..]
>> +struct icc_node {
>> + int id;
>> + const char *name;
>> + struct icc_node **links;
>> + size_t num_links;
>> +
>> + struct icc_provider *provider;
>> + struct list_head node_list;
>> + struct list_head orphan_list;
>
> Is this used?
Ah, I thought I had already removed it!
>> + struct list_head search_list;
>> + struct icc_node *reverse;
>> + bool is_traversed;
>> + struct hlist_head req_list;
>> + u32 avg_bw;
>> + u32 peak_bw;
>> + void *data;
>> +};
>> +
[..]
>> +static inline int icc_set(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>> +{
>> + return 0;
>
> I was originally going to suggest that this should return a failure.
> Then I talked myself out of it, saying that if the interconnect
> framework is not compiled in, then clients should assume all their bus
> needs are already met. I guess this is the correct assumption?
Yes, exactly!
Thanks,
Georgi
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
