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] [day] [month] [year] [list]
Date: Sat, 29 Jun 2024 22:14:50 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Viresh Kumar <viresh.kumar@...aro.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 Wed, Jun 26, 2024 at 12:34 PM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> 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.
OK, I will remove this field.

>
> > > > +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.
A package here is a group of CPUs sharing the same control register,
not a group sharing the same policy. And yes, it is better to move the
lock to do_service_request().


>
> > > > +     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.
OK, I will do in the next version.

>
> > > > +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.
OK, I will accept your suggestion.

>
> > > > +     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.
Emm, the initialization print isn't very noisy, I will keep this line.
But all other issues will be addressed.

Huacai
>
> --
> viresh
>