powercap: arm_scmi: Fix signedness bug in probe

PATCH In review Dan Carpenter 2 participants 6 messages Jul 6, 2022

Subsystems (3)

Kernel janitors Power management core System control & power/management interface (SCPI/SCMI) Message Protocol drivers

lore

Share & tools

lore

Dan Carpenter

author

2022-07-06 14:27 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/powercap/arm_scmi_powercap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/powercap/arm_scmi_powercap.c +4 -3

index 2273768d70ce..ab96cf9a8604 100644
@@ -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);

Signature · 1 line

2.35.1

Dan Carpenter

author

2022-07-06 14:27 UTC #1 lore raw

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

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 <[email protected]>

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

drivers/powercap/arm_scmi_powercap.c +2 -2

index ab96cf9a8604..2d505ec7ff81 100644
@@ -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);
 }

Signature · 1 line

2.35.1

Cristian Marussi

reviewer

2022-07-06 15:20 UTC #2 lore raw

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

Quoted 10 lines · 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 <[email protected]>
---
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

Quoted 21 lines

 drivers/powercap/arm_scmi_powercap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/powercap/arm_scmi_powercap.c +2 -2

index ab96cf9a8604..2d505ec7ff81 100644
@@ -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);
 }

Signature · 1 line

2.35.1

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

reviewer

2022-07-06 15:51 UTC #4 lore raw

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

Quoted 21 lines · Dan Carpenter

Quoted 3 lines · Cristian Marussi
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 $

Quoted 4 lines

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

Cristian Marussi

reviewer

2022-07-06 14:52 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 5 lines · 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 <[email protected]>

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

Quoted 27 lines

---
 drivers/powercap/arm_scmi_powercap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/powercap/arm_scmi_powercap.c +4 -3

index 2273768d70ce..ab96cf9a8604 100644
@@ -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);

Signature · 1 line

2.35.1