Re: [RFC v0 1/2] interconnect: Add generic interconnect controller API

[Georgi Djakov <georgi.djakov@linaro.org>]

On 03/03/2017 04:07 AM, Saravana Kannan wrote:
> On 03/01/2017 10:22 AM, Georgi Djakov wrote:
>> This patch introduce a new API to get the requirement and configure the
>> interconnect buses across the entire chipset to fit with the current
>> demand.
>>

[..]

>> +int interconnect_set(struct interconnect_path *path, u32 bandwidth)
>> +{
>> +    struct interconnect_node *node;
>> +
>> +    list_for_each_entry(node, &path->node_list, search_list) {
>> +        if (node->icp->ops->set)
>> +            node->icp->ops->set(node, bandwidth);
>
> This ops needs to take a "source" and "dest" input and you'll need to
> pass in the prev/next nodes of each "node" in this list. This is needed
> so that each interconnect know the local source/destination and can make
> the aggregation decisions correctly based on the internal implementation
> of the interconnect. For the first and last nodes in the list, the
> source and destination nodes can be NULL, respectively.

I agree. Updated.

>
>> +    }
>> +
>> +    return 0;
>> +}
>> +

[..]

>> +void interconnect_put(struct interconnect_path *path)
>> +{
>> +    struct interconnect_node *node;
>> +    struct icn_qos *req;
>> +    struct hlist_node *tmp;
>> +
>> +    if (IS_ERR(path))
>> +        return;
>> +
>> +    list_for_each_entry(node, &path->node_list, search_list) {
>> +        hlist_for_each_entry_safe(req, tmp, &node->qos_list, node) {
>> +            if (req->path == path) {
>> +                hlist_del(&req->node);
>> +                kfree(req);
>> +            }
>
> Should we go through and remove any bandwidth votes that were made on
> this path before we free it?
>

Yes, thanks! We should remove the constraints from the path, then
update the nodes and after that free the memory.

>> +        }
>> +    }
>> +
>> +    kfree(path);
>> +}
>> +EXPORT_SYMBOL_GPL(interconnect_put);
>> +
>> +int interconnect_add_provider(struct icp *icp)
>> +{
>> +    struct interconnect_node *node;
>> +
>> +    WARN(!icp->ops->xlate, "%s: .xlate is not implemented\n", __func__);
>> +    WARN(!icp->ops->set, "%s: .set is not implemented\n", __func__);
>> +
>> +    mutex_lock(&interconnect_provider_list_mutex);
>> +    list_add(&icp->icp_list, &interconnect_provider_list);
>> +    mutex_unlock(&interconnect_provider_list_mutex);
>> +
>> +    list_for_each_entry(node, &icp->nodes, icn_list) {
>> +        INIT_HLIST_HEAD(&node->qos_list);
>> +    }
>> +
>> +    dev_info(icp->dev, "added interconnect provider %s\n", icp->name);
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(interconnect_add_provider);
>> +
>> +int interconnect_del_provider(struct icp *icp)
>> +{
>> +    mutex_lock(&interconnect_provider_list_mutex);
>> +    of_node_put(icp->of_node);
>> +    list_del(&icp->icp_list);
>
> If there's a path with an active vote, we should probably return a
> -EBUSY to prevent deleting a provider that's actively used?
>

Thanks, sounds good. Will do.

BR,
Georgi