From: Dan Carpenter <dan.carpenter@oracle.com>
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: kbuild@lists.01.org, linux-kernel@vger.kernel.org,
linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
dri-devel@lists.freedesktop.org, linux-omap@vger.kernel.org,
linux-mediatek@lists.infradead.org,
linux-arm-msm@vger.kernel.org, linux-imx@nxp.com, lkp@intel.com,
Dan Carpenter <error27@gmail.com>,
kbuild-all@lists.01.org, Dietmar.Eggemann@arm.com,
cw00.choi@samsung.com
Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
Date: Mon, 8 Jun 2020 15:51:27 +0300 [thread overview]
Message-ID: <20200608125127.GN22511@kadam> (raw)
In-Reply-To: <b347fb60-46d3-e59c-59fa-a2b10932fc49@arm.com>
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@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@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/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
That would make the warning go away as well.
regards,
dan carpenter
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: kbuild-all@lists.01.org, lkp@intel.com, linux-pm@vger.kernel.org,
linux-arm-msm@vger.kernel.org, kbuild@lists.01.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
cw00.choi@samsung.com, linux-mediatek@lists.infradead.org,
linux-imx@nxp.com, linux-omap@vger.kernel.org,
Dietmar.Eggemann@arm.com, linux-arm-kernel@lists.infradead.org,
Dan Carpenter <error27@gmail.com>
Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
Date: Mon, 8 Jun 2020 15:51:27 +0300 [thread overview]
Message-ID: <20200608125127.GN22511@kadam> (raw)
In-Reply-To: <b347fb60-46d3-e59c-59fa-a2b10932fc49@arm.com>
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@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@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/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
That would make the warning go away as well.
regards,
dan carpenter
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: kbuild-all@lists.01.org, lkp@intel.com, linux-pm@vger.kernel.org,
linux-arm-msm@vger.kernel.org, kbuild@lists.01.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
cw00.choi@samsung.com, linux-mediatek@lists.infradead.org,
linux-imx@nxp.com, linux-omap@vger.kernel.org,
Dietmar.Eggemann@arm.com, linux-arm-kernel@lists.infradead.org,
Dan Carpenter <error27@gmail.com>
Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
Date: Mon, 8 Jun 2020 15:51:27 +0300 [thread overview]
Message-ID: <20200608125127.GN22511@kadam> (raw)
In-Reply-To: <b347fb60-46d3-e59c-59fa-a2b10932fc49@arm.com>
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@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@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/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
That would make the warning go away as well.
regards,
dan carpenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: kbuild-all@lists.01.org, lkp@intel.com, linux-pm@vger.kernel.org,
linux-arm-msm@vger.kernel.org, kbuild@lists.01.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
cw00.choi@samsung.com, linux-mediatek@lists.infradead.org,
linux-imx@nxp.com, linux-omap@vger.kernel.org,
Dietmar.Eggemann@arm.com, linux-arm-kernel@lists.infradead.org,
Dan Carpenter <error27@gmail.com>
Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
Date: Mon, 8 Jun 2020 15:51:27 +0300 [thread overview]
Message-ID: <20200608125127.GN22511@kadam> (raw)
In-Reply-To: <b347fb60-46d3-e59c-59fa-a2b10932fc49@arm.com>
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@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@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/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
That would make the warning go away as well.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: kbuild@lists.01.org
Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
Date: Mon, 08 Jun 2020 15:51:27 +0300 [thread overview]
Message-ID: <20200608125127.GN22511@kadam> (raw)
In-Reply-To: <b347fb60-46d3-e59c-59fa-a2b10932fc49@arm.com>
[-- Attachment #1: Type: text/plain, Size: 6354 bytes --]
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@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@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/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
That would make the warning go away as well.
regards,
dan carpenter
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: kbuild-all@lists.01.org
Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
Date: Mon, 08 Jun 2020 15:51:27 +0300 [thread overview]
Message-ID: <20200608125127.GN22511@kadam> (raw)
In-Reply-To: <b347fb60-46d3-e59c-59fa-a2b10932fc49@arm.com>
[-- Attachment #1: Type: text/plain, Size: 6354 bytes --]
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@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@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/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
That would make the warning go away as well.
regards,
dan carpenter
next prev parent reply other threads:[~2020-06-08 12:51 UTC|newest]
Thread overview: 127+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-27 9:58 [PATCH v8 0/8] Add support for devices in the Energy Model Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 1/8] PM / EM: change naming convention from 'capacity' to 'performance' Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 2/8] PM / EM: introduce em_dev_register_perf_domain function Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 3/8] PM / EM: update callback structure and add device pointer Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-29 17:43 ` Daniel Lezcano
2020-05-29 17:43 ` Daniel Lezcano
2020-05-29 17:43 ` Daniel Lezcano
2020-05-29 17:43 ` Daniel Lezcano
2020-06-01 9:20 ` Lukasz Luba
2020-06-01 9:20 ` Lukasz Luba
2020-06-01 9:20 ` Lukasz Luba
2020-06-01 9:20 ` Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-06-01 21:44 ` Daniel Lezcano
2020-06-01 21:44 ` Daniel Lezcano
2020-06-01 21:44 ` Daniel Lezcano
2020-06-01 21:44 ` Daniel Lezcano
2020-06-02 11:31 ` Lukasz Luba
2020-06-02 11:31 ` Lukasz Luba
2020-06-02 11:31 ` Lukasz Luba
2020-06-02 11:31 ` Lukasz Luba
2020-06-03 15:13 ` Rafael J. Wysocki
2020-06-03 15:13 ` Rafael J. Wysocki
2020-06-03 15:13 ` Rafael J. Wysocki
2020-06-03 15:25 ` Lukasz Luba
2020-06-03 15:25 ` Lukasz Luba
2020-06-03 15:25 ` Lukasz Luba
2020-06-03 15:40 ` Rafael J. Wysocki
2020-06-03 15:40 ` Rafael J. Wysocki
2020-06-03 15:40 ` Rafael J. Wysocki
2020-06-03 16:12 ` Lukasz Luba
2020-06-03 16:12 ` Lukasz Luba
2020-06-03 16:12 ` Lukasz Luba
2020-06-03 16:22 ` Rafael J. Wysocki
2020-06-03 16:22 ` Rafael J. Wysocki
2020-06-03 16:22 ` Rafael J. Wysocki
2020-06-03 16:45 ` Lukasz Luba
2020-06-03 16:45 ` Lukasz Luba
2020-06-03 16:45 ` Lukasz Luba
2020-06-08 11:51 ` Dan Carpenter
2020-06-08 11:51 ` Dan Carpenter
2020-06-08 11:51 ` Dan Carpenter
2020-06-08 11:51 ` Dan Carpenter
2020-06-08 11:51 ` Dan Carpenter
2020-06-08 11:51 ` Dan Carpenter
2020-06-08 12:34 ` Lukasz Luba
2020-06-08 12:34 ` Lukasz Luba
2020-06-08 12:34 ` Lukasz Luba
2020-06-08 12:34 ` Lukasz Luba
2020-06-08 12:34 ` Lukasz Luba
2020-06-08 12:51 ` Dan Carpenter [this message]
2020-06-08 12:51 ` Dan Carpenter
2020-06-08 12:51 ` Dan Carpenter
2020-06-08 12:51 ` Dan Carpenter
2020-06-08 12:51 ` Dan Carpenter
2020-06-08 12:51 ` Dan Carpenter
2020-06-08 12:59 ` Lukasz Luba
2020-06-08 12:59 ` Lukasz Luba
2020-06-08 12:59 ` Lukasz Luba
2020-06-08 12:59 ` Lukasz Luba
2020-06-08 12:59 ` Lukasz Luba
2020-06-08 13:25 ` Dan Carpenter
2020-06-08 13:25 ` Dan Carpenter
2020-06-08 13:25 ` Dan Carpenter
2020-06-08 13:25 ` Dan Carpenter
2020-06-08 13:25 ` Dan Carpenter
2020-06-08 13:25 ` Dan Carpenter
2020-06-08 13:49 ` Lukasz Luba
2020-06-08 13:49 ` Lukasz Luba
2020-06-08 13:49 ` Lukasz Luba
2020-06-08 13:49 ` Lukasz Luba
2020-06-08 13:49 ` Lukasz Luba
2020-06-10 10:12 ` [RESEND][PATCH " Lukasz Luba
2020-06-10 10:12 ` Lukasz Luba
2020-06-10 10:12 ` Lukasz Luba
2020-06-10 10:12 ` Lukasz Luba
2020-06-24 15:21 ` Rafael J. Wysocki
2020-06-24 15:21 ` Rafael J. Wysocki
2020-06-24 15:21 ` Rafael J. Wysocki
2020-06-24 15:29 ` Lukasz Luba
2020-06-24 15:29 ` Lukasz Luba
2020-06-24 15:29 ` Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 5/8] PM / EM: remove em_register_perf_domain Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 6/8] PM / EM: change name of em_pd_energy to em_cpu_energy Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 7/8] Documentation: power: update Energy Model description Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 8/8] OPP: refactor dev_pm_opp_of_register_em() and update related drivers Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-27 9:58 ` Lukasz Luba
2020-05-29 15:00 ` [PATCH v8 0/8] Add support for devices in the Energy Model Lukasz Luba
2020-05-29 15:00 ` Lukasz Luba
2020-05-29 15:00 ` Lukasz Luba
2020-05-29 15:00 ` Lukasz Luba
2020-05-29 16:18 ` Rafael J. Wysocki
2020-05-29 16:18 ` Rafael J. Wysocki
2020-05-29 16:18 ` Rafael J. Wysocki
2020-05-29 17:05 ` Lukasz Luba
2020-05-29 17:05 ` Lukasz Luba
2020-05-29 17:05 ` Lukasz Luba
2020-06-17 9:17 ` Lukasz Luba
2020-06-17 9:17 ` Lukasz Luba
2020-06-17 9:17 ` Lukasz Luba
2020-06-05 22:56 [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in " kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200608125127.GN22511@kadam \
--to=dan.carpenter@oracle.com \
--cc=Dietmar.Eggemann@arm.com \
--cc=cw00.choi@samsung.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=error27@gmail.com \
--cc=kbuild-all@lists.01.org \
--cc=kbuild@lists.01.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-omap@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lkp@intel.com \
--cc=lukasz.luba@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.