Skip to contentSkip to conversation

media: rkisp1: Fix unused value issue

PATCH Superseded Dheeraj Reddy Jonnalagadda 3 participants 4 messages Nov 18, 2024 – Nov 19, 2024

Subsystems (3)
Media (V4L/DVB) Arm Rockchip Rockchip ISP V1 driver

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Dheeraj Reddy Jonnalagadda

author
#0 lore raw

PATCH v2 1/1 media: rkisp1: Fix unused value issue

This commit fixes an unused value issue detected by Coverity (CID 1519008). If ret is set to an error value in the switch statement, it is not handled before being overwritten later.

Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +3 -0
index dd114ab77800..9ad5026ab10a 100644
@@ -228,6 +228,9 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
 			break;
 		}
 
+		if (ret)
+			break;
+
 		/* Parse the endpoint and validate the bus type. */
 		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
 		if (ret) {
Signature · 1 line
2.34.1
Reviewed-by on v2 1/1
#1 lore raw

Hi Dheeraj

Quoted 5 lines · Dheeraj Reddy Jonnalagadda

This commit fixes an unused value issue detected by Coverity (CID 1519008). If ret is set to an error value in the switch statement, it is not handled before being overwritten later.

Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>

Indeed there's something fishy here, however the issue is not very much about ret being overritten but rather the error condition

	fwnode_graph_for_each_endpoint(fwnode, ep) {
		switch (reg) {
		case 0:
        HERE   ---->   (!rkisp1_has_feature(rkisp1, MIPI_CSI2)) {
				ret = -EINVAL;
				break;
			}

			break;

		case 1:
			vep.bus_type = V4L2_MBUS_UNKNOWN;
			break;
		}
        }

breaks the inner switch and not the for loop.

I would
1) Slight reword the commit message to make it about missing an error condition
2) Add a Fixes tag

Fixes 7d4f126fde89 ("media: rkisp1: Make the internal CSI-2 receiver optional")

so that this is collected in the stable trees

Quoted 9 lines
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +0 -0
index dd114ab77800..9ad5026ab10a 100644

drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) · line 233

Quoted 6 lines
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +3 -0
@@ -228,6 +228,9 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
 			break;
 		}
 
+		if (ret)
+			break;
+

The change is correct

Reviewed-by Jacopo Mondi <[email protected]>

Thanks
j

Quoted 7 lines
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +0 -1
 		/* Parse the endpoint and validate the bus type. */
 		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
 		if (ret) {
--

2.34.1

author
#2 lore raw
Quoted 54 lines · Jacopo Mondi

Hi Dheeraj

Quoted 5 lines · Dheeraj Reddy Jonnalagadda

This commit fixes an unused value issue detected by Coverity (CID 1519008). If ret is set to an error value in the switch statement, it is not handled before being overwritten later.

Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>

Indeed there's something fishy here, however the issue is not very much about ret being overritten but rather the error condition

	fwnode_graph_for_each_endpoint(fwnode, ep) {
		switch (reg) {
		case 0:
        HERE   ---->   (!rkisp1_has_feature(rkisp1, MIPI_CSI2)) {
				ret = -EINVAL;
				break;
			}

			break;

		case 1:
			vep.bus_type = V4L2_MBUS_UNKNOWN;
			break;
		}
        }

breaks the inner switch and not the for loop.

I would
1) Slight reword the commit message to make it about missing an error condition
2) Add a Fixes tag

Fixes 7d4f126fde89 ("media: rkisp1: Make the internal CSI-2 receiver optional")

so that this is collected in the stable trees

Quoted 9 lines
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +0 -0
index dd114ab77800..9ad5026ab10a 100644

drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1) · line 233

Quoted 6 lines
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +3 -0
@@ -228,6 +228,9 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
 			break;
 		}
 
+		if (ret)
+			break;
+

The change is correct

Quoted 12 lines
Reviewed-by Jacopo Mondi <[email protected]>

Thanks
j

Quoted 7 lines
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +0 -1
 		/* Parse the endpoint and validate the bus type. */
 		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
 		if (ret) {
--

2.34.1

Hi Jacopo,

Thank you for your feedback. I agree with your suggestion and will update the commit message to describe the missing error condition.

-Dheeraj

maintainer Reviewed-by on v2 1/1
#3 lore raw
In reply to Dheeraj Reddy Jonnalagadda

Hi Dheeraj,

Thank you for the patch.

Quoted 5 lines · Dheeraj Reddy Jonnalagadda

This commit fixes an unused value issue detected by Coverity (CID 1519008). If ret is set to an error value in the switch statement, it is not handled before being overwritten later.

Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>
Reviewed-by Laurent Pinchart <[email protected]>
Quoted 18 lines
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +3 -0
index dd114ab77800..9ad5026ab10a 100644
@@ -228,6 +228,9 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
 			break;
 		}
 
+		if (ret)
+			break;
+
 		/* Parse the endpoint and validate the bus type. */
 		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
 		if (ret) {
Signature · 3 lines
Regards,

Laurent Pinchart