Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
From: Lukasz Luba
Date: Mon Jun 08 2020 - 08:59:32 EST
On 6/8/20 1:51 PM, Dan Carpenter wrote:
On Mon, Jun 08, 2020 at 01:34:37PM +0100, Lukasz Luba wrote:
Hi Dan,
Thank you for your analyzes.
On 6/8/20 12:51 PM, Dan Carpenter wrote:
Hi Lukasz,
I love your patch! Perhaps something to improve:
url: https://github.com/0day-ci/linux/commits/Lukasz-Luba/Add-support-for-devices-in-the-Energy-Model/20200527-180614
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-m021-20200605 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@xxxxxxxxx>
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
smatch warnings:
kernel/power/energy_model.c:316 em_dev_register_perf_domain() error: we previously assumed 'dev->em_pd' could be null (see line 277)
# https://github.com/0day-ci/linux/commit/110d050cb7ba1c96e63ada498979d1fd99529be2
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 110d050cb7ba1c96e63ada498979d1fd99529be2
vim +316 kernel/power/energy_model.c
0e294e607adaf3 Lukasz Luba 2020-05-27 262 int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
110d050cb7ba1c Lukasz Luba 2020-05-27 263 struct em_data_callback *cb, cpumask_t *cpus)
27871f7a8a341e Quentin Perret 2018-12-03 264 {
27871f7a8a341e Quentin Perret 2018-12-03 265 unsigned long cap, prev_cap = 0;
110d050cb7ba1c Lukasz Luba 2020-05-27 266 int cpu, ret;
27871f7a8a341e Quentin Perret 2018-12-03 267
110d050cb7ba1c Lukasz Luba 2020-05-27 268 if (!dev || !nr_states || !cb)
27871f7a8a341e Quentin Perret 2018-12-03 269 return -EINVAL;
27871f7a8a341e Quentin Perret 2018-12-03 270
27871f7a8a341e Quentin Perret 2018-12-03 271 /*
27871f7a8a341e Quentin Perret 2018-12-03 272 * Use a mutex to serialize the registration of performance domains and
27871f7a8a341e Quentin Perret 2018-12-03 273 * let the driver-defined callback functions sleep.
27871f7a8a341e Quentin Perret 2018-12-03 274 */
27871f7a8a341e Quentin Perret 2018-12-03 275 mutex_lock(&em_pd_mutex);
27871f7a8a341e Quentin Perret 2018-12-03 276
110d050cb7ba1c Lukasz Luba 2020-05-27 @277 if (dev->em_pd) {
^^^^^^^^^^
Check for NULL.
27871f7a8a341e Quentin Perret 2018-12-03 278 ret = -EEXIST;
27871f7a8a341e Quentin Perret 2018-12-03 279 goto unlock;
27871f7a8a341e Quentin Perret 2018-12-03 280 }
27871f7a8a341e Quentin Perret 2018-12-03 281
110d050cb7ba1c Lukasz Luba 2020-05-27 282 if (_is_cpu_device(dev)) {
110d050cb7ba1c Lukasz Luba 2020-05-27 283 if (!cpus) {
110d050cb7ba1c Lukasz Luba 2020-05-27 284 dev_err(dev, "EM: invalid CPU mask\n");
110d050cb7ba1c Lukasz Luba 2020-05-27 285 ret = -EINVAL;
110d050cb7ba1c Lukasz Luba 2020-05-27 286 goto unlock;
110d050cb7ba1c Lukasz Luba 2020-05-27 287 }
110d050cb7ba1c Lukasz Luba 2020-05-27 288
110d050cb7ba1c Lukasz Luba 2020-05-27 289 for_each_cpu(cpu, cpus) {
110d050cb7ba1c Lukasz Luba 2020-05-27 290 if (em_cpu_get(cpu)) {
110d050cb7ba1c Lukasz Luba 2020-05-27 291 dev_err(dev, "EM: exists for CPU%d\n", cpu);
110d050cb7ba1c Lukasz Luba 2020-05-27 292 ret = -EEXIST;
110d050cb7ba1c Lukasz Luba 2020-05-27 293 goto unlock;
110d050cb7ba1c Lukasz Luba 2020-05-27 294 }
27871f7a8a341e Quentin Perret 2018-12-03 295 /*
110d050cb7ba1c Lukasz Luba 2020-05-27 296 * All CPUs of a domain must have the same
110d050cb7ba1c Lukasz Luba 2020-05-27 297 * micro-architecture since they all share the same
110d050cb7ba1c Lukasz Luba 2020-05-27 298 * table.
27871f7a8a341e Quentin Perret 2018-12-03 299 */
8ec59c0f5f4966 Vincent Guittot 2019-06-17 300 cap = arch_scale_cpu_capacity(cpu);
27871f7a8a341e Quentin Perret 2018-12-03 301 if (prev_cap && prev_cap != cap) {
110d050cb7ba1c Lukasz Luba 2020-05-27 302 dev_err(dev, "EM: CPUs of %*pbl must have the same capacity\n",
110d050cb7ba1c Lukasz Luba 2020-05-27 303 cpumask_pr_args(cpus));
110d050cb7ba1c Lukasz Luba 2020-05-27 304
27871f7a8a341e Quentin Perret 2018-12-03 305 ret = -EINVAL;
27871f7a8a341e Quentin Perret 2018-12-03 306 goto unlock;
27871f7a8a341e Quentin Perret 2018-12-03 307 }
27871f7a8a341e Quentin Perret 2018-12-03 308 prev_cap = cap;
27871f7a8a341e Quentin Perret 2018-12-03 309 }
110d050cb7ba1c Lukasz Luba 2020-05-27 310 }
27871f7a8a341e Quentin Perret 2018-12-03 311
110d050cb7ba1c Lukasz Luba 2020-05-27 312 ret = em_create_pd(dev, nr_states, cb, cpus);
If it's a _is_cpu_device() then it iterates through a bunch of devices
and sets up cpu_dev->em_pd for each. Presumably one of the devices is
"dev" or this would crash pretty early on in testing?
Yes, all of the devices taken from 'cpus' mask will get the em_pd set
including the suspected @dev.
To calm down this static analyzer I can remove the 'else'
in line 204 to make 'dev->em_pd = pd' set always.
199 if (_is_cpu_device(dev))
200 for_each_cpu(cpu, cpus) {
201 cpu_dev = get_cpu_device(cpu);
202 cpu_dev->em_pd = pd;
203 }
204 else
205 dev->em_pd = pd;
Do you think it's a good solution and will work for this tool?
It's not about the tool... Ignore the tool when it's wrong. But I do
think the code is slightly more clear without the else statement.
Arguments could be made either way. Removing the else statement means
we set dev->em_pd twice... But I *personally* lean vaguely towards
removing the else statement. :P
Thanks, I will remove the else statement and add your 'Reported-by'
Regards,
Lukasz
That would make the warning go away as well.
regards,
dan carpenter