linux-kernel - Re: [PATCH 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 20 Nov 2018 16:59:30 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Matthias Kaehlcke <mka@...omium.org>,
        Taniya Das <tdas@...eaurora.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Rajendra Nayak <rnayak@...eaurora.org>,
        devicetree@...r.kernel.org, robh@...nel.org,
        skannan@...eaurora.org, linux-arm-msm@...r.kernel.org,
        amit.kucheria@...aro.org, evgreen@...gle.com
Subject: Re: [PATCH 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

Quoting Matthias Kaehlcke (2018-11-15 16:23:37)
> On Sun, Nov 11, 2018 at 06:12:29PM +0530, Taniya Das wrote:
> > On 11/4/2018 9:50 AM, Stephen Boyd wrote:
> > > Quoting Taniya Das (2018-11-02 20:06:00)
> > > > On 10/18/2018 5:02 AM, Stephen Boyd wrote:
> > > > > Quoting Taniya Das (2018-10-11 04:36:01)
> > > > > > --- a/drivers/cpufreq/Kconfig.arm
> > > > > > +++ b/drivers/cpufreq/Kconfig.arm
> > > > > > @@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
> > > > > > 
> > > > > >             If in doubt, say N.
> > > > > > 
> > > > > > +config ARM_QCOM_CPUFREQ_HW
> > > > > > +       bool "QCOM CPUFreq HW driver"
> > > > > 
> > > > > Is there any reason this can't be a module?
> > > > > 
> > > > 
> > > > We do not have any use cases where we need to support it as module.
> > > 
> > > Ok, so it could easily be tristate then? Why not allow it?
> > > 
> > 
> > I have checked other vendors CPUfreq drivers and those too support only
> > "bool".
> 
> That's not entirely correct. Most drivers in Kconfig are 'tristate'
> and about 50% of those in KConfig.arm are. I'd say make it 'tristate'
> unless there are good reasons not to do so.

Yes, please make tristate.

> 
> > > > > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > > > new file mode 100644
> > > > > > index 0000000..fe1c264
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > > > @@ -0,0 +1,354 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> > > > > > + */
> > > [...]
> > > > > > +
> > > > > > +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> > > > > 
> > > > > Is this going to change in the future?
> > > > > 
> > > > 
> > > > Yes, they could change and that was the reason to introduce the offsets.
> > > > This was discussed earlier too with Sudeep and was to add them.
> > > > 
> > > > > > +       [REG_ENABLE]            = 0x0,
> > > 
> > > This is only used once? Maybe it could be removed.
> > > 
> > > > > > +       [REG_LUT_TABLE]         = 0x110,
> > > 
> > > And this is only used during probe to figure out the supported
> > > frequencies. So we definitely don't need to store around the registers
> > > after probe in an array of iomem pointers. The only one that we need
> > > after probe is the one below.
> > > 
> > > > > > +       [REG_PERF_STATE]        = 0x920,
> > > > > > +};
> > > > > > +
> > 
> > As these address offsets could change, so I am of the opinion to leave them
> > as it is.
> 
> As of now there is only one set of offsets. Let's just keep the code
> simple while this is the case and address different offsets when it is
> actually needed, as suggested by Stephen and Sudeep.

Yes, please simplify by getting rid of this and not storing anything in
the struct that's only used during probe.

> 
> > > 
> > > With fast switching we can avoid incurring any extra instructions, so
> > > please make another iomem pointer in the cpufreq_qcom struct just for
> > > writing the index or if possible, just pass the iomem pointer that
> > > points to the REG_PERF_STATE as the policy->driver_data variable here.
> > > Then we have the address in hand without any extra load. If my
> > > understanding is correct, we don't need to keep around anything besides
> > > this register address anyway so we should be able to just load it and
> > > write it immediately.
> > > 
> > 
> > The c->reg_bases[] is just an index to the updated bases addresses. I am not
> > clear as to why it would incur an extra instruction.
> > 
> > The below code would already take care of it.
> > 
> > +     for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> > +             c->reg_bases[i] = base + offsets[i];
> > +
> 
> From a performance point of view using a direct iomem pointer
> seems like a micro-optimization that probably doesn't have a
> measurable impact. However I think the code shouldn't be more complex
> than necessary, and at this point the indirection isn't needed.
> 

Yes it's a micro-optimization for sure, in the task switching path so it
may actually be useful. Either way, I think we can greatly simplify by
just having the iomem pointer be the only pointer that is stored in the
policy driver_data.