Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Georgi Djakov]

Hi Evan,

On 11/19/19 00:42, Evan Green wrote:
> Hi Sibi,
> 
> On Mon, Nov 18, 2019 at 7:45 AM Sibi Sankar <sibis@xxxxxxxxxxxxxx> wrote:
>>
>> On some Qualcomm SoCs, Operating State Manager (OSM) controls the
>> resources of scaling L3 caches. Add a driver to handle bandwidth
>> requests to OSM L3 from CPU/GPU.
>>
>> Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>
>> ---
>>  drivers/interconnect/qcom/Kconfig  |   7 +
>>  drivers/interconnect/qcom/Makefile |   2 +
>>  drivers/interconnect/qcom/osm-l3.c | 284 +++++++++++++++++++++++++++++
>>  3 files changed, 293 insertions(+)
>>  create mode 100644 drivers/interconnect/qcom/osm-l3.c
>>
[..]
>> +static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>> +                             u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
>> +{
>> +       *agg_avg += avg_bw;
>> +       *agg_peak = max_t(u32, *agg_peak, peak_bw);
>> +
>> +       return 0;
>> +}
> 
> Georgi, I wonder if it's a good idea to make a small collection of
> "std" aggregate functions in the interconnect core that a driver can
> just point to if it's doing something super standard like this (ie
> driver->aggregate = icc_std_aggregate;). This is probably fine as-is
> for now, but if we see a lot more copy/pastes of this function we
> should think about it.

Sure, thanks for the suggestion! Will submit a patch for this.

> 
>> +
>> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>> +{
[..]
>> +
>> +static int qcom_osm_l3_remove(struct platform_device *pdev)
>> +{
>> +       struct qcom_osm_l3_icc_provider *qp = platform_get_drvdata(pdev);
>> +       struct icc_provider *provider = &qp->provider;
>> +       struct icc_node *n;
>> +
>> +       list_for_each_entry(n, &provider->nodes, node_list) {
> 
> There was a comment on one of the other threads that we've been
> copy/pasting this snippet around and it's wrong because it doesn't use
> the _safe variant of the macro. So we end up destroying the list we're
> iterating over.
> 
> Georgi, did you have a plan to refactor this, or should we just change
> this to be the _safe version?

I have fixes that convert this to the _safe variant (for stable) and also
factor this out into icc_nodes_remove() function. Will post them.

Thanks,
Georgi

> 
>> +               icc_node_del(n);
>> +               icc_node_destroy(n->id);
>> +       }
>> +
>> +       return icc_provider_del(provider);
>> +}