| Message ID | 8b4203dc-bc0a-4c00-8862-e2d0ed6e346b@web.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: rcar-csi2: Use common error handling code in rcsi2_parse_dt() | expand |
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
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; > }
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/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
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); > >
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 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 >
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.
> 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
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; }