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
next prev parent reply other threads:[~2020-06-08 12:52 UTC|newest]
Thread overview: 32+ 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 ` [PATCH v8 1/8] PM / EM: change naming convention from 'capacity' to 'performance' 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 ` [PATCH v8 3/8] PM / EM: update callback structure and add device pointer Lukasz Luba
2020-05-29 17:43 ` Daniel Lezcano
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-06-01 21:44 ` Daniel Lezcano
2020-06-02 11:31 ` Lukasz Luba
2020-06-03 15:13 ` Rafael J. Wysocki
2020-06-03 15:25 ` Lukasz Luba
2020-06-03 15:40 ` Rafael J. Wysocki
2020-06-03 16:12 ` Lukasz Luba
2020-06-03 16:22 ` Rafael J. Wysocki
2020-06-03 16:45 ` Lukasz Luba
2020-06-08 11:51 ` Dan Carpenter
2020-06-08 12:34 ` Lukasz Luba
2020-06-08 12:51 ` Dan Carpenter [this message]
2020-06-08 12:59 ` Lukasz Luba
2020-06-08 13:25 ` Dan Carpenter
2020-06-08 13:49 ` Lukasz Luba
2020-06-10 10:12 ` [RESEND][PATCH " Lukasz Luba
2020-06-24 15:21 ` Rafael J. Wysocki
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 ` [PATCH v8 6/8] PM / EM: change name of em_pd_energy to em_cpu_energy Lukasz Luba
2020-05-27 9:58 ` [PATCH v8 7/8] Documentation: power: update Energy Model description 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-29 15:00 ` [PATCH v8 0/8] Add support for devices in the Energy Model Lukasz Luba
2020-05-29 16:18 ` Rafael J. Wysocki
2020-05-29 17:05 ` Lukasz Luba
2020-06-17 9:17 ` Lukasz Luba
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).