linux-kernel - Re: [PATCH 2/2] cpufreq: Add Loongson-3 CPUFreq driver 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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Wed, 26 Jun 2024 10:04:41 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: Huacai Chen <chenhuacai@...ngson.cn>,
	"Rafael J . Wysocki" <rafael@...nel.org>, loongarch@...ts.linux.dev,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
	Xuerui Wang <kernel@...0n.name>,
	Jiaxun Yang <jiaxun.yang@...goat.com>,
	Binbin Zhou <zhoubinbin@...ngson.cn>
Subject: Re: [PATCH 2/2] cpufreq: Add Loongson-3 CPUFreq driver support

On 26-06-24, 11:51, Huacai Chen wrote:
> On Tue, Jun 25, 2024 at 3:56 PM Viresh Kumar <viresh.kumar@...aro.org> wrote:
> > On 12-06-24, 14:42, Huacai Chen wrote:
> > > +struct loongson3_freq_data {
> > > +     unsigned int cur_cpu_freq;
> >
> > You never use it. Remove it.
> Emm, it is used in loongson3_cpufreq_get().

Yeah, you are just filling it there and reading immediately after
that, which can be done directly too. But you don't use it anywhere
else.

> > > +static int loongson3_cpufreq_target(struct cpufreq_policy *policy, unsigned int index)
> > > +{
> > > +     unsigned int cpu = policy->cpu;
> > > +     unsigned int package = cpu_data[cpu].package;
> > > +
> > > +     if (!cpu_online(cpu))
> > > +             return -ENODEV;
> > > +
> > > +     /* setting the cpu frequency */
> > > +     mutex_lock(&cpufreq_mutex[package]);
> >
> > No locking required here. Core doesn't call them in parallel.

s/Core/CPUFreq core/

> I'm a bit confused, I think different cores may call .target() in
> parallel.

Not for same policy, but for different yes.

> Cores in the same package share the same
> LOONGARCH_IOCSR_SMCMBX register, so I think the lock is required.

If that is the access you are protecting, then you better move the
lock to do_service_request() instead, which gets called from other
places too.

What exactly is a package here though ? A group of CPUs doing DVFS
together ? Governed by the same policy structure ? In that case, you
don't need a lock as the cpufreq core guarantees to not call multiple
target_index() routines in parallel for the same policy.

> > > +     msg.id          = cpu;
> > > +     msg.cmd         = CMD_GET_FREQ_LEVEL_NUM;
> > > +     msg.extra       = 0;
> > > +     msg.complete    = 0;
> > > +     ret = do_service_request(&msg);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +     max_level = msg.val;
> > > +
> >
> >
> > > +     msg.id          = cpu;
> > > +     msg.cmd         = CMD_GET_FREQ_BOOST_LEVEL;
> > > +     msg.extra       = 0;
> > > +     msg.complete    = 0;
> > > +     ret = do_service_request(&msg);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +     boost_level = msg.val;
> >
> > This stuff is repeated a lot, maybe create a generic function for this
> > ?
> Do you means move the msg filling into do_service_request()?

Yeah, the filling of the msg structure, the call to
do_service_request() and returning msg.val.

> > > +static int __init cpufreq_init(void)
> > > +{
> > > +     int i, ret;
> > > +
> > > +     ret = platform_driver_register(&loongson3_platform_driver);
> > > +     if (ret)
> > > +             return ret;
> >
> > What is the use of this platform driver ? I thought the whole purpose
> > of the platform device/driver in your case was to probe this driver.
> > In that case cpufreq_init() should only be doing above and not the
> > below part. The rest should be handled in the probe() function of the
> > driver.
> This driver file is now a very basic version, in future it will be a
> little like intel_pstate that has more than one cpufreq_drivers
> (active/passive, hwp/nohwp, etc.), so it will register different
> cpufreq_drivers depends on the result of configure_cpufreq_info().

At this moment we can only review the current version on its merit.
For the current version, the way things are done is simply wrong. We
can review later once you have more things to add to this. So simplify
it to the best of our understanding for now and make as many changes
later as you need.

> > > +     ret = cpufreq_register_driver(&loongson3_cpufreq_driver);
> > > +     if (ret)
> > > +             goto err;
> > > +
> > > +     pr_info("cpufreq: Loongson-3 CPU frequency driver.\n");
> >
> > Make this pr_debug if you want.. There is not much use of this for the
> > user.
> Emm, I just want to see a line in dmesg.

Yeah, a debug message is what you need then. We don't want to print
too much unnecessarily on the console, unless there is an error.

-- 
viresh