[PATCH 1/2] powercap: arm_scmi: Fix signedness bug in probe

[PATCH 1/2] powercap: arm_scmi: Fix signedness bug in probe

[Dan Carpenter]

The "pr->num_zones" variable is an unsigned int so it can't be less than
zero.

Fixes: 31afdd34f2b9 ("powercap: arm_scmi: Add SCMI powercap based driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/powercap/arm_scmi_powercap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
index 2273768d70ce..ab96cf9a8604 100644
--- a/drivers/powercap/arm_scmi_powercap.c
+++ b/drivers/powercap/arm_scmi_powercap.c
@@ -425,11 +425,12 @@ static int scmi_powercap_probe(struct scmi_device *sdev)
 	if (!pr)
 		return -ENOMEM;
 
-	pr->num_zones = powercap_ops->num_domains_get(ph);
-	if (pr->num_zones < 0) {
+	ret = powercap_ops->num_domains_get(ph);
+	if (ret < 0) {
 		dev_err(dev, "number of powercap domains not found\n");
-		return pr->num_zones;
+		return ret;
 	}
+	pr->num_zones = ret;
 
 	pr->spzones = devm_kcalloc(dev, pr->num_zones,
 				   sizeof(*pr->spzones), GFP_KERNEL);
-- 
2.35.1

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

[Dan Carpenter]

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(-)

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

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

[Cristian Marussi]

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
> 

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

[Dan Carpenter]

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

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

[Cristian Marussi]

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

Re: [PATCH 1/2] powercap: arm_scmi: Fix signedness bug in probe

[Cristian Marussi]

On Wed, Jul 06, 2022 at 05:26:56PM +0300, Dan Carpenter wrote:
> The "pr->num_zones" variable is an unsigned int so it can't be less than
> zero.
> 
> Fixes: 31afdd34f2b9 ("powercap: arm_scmi: Add SCMI powercap based driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Hi,

Thanks for having a look.

This driver has NOT been pulled in this cycle due to a lack of
reviews/feedbacks so I'll integrate your fixes in the next version.

Thanks,
Cristian

> ---
>  drivers/powercap/arm_scmi_powercap.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
> index 2273768d70ce..ab96cf9a8604 100644
> --- a/drivers/powercap/arm_scmi_powercap.c
> +++ b/drivers/powercap/arm_scmi_powercap.c
> @@ -425,11 +425,12 @@ static int scmi_powercap_probe(struct scmi_device *sdev)
>  	if (!pr)
>  		return -ENOMEM;
>  
> -	pr->num_zones = powercap_ops->num_domains_get(ph);
> -	if (pr->num_zones < 0) {
> +	ret = powercap_ops->num_domains_get(ph);
> +	if (ret < 0) {
>  		dev_err(dev, "number of powercap domains not found\n");
> -		return pr->num_zones;
> +		return ret;
>  	}
> +	pr->num_zones = ret;
>  
>  	pr->spzones = devm_kcalloc(dev, pr->num_zones,
>  				   sizeof(*pr->spzones), GFP_KERNEL);
> -- 
> 2.35.1
>