[v2] media: rkisp1: Fix unused value issue

[PATCH v2 media-next] media: rkisp1: Fix unused value issue

Posted by Dheeraj Reddy Jonnalagadda 1 year, 2 months ago

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 <dheeraj.linuxdev@gmail.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index dd114ab77800..9ad5026ab10a 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -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) {
-- 
2.34.1

Re: [PATCH v2 media-next] media: rkisp1: Fix unused value issue

Posted by Laurent Pinchart 1 year, 2 months ago

Hi Dheeraj,

Thank you for the patch.

On Mon, Nov 18, 2024 at 03:07:21PM +0530, Dheeraj Reddy Jonnalagadda wrote:
> 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 <dheeraj.linuxdev@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index dd114ab77800..9ad5026ab10a 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -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) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 media-next] media: rkisp1: Fix unused value issue

Posted by Jacopo Mondi 1 year, 2 months ago

Hi Dheeraj

On Mon, Nov 18, 2024 at 03:07:21PM +0530, Dheeraj Reddy Jonnalagadda wrote:
> 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 <dheeraj.linuxdev@gmail.com>

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

> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index dd114ab77800..9ad5026ab10a 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -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 <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  		/* Parse the endpoint and validate the bus type. */
>  		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
>  		if (ret) {
> --
> 2.34.1
>
>

Re: [PATCH v2 media-next] media: rkisp1: Fix unused value issue

Posted by Dheeraj Reddy Jonnalagadda 1 year, 2 months ago

On Mon, Nov 18, 2024 at 11:18:34AM +0100, Jacopo Mondi wrote:
> Hi Dheeraj
> 
> On Mon, Nov 18, 2024 at 03:07:21PM +0530, Dheeraj Reddy Jonnalagadda wrote:
> > 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 <dheeraj.linuxdev@gmail.com>
> 
> 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
> 
> > ---
> >  drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > index dd114ab77800..9ad5026ab10a 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -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 <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>   j
> 
> >  		/* 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