* re: PM / OPP: Add OPP sharing information to OPP library
@ 2015-08-10 16:40 Dan Carpenter
2015-08-11 8:22 ` Viresh Kumar
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2015-08-10 16:40 UTC (permalink / raw)
To: viresh.kumar; +Cc: linux-pm
Hello Viresh Kumar,
The patch 064416586190: "PM / OPP: Add OPP sharing information to OPP
library" from Jul 29, 2015, leads to the following static checker
warning:
drivers/base/power/opp.c:1341 _of_init_opp_table_v2()
error: 'dev_opp' dereferencing possible ERR_PTR()
drivers/base/power/opp.c
1328 }
1329
1330 /* There should be one of more OPP defined */
1331 if (WARN_ON(!count))
1332 goto put_opp_np;
Should we set "ret" here?
1333
1334 if (!ret) {
1335 if (!dev_opp) {
^^^^^^^
No need to test this, we tested at the start of the function.
1336 dev_opp = _find_device_opp(dev);
1337 if (WARN_ON(!dev_opp))
This should be checking for IS_ERR(). We probably want to set "ret =
PTR_ERR(dev_opp) as well.
1338 goto put_opp_np;
1339 }
1340
1341 dev_opp->np = opp_np;
1342 dev_opp->shared_opp = of_property_read_bool(opp_np,
1343 "opp-shared");
1344 } else {
1345 of_free_opp_table(dev);
1346 }
1347
1348 put_opp_np:
1349 of_node_put(opp_np);
1350
1351 return ret;
1352 }
Checking for "!ret" on line 1334 is "success handling" style code.
Success handling leads to multiple indent levels and is more twisted and
confusing. I generally prefer to keep my error handling paths separate
from the success path although in this case we call of_node_put() on
both paths, so maybe a shared exit path makes sense.
To me boils down to a question of, "How much is shared and how much is
different. Also should we call of_free_opp_table() if _find_device_opp()
fails?" If not then I would use a common exit path, otherwise I would
split them apart.
Shared:
if (ret) {
of_free_opp_table(dev);
goto put_opp_npl;
}
dev_opp = _find_device_opp(dev);
if (WARN_ON(IS_ERR(dev_opp))) {
ret = PTR_ERR(dev_opp);
goto put_opp_np;
}
dev_opp->np = opp_np;
dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared");
put_opp_np:
of_node_put(opp_np);
return ret;
Split apart:
if (ret)
goto free_opp_table;
dev_opp = _find_device_opp(dev);
if (WARN_ON(IS_ERR(dev_opp))) {
ret = PTR_ERR(dev_opp);
goto free_opp_table;
}
dev_opp->np = opp_np;
dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared");
of_node_put(opp_np);
return 0;
free_opp_table:
of_free_opp_table(dev);
put_opp_np:
of_node_put(opp_np);
return ret;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: PM / OPP: Add OPP sharing information to OPP library
2015-08-10 16:40 PM / OPP: Add OPP sharing information to OPP library Dan Carpenter
@ 2015-08-11 8:22 ` Viresh Kumar
0 siblings, 0 replies; 2+ messages in thread
From: Viresh Kumar @ 2015-08-11 8:22 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-pm
Hi Dan,
Are the static checker scripts available (and easy to use) for others
as well? Maybe I can use them on code, before sending patches.
On 10-08-15, 19:40, Dan Carpenter wrote:
> Hello Viresh Kumar,
>
> The patch 064416586190: "PM / OPP: Add OPP sharing information to OPP
> library" from Jul 29, 2015, leads to the following static checker
> warning:
>
> drivers/base/power/opp.c:1341 _of_init_opp_table_v2()
> error: 'dev_opp' dereferencing possible ERR_PTR()
>
> drivers/base/power/opp.c
> 1328 }
> 1329
> 1330 /* There should be one of more OPP defined */
> 1331 if (WARN_ON(!count))
> 1332 goto put_opp_np;
>
> Should we set "ret" here?
Yes, one of my new patches is doing this.
> 1333
> 1334 if (!ret) {
> 1335 if (!dev_opp) {
> ^^^^^^^
> No need to test this, we tested at the start of the function.
Yes, I have killed this extra indentation level as well.
> 1336 dev_opp = _find_device_opp(dev);
> 1337 if (WARN_ON(!dev_opp))
>
> This should be checking for IS_ERR(). We probably want to set "ret =
> PTR_ERR(dev_opp) as well.
Okay, that's new. Will fix.
> 1338 goto put_opp_np;
> 1339 }
> 1340
> 1341 dev_opp->np = opp_np;
> 1342 dev_opp->shared_opp = of_property_read_bool(opp_np,
> 1343 "opp-shared");
> 1344 } else {
> 1345 of_free_opp_table(dev);
> 1346 }
> 1347
> 1348 put_opp_np:
> 1349 of_node_put(opp_np);
> 1350
> 1351 return ret;
> 1352 }
>
> Checking for "!ret" on line 1334 is "success handling" style code.
> Success handling leads to multiple indent levels and is more twisted and
> confusing. I generally prefer to keep my error handling paths separate
> from the success path although in this case we call of_node_put() on
> both paths, so maybe a shared exit path makes sense.
>
> To me boils down to a question of, "How much is shared and how much is
> different. Also should we call of_free_opp_table() if _find_device_opp()
> fails?" If not then I would use a common exit path, otherwise I would
> split them apart.
>
> Shared:
>
> if (ret) {
> of_free_opp_table(dev);
> goto put_opp_npl;
> }
>
> dev_opp = _find_device_opp(dev);
> if (WARN_ON(IS_ERR(dev_opp))) {
> ret = PTR_ERR(dev_opp);
> goto put_opp_np;
> }
>
> dev_opp->np = opp_np;
> dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared");
>
> put_opp_np:
> of_node_put(opp_np);
> return ret;
>
> Split apart:
>
> if (ret)
> goto free_opp_table;
>
> dev_opp = _find_device_opp(dev);
> if (WARN_ON(IS_ERR(dev_opp))) {
> ret = PTR_ERR(dev_opp);
> goto free_opp_table;
> }
>
> dev_opp->np = opp_np;
> dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared");
>
> of_node_put(opp_np);
> return 0;
>
> free_opp_table:
> of_free_opp_table(dev);
> put_opp_np:
> of_node_put(opp_np);
> return ret;
Will check that as well.
--
viresh
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-08-11 8:22 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-10 16:40 PM / OPP: Add OPP sharing information to OPP library Dan Carpenter
2015-08-11 8:22 ` Viresh Kumar
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).