10:59 p.m.
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-devi...
>> 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(a)intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter(a)oracle.com>
>>
>> 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/110d050cb7ba1c96e63ada498979d1fd9...
>> 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
>