[PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Mar 2024 13:02:18 +0100

Add a label so that a bit of exception handling can be better reused
in an if branch of this function implementation.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/platform/renesas/rcar-csi2.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index 582d5e35db0e..621c92c31965 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -1388,12 +1388,13 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 	ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep);
 	if (ret) {
 		dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
-		fwnode_handle_put(ep);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto put_fwnode_ep;
 	}

 	ret = rcsi2_parse_v4l2(priv, &v4l2_ep);
 	if (ret) {
+put_fwnode_ep:
 		fwnode_handle_put(ep);
 		return ret;
 	}
--
2.44.0

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Geert Uytterhoeven]

Hi Markus,

On Fri, Mar 1, 2024 at 1:10 PM Markus Elfring <Markus.Elfring@web.de> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Mar 2024 13:02:18 +0100
>
> Add a label so that a bit of exception handling can be better reused
> in an if branch of this function implementation.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Thanks for your patch!

> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -1388,12 +1388,13 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>         ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep);
>         if (ret) {
>                 dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> -               fwnode_handle_put(ep);
> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto put_fwnode_ep;
>         }
>
>         ret = rcsi2_parse_v4l2(priv, &v4l2_ep);
>         if (ret) {
> +put_fwnode_ep:
>                 fwnode_handle_put(ep);
>                 return ret;
>         }

Please do not use goto to jump to random positions buried deep inside
a function,as this makes the code harder to read.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Laurent Pinchart]

On Fri, Mar 01, 2024 at 01:10:15PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Mar 2024 13:02:18 +0100
> 
> Add a label so that a bit of exception handling can be better reused
> in an if branch of this function implementation.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Just in case someone may be tempted to apply this:

NAK

Markus, don't bother replying to this e-mail, I will delete your reply
without reading it.

> ---
>  drivers/media/platform/renesas/rcar-csi2.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> index 582d5e35db0e..621c92c31965 100644
> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -1388,12 +1388,13 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  	ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep);
>  	if (ret) {
>  		dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> -		fwnode_handle_put(ep);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto put_fwnode_ep;
>  	}
> 
>  	ret = rcsi2_parse_v4l2(priv, &v4l2_ep);
>  	if (ret) {
> +put_fwnode_ep:
>  		fwnode_handle_put(ep);
>  		return ret;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Dan Carpenter]

Sakari Ailus pointed out in another thread that we could use __free()
instead.  Something like this:

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index 582d5e35db0e..c569df6057b7 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -1372,8 +1372,8 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 {
 	struct v4l2_async_connection *asc;
-	struct fwnode_handle *fwnode;
-	struct fwnode_handle *ep;
+	struct fwnode_handle *fwnode __free(fwnode_handle) = NULL;
+	struct fwnode_handle *ep __free(fwnode_handle);
 	struct v4l2_fwnode_endpoint v4l2_ep = {
 		.bus_type = V4L2_MBUS_UNKNOWN,
 	};
@@ -1388,18 +1388,14 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 	ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep);
 	if (ret) {
 		dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
-		fwnode_handle_put(ep);
 		return -EINVAL;
 	}
 
 	ret = rcsi2_parse_v4l2(priv, &v4l2_ep);
-	if (ret) {
-		fwnode_handle_put(ep);
+	if (ret)
 		return ret;
-	}
 
 	fwnode = fwnode_graph_get_remote_endpoint(ep);
-	fwnode_handle_put(ep);
 
 	dev_dbg(priv->dev, "Found '%pOF'\n", to_of_node(fwnode));
 
@@ -1408,7 +1404,6 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 
 	asc = v4l2_async_nf_add_fwnode(&priv->notifier, fwnode,
 				       struct v4l2_async_connection);
-	fwnode_handle_put(fwnode);
 	if (IS_ERR(asc))
 		return PTR_ERR(asc);
 

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Markus Elfring]

> Sakari Ailus pointed out in another thread that we could use __free() instead.

See also:
Contributions by Jonathan Cameron from 2024-02-17

* device property: Move fwnode_handle_put() into property.h
  https://lore.kernel.org/r/20240217164249.921878-2-jic23@kernel.org

* device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
  https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org


> Something like this:
>
> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> index 582d5e35db0e..c569df6057b7 100644
> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -1372,8 +1372,8 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  {
>  	struct v4l2_async_connection *asc;
> -	struct fwnode_handle *fwnode;
> -	struct fwnode_handle *ep;
> +	struct fwnode_handle *fwnode __free(fwnode_handle) = NULL;
> +	struct fwnode_handle *ep __free(fwnode_handle);
>  	struct v4l2_fwnode_endpoint v4l2_ep = {
>  		.bus_type = V4L2_MBUS_UNKNOWN,
>  	};

I suggest to reconsider the position for the adjusted variable declarations
a bit more.


> @@ -1388,18 +1388,14 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  	ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep);
>  	if (ret) {
>  		dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> -		fwnode_handle_put(ep);
>  		return -EINVAL;
>  	}
>
>  	ret = rcsi2_parse_v4l2(priv, &v4l2_ep);
> -	if (ret) {
> -		fwnode_handle_put(ep);
> +	if (ret)
>  		return ret;
> -	}
>
>  	fwnode = fwnode_graph_get_remote_endpoint(ep);
> -	fwnode_handle_put(ep);
>
>  	dev_dbg(priv->dev, "Found '%pOF'\n", to_of_node(fwnode));
>
> @@ -1408,7 +1404,6 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>
>  	asc = v4l2_async_nf_add_fwnode(&priv->notifier, fwnode,
>  				       struct v4l2_async_connection);
> -	fwnode_handle_put(fwnode);
>  	if (IS_ERR(asc))
>  		return PTR_ERR(asc);
>

I find that two function calls marked the end of scopes here
which obviously are not at the end of the discussed function implementation.
Thus I imagine that the known source code transformation “Reduce scope for variables”
will become relevant.
https://refactoring.com/catalog/reduceScopeOfVariable.html

Regards,
Markus

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Sakari Ailus]

Hi Dan,

On Fri, Mar 01, 2024 at 04:42:01PM +0300, Dan Carpenter wrote:
> Sakari Ailus pointed out in another thread that we could use __free()
> instead.  Something like this:
> 

Looks good to me.

We could merge this with your SoB (pending Niklas's review). :-) The driver
has been since moved under drivers/media/platform/renesas/rcar-vin/ .

> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> index 582d5e35db0e..c569df6057b7 100644
> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -1372,8 +1372,8 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  {
>  	struct v4l2_async_connection *asc;
> -	struct fwnode_handle *fwnode;
> -	struct fwnode_handle *ep;
> +	struct fwnode_handle *fwnode __free(fwnode_handle) = NULL;
> +	struct fwnode_handle *ep __free(fwnode_handle);
>  	struct v4l2_fwnode_endpoint v4l2_ep = {
>  		.bus_type = V4L2_MBUS_UNKNOWN,
>  	};
> @@ -1388,18 +1388,14 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  	ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep);
>  	if (ret) {
>  		dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> -		fwnode_handle_put(ep);
>  		return -EINVAL;
>  	}
>  
>  	ret = rcsi2_parse_v4l2(priv, &v4l2_ep);
> -	if (ret) {
> -		fwnode_handle_put(ep);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	fwnode = fwnode_graph_get_remote_endpoint(ep);
> -	fwnode_handle_put(ep);
>  
>  	dev_dbg(priv->dev, "Found '%pOF'\n", to_of_node(fwnode));
>  
> @@ -1408,7 +1404,6 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  
>  	asc = v4l2_async_nf_add_fwnode(&priv->notifier, fwnode,
>  				       struct v4l2_async_connection);
> -	fwnode_handle_put(fwnode);
>  	if (IS_ERR(asc))
>  		return PTR_ERR(asc);
>  
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Dan Carpenter]

On Mon, Mar 04, 2024 at 10:48:47AM +0000, Sakari Ailus wrote:
> Hi Dan,
> 
> On Fri, Mar 01, 2024 at 04:42:01PM +0300, Dan Carpenter wrote:
> > Sakari Ailus pointed out in another thread that we could use __free()
> > instead.  Something like this:
> > 
> 
> Looks good to me.

Thanks for checking!  I've never used these before.

> 
> We could merge this with your SoB (pending Niklas's review). :-) The driver
> has been since moved under drivers/media/platform/renesas/rcar-vin/ .

Alright.  I can resend this as a proper patch.

regards,
dan carpenter

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Niklas Söderlund]

Hi Dan,

On 2024-03-04 14:16:56 +0300, Dan Carpenter wrote:
> On Mon, Mar 04, 2024 at 10:48:47AM +0000, Sakari Ailus wrote:
> > Hi Dan,
> > 
> > On Fri, Mar 01, 2024 at 04:42:01PM +0300, Dan Carpenter wrote:
> > > Sakari Ailus pointed out in another thread that we could use __free()
> > > instead.  Something like this:
> > > 
> > 
> > Looks good to me.
> 
> Thanks for checking!  I've never used these before.
> 
> > 
> > We could merge this with your SoB (pending Niklas's review). :-) The driver
> > has been since moved under drivers/media/platform/renesas/rcar-vin/ .
> 
> Alright.  I can resend this as a proper patch.

Please do.

I do find the idea of scoped operations and the syntax

    struct fwnode_handle *fwnode __free(fwnode_handle) = NULL;

a bit foreign in a C context. But I think the intention is clear and it 
allows us to avoid having the remember to free the fwnode in error paths 
which is a nice thing.

> 
> regards,
> dan carpenter
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Dan Carpenter]

On Sat, Mar 16, 2024 at 10:46:52AM +0100, Niklas Söderlund wrote:
> Hi Dan,
> 
> On 2024-03-04 14:16:56 +0300, Dan Carpenter wrote:
> > On Mon, Mar 04, 2024 at 10:48:47AM +0000, Sakari Ailus wrote:
> > > Hi Dan,
> > > 
> > > On Fri, Mar 01, 2024 at 04:42:01PM +0300, Dan Carpenter wrote:
> > > > Sakari Ailus pointed out in another thread that we could use __free()
> > > > instead.  Something like this:
> > > > 
> > > 
> > > Looks good to me.
> > 
> > Thanks for checking!  I've never used these before.
> > 
> > > 
> > > We could merge this with your SoB (pending Niklas's review). :-) The driver
> > > has been since moved under drivers/media/platform/renesas/rcar-vin/ .
> > 
> > Alright.  I can resend this as a proper patch.
> 
> Please do.
> 
> I do find the idea of scoped operations and the syntax
> 
>     struct fwnode_handle *fwnode __free(fwnode_handle) = NULL;
> 
> a bit foreign in a C context. But I think the intention is clear and it 
> allows us to avoid having the remember to free the fwnode in error paths 
> which is a nice thing.
> 

I said I would send a couple of these but then Markus went ahead and
sent the patches that I was going to write...  And then it was like,
"Oh, these have some questionable style issues" so it wasn't clear what
was happening and I lost track.

regards,
dan carpenter

Re: [PATCH] media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Niklas Söderlund]

On 2024-03-16 12:54:23 +0300, Dan Carpenter wrote:
> On Sat, Mar 16, 2024 at 10:46:52AM +0100, Niklas Söderlund wrote:
> > Hi Dan,
> > 
> > On 2024-03-04 14:16:56 +0300, Dan Carpenter wrote:
> > > On Mon, Mar 04, 2024 at 10:48:47AM +0000, Sakari Ailus wrote:
> > > > Hi Dan,
> > > > 
> > > > On Fri, Mar 01, 2024 at 04:42:01PM +0300, Dan Carpenter wrote:
> > > > > Sakari Ailus pointed out in another thread that we could use __free()
> > > > > instead.  Something like this:
> > > > > 
> > > > 
> > > > Looks good to me.
> > > 
> > > Thanks for checking!  I've never used these before.
> > > 
> > > > 
> > > > We could merge this with your SoB (pending Niklas's review). :-) The driver
> > > > has been since moved under drivers/media/platform/renesas/rcar-vin/ .
> > > 
> > > Alright.  I can resend this as a proper patch.
> > 
> > Please do.
> > 
> > I do find the idea of scoped operations and the syntax
> > 
> >     struct fwnode_handle *fwnode __free(fwnode_handle) = NULL;
> > 
> > a bit foreign in a C context. But I think the intention is clear and it 
> > allows us to avoid having the remember to free the fwnode in error paths 
> > which is a nice thing.
> > 
> 
> I said I would send a couple of these but then Markus went ahead and
> sent the patches that I was going to write...  And then it was like,
> "Oh, these have some questionable style issues" so it wasn't clear what
> was happening and I lost track.

I have not been CCed on any other work in this area for this driver then 
what's in this thread at least. So if you know of no other work in 
another thread I think you can go a head and send a proper patch for 
this driver at least, if you want.

-- 
Kind Regards,
Niklas Söderlund

Re: media: rcar-csi2: Use common error handling code in rcsi2_parse_dt()

[Markus Elfring]

> I said I would send a couple of these but then Markus went ahead and
> sent the patches that I was going to write...

I dared also to touch some software components.


>                                                And then it was like,
> "Oh, these have some questionable style issues"

The patch review is still evolving, isn't it?


> so it wasn't clear what was happening and I lost track.

I find such information surprising.


There are various source code places left over which could be adjusted somehow.

Some contributors would appreciate further clarifications according to
desirable collateral evolution.

See also:
question about kernel cocci and cleanup.h
2024-03-07
https://lore.kernel.org/cocci/CO1PR11MB49149F1167679926A2917E0997202@CO1PR11MB4914.namprd11.prod.outlook.com/

Regards,
Markus