diff mbox series

[2/2] powercap: arm_scmi: Fix a NULL vs IS_ERR() bug

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

Commit Message

Dan Carpenter July 6, 2022, 2:27 p.m. UTC
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(-)

Comments

Cristian Marussi July 6, 2022, 3:20 p.m. UTC | #1
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
>
Dan Carpenter July 6, 2022, 3:32 p.m. UTC | #2
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
Cristian Marussi July 6, 2022, 3:46 p.m. UTC | #3
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 mbox series

Patch

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);
 }