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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <534d80fb-249d-ce18-86c0-1a1bc4f98c03@linaro.org>
Date:   Thu, 21 Nov 2019 14:58:27 +0200
From:   Georgi Djakov <georgi.djakov@...aro.org>
To:     Evan Green <evgreen@...omium.org>,
        Sibi Sankar <sibis@...eaurora.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Andy Gross <agross@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Mark Rutland <mark.rutland@....com>,
        David Dai <daidavid1@...eaurora.org>,
        Saravana Kannan <saravanak@...gle.com>,
        Viresh Kumar <viresh.kumar@...aro.org>
Subject: Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect
 provider support

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@...eaurora.org> 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@...eaurora.org>
>> ---
>>  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);
>> +}