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 [off-list ref] wrote:

quoted

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.

[..]

quoted

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

quoted

+
+       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!

[..]

quoted

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

quoted

+       }
+
+       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.

quoted

+
+               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().

quoted

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

[..]

quoted

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

quoted

+               mutex_unlock(&icc_lock);
+       }
+
+       kfree(node);
+}
+EXPORT_SYMBOL_GPL(icc_node_remove);

[..]

quoted

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

quoted

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

[..]

quoted

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

quoted

+       }
+
+       list_del(&provider->provider_list);
+       mutex_unlock(&icc_lock);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(icc_provider_del);
+

[..]

quoted

+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!

quoted

+       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;
+};
+

[..]

quoted

+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