| Message ID | YsWb0JLVFbXS+qGj@kili (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [1/2] powercap: arm_scmi: Fix signedness bug in probe | expand |
On Wed, Jul 06, 2022 at 05:27:28PM +0300, Dan Carpenter wrote: > The powercap_register_control_type() return error pointers. It never > returns NULL. > > Fixes: 31afdd34f2b9 ("powercap: arm_scmi: Add SCMI powercap based driver") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > This functions should really clean up after itself if scmi_register() > fails. I need to fix the static checker for that and then I'll come > back and fix it if no one else does. > Hi, thanks for the fix and the suggestion to clean up better (this part was indeed reworked in V4 and I think it's where I introduced the missing cleanup when scmi_register fails...) As said, the SCMI Powercap driver was NOT pulled for this cycle due to insufficent reviews so I'll pick your fixes and suggestions for the next version. May I ask which static checker you use ? Sparse/smatch and W=1 did not spot any of these issues (including other in the series) in my workflow ... Thanks, Cristian > drivers/powercap/arm_scmi_powercap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c > index ab96cf9a8604..2d505ec7ff81 100644 > --- a/drivers/powercap/arm_scmi_powercap.c > +++ b/drivers/powercap/arm_scmi_powercap.c > @@ -519,8 +519,8 @@ static struct scmi_driver scmi_powercap_driver = { > static int __init scmi_powercap_init(void) > { > scmi_top_pcntrl = powercap_register_control_type(NULL, "arm-scmi", NULL); > - if (!scmi_top_pcntrl) > - return -ENODEV; > + if (IS_ERR(scmi_top_pcntrl)) > + return PTR_ERR(scmi_top_pcntrl); > > return scmi_register(&scmi_powercap_driver); > } > -- > 2.35.1 >
On Wed, Jul 06, 2022 at 04:20:41PM +0100, Cristian Marussi wrote: > May I ask which static checker you use ? Sparse/smatch and W=1 did not > spot any of these issues (including other in the series) in my workflow ... > These are Smatch warnings: $ kchecker drivers/powercap/arm_scmi_powercap.c Using test/ version of smatch CALL scripts/checksyscalls.sh CALL scripts/atomic/check-atomics.sh DESCEND objtool CC [M] drivers/powercap/arm_scmi_powercap.o CHECK drivers/powercap/arm_scmi_powercap.c drivers/powercap/arm_scmi_powercap.c:429 scmi_powercap_probe() warn: unsigned 'pr->num_zones' is never less than zero. drivers/powercap/arm_scmi_powercap.c:494 scmi_powercap_probe() error: uninitialized symbol 'ret'. drivers/powercap/arm_scmi_powercap.c:521 scmi_powercap_init() warn: 'scmi_top_pcntrl' is an error pointer or valid $ The problem is that the "is an error pointer or valid" requires the cross function DB to work and that takes forever (over night on my system). regards, dan carpenter
On Wed, Jul 06, 2022 at 06:32:33PM +0300, Dan Carpenter wrote: > On Wed, Jul 06, 2022 at 04:20:41PM +0100, Cristian Marussi wrote: > > May I ask which static checker you use ? Sparse/smatch and W=1 did not > > spot any of these issues (including other in the series) in my workflow ... > > > > These are Smatch warnings: > > $ kchecker drivers/powercap/arm_scmi_powercap.c > > Using test/ version of smatch > > CALL scripts/checksyscalls.sh > CALL scripts/atomic/check-atomics.sh > DESCEND objtool > CC [M] drivers/powercap/arm_scmi_powercap.o > CHECK drivers/powercap/arm_scmi_powercap.c > drivers/powercap/arm_scmi_powercap.c:429 scmi_powercap_probe() warn: unsigned 'pr->num_zones' is never less than zero. > drivers/powercap/arm_scmi_powercap.c:494 scmi_powercap_probe() error: uninitialized symbol 'ret'. > drivers/powercap/arm_scmi_powercap.c:521 scmi_powercap_init() warn: 'scmi_top_pcntrl' is an error pointer or valid > $ > > The problem is that the "is an error pointer or valid" requires the > cross function DB to work and that takes forever (over night on my > system). > Thanks. Turns out even my setup can spot it now (beside the last one), cause my workflow self checks were targeted at where is usually rooted my work drivers/firmware/arm_scmi/ ...so missing out completely on drivers/powercap ... my bad :< Regards Cristian
diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c index ab96cf9a8604..2d505ec7ff81 100644 --- a/drivers/powercap/arm_scmi_powercap.c +++ b/drivers/powercap/arm_scmi_powercap.c @@ -519,8 +519,8 @@ static struct scmi_driver scmi_powercap_driver = { static int __init scmi_powercap_init(void) { scmi_top_pcntrl = powercap_register_control_type(NULL, "arm-scmi", NULL); - if (!scmi_top_pcntrl) - return -ENODEV; + if (IS_ERR(scmi_top_pcntrl)) + return PTR_ERR(scmi_top_pcntrl); return scmi_register(&scmi_powercap_driver); }
The powercap_register_control_type() return error pointers. It never returns NULL. Fixes: 31afdd34f2b9 ("powercap: arm_scmi: Add SCMI powercap based driver") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- This functions should really clean up after itself if scmi_register() fails. I need to fix the static checker for that and then I'll come back and fix it if no one else does. drivers/powercap/arm_scmi_powercap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)