From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
Hi Markus,
On Fri, Mar 1, 2024 at 1:10 PM Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
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-m68korg
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
On Fri, Mar 01, 2024 at 01:10:15PM +0100, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
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
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);
> 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/[email protected]
* device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
https://lore.kernel.org/r/[email protected]
> 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
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
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
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
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
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
> 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