diff mbox series

Input: iqs626a - Use common error handling code in iqs626_parse_events()

Message ID 8a7607f8-d634-415e-8269-e26dcc0f9fdc@web.de (mailing list archive)
State New
Headers show
Series Input: iqs626a - Use common error handling code in iqs626_parse_events() | expand

Commit Message

Markus Elfring March 2, 2024, 11:42 a.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Mar 2024 11:44:17 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function implementation.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/input/misc/iqs626a.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

--
2.44.0

Comments

Jeff LaBundy March 4, 2024, 3:12 a.m. UTC | #1 
Hi Markus,

On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Mar 2024 11:44:17 +0100
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function implementation.
> 
> This issue was transformed by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/input/misc/iqs626a.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> index 0dab54d3a060..fa9570755f7b 100644
> --- a/drivers/input/misc/iqs626a.c
> +++ b/drivers/input/misc/iqs626a.c
> @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  					dev_err(&client->dev,
>  						"Invalid input type: %u\n",
>  						val);
> -					fwnode_handle_put(ev_node);
> -					return -EINVAL;
> +					goto put_fwnode;
>  				}
> 
>  				iqs626->kp_type[ch_id][i] = val;
> @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s channel hysteresis: %u\n",
>  					fwnode_get_name(ch_node), val);
> -				fwnode_handle_put(ev_node);
> -				return -EINVAL;
> +				goto put_fwnode;
>  			}
> 
>  			if (i == IQS626_EVENT_DEEP_DN ||
> @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s channel threshold: %u\n",
>  					fwnode_get_name(ch_node), val);
> -				fwnode_handle_put(ev_node);
> -				return -EINVAL;
> +				goto put_fwnode;
>  			}
> 
>  			if (ch_id == IQS626_CH_HALL)
> @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  	}
> 
>  	return 0;
> +
> +put_fwnode:
> +	fwnode_handle_put(ev_node);
> +	return -EINVAL;
>  }
> 
>  static noinline_for_stack int
> --
> 2.44.0
> 

Thank you for this patch, but it seems like a NAK to me. I think this is
a matter of personal preference, and according to mine, it is much more
confusing to insert a goto label after a function's primary return path
than to have 2-3 calls to fwnode_handle_put().

If you feel strongly otherwise, then I would suggest a helper function as
recommended by Dmitry in another thread. However, maybe that helper should
live in the driver core, as I suspect this driver is not the only place we
can avoid calling fwnode_handle_put() in an error path that returns an int.

Kind regards,
Jeff LaBundy
Dmitry Torokhov March 4, 2024, 4:29 a.m. UTC | #2 
On Sun, Mar 03, 2024 at 09:12:16PM -0600, Jeff LaBundy wrote:
> Hi Markus,
> 
> On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sat, 2 Mar 2024 11:44:17 +0100
> > 
> > Add a jump target so that a bit of exception handling can be better reused
> > at the end of this function implementation.
> > 
> > This issue was transformed by using the Coccinelle software.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/input/misc/iqs626a.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > index 0dab54d3a060..fa9570755f7b 100644
> > --- a/drivers/input/misc/iqs626a.c
> > +++ b/drivers/input/misc/iqs626a.c
> > @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  					dev_err(&client->dev,
> >  						"Invalid input type: %u\n",
> >  						val);
> > -					fwnode_handle_put(ev_node);
> > -					return -EINVAL;
> > +					goto put_fwnode;
> >  				}
> > 
> >  				iqs626->kp_type[ch_id][i] = val;
> > @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  				dev_err(&client->dev,
> >  					"Invalid %s channel hysteresis: %u\n",
> >  					fwnode_get_name(ch_node), val);
> > -				fwnode_handle_put(ev_node);
> > -				return -EINVAL;
> > +				goto put_fwnode;
> >  			}
> > 
> >  			if (i == IQS626_EVENT_DEEP_DN ||
> > @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  				dev_err(&client->dev,
> >  					"Invalid %s channel threshold: %u\n",
> >  					fwnode_get_name(ch_node), val);
> > -				fwnode_handle_put(ev_node);
> > -				return -EINVAL;
> > +				goto put_fwnode;
> >  			}
> > 
> >  			if (ch_id == IQS626_CH_HALL)
> > @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  	}
> > 
> >  	return 0;
> > +
> > +put_fwnode:
> > +	fwnode_handle_put(ev_node);
> > +	return -EINVAL;
> >  }
> > 
> >  static noinline_for_stack int
> > --
> > 2.44.0
> > 
> 
> Thank you for this patch, but it seems like a NAK to me. I think this is
> a matter of personal preference, and according to mine, it is much more
> confusing to insert a goto label after a function's primary return path
> than to have 2-3 calls to fwnode_handle_put().
> 
> If you feel strongly otherwise, then I would suggest a helper function as
> recommended by Dmitry in another thread. However, maybe that helper should
> live in the driver core, as I suspect this driver is not the only place we
> can avoid calling fwnode_handle_put() in an error path that returns an int.

Yes, it should go into include/linux/fwnode.h, something like

DEFINE_FREE(fwnode, struct fwnode_handle *, if (_T) fwnode_hanlde_put(_T));

Then drivers can do:

	struct fwnode_handle *ev_node __free(fwnode) =
		fwnode_get_named_child_node(ch_node, ev_name);

and have it automatically be "put" once execution leaves the variable
scope.

Ah, we actually already have it defined in include/linux/property.h, all
the better.

Thanks.
Dan Carpenter March 4, 2024, 8:18 a.m. UTC | #3 
On Sun, Mar 03, 2024 at 08:29:49PM -0800, Dmitry Torokhov wrote:
> On Sun, Mar 03, 2024 at 09:12:16PM -0600, Jeff LaBundy wrote:
> > Hi Markus,
> > 
> > On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote:
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Sat, 2 Mar 2024 11:44:17 +0100
> > > 
> > > Add a jump target so that a bit of exception handling can be better reused
> > > at the end of this function implementation.
> > > 
> > > This issue was transformed by using the Coccinelle software.
> > > 
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > ---
> > >  drivers/input/misc/iqs626a.c | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > > index 0dab54d3a060..fa9570755f7b 100644
> > > --- a/drivers/input/misc/iqs626a.c
> > > +++ b/drivers/input/misc/iqs626a.c
> > > @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  					dev_err(&client->dev,
> > >  						"Invalid input type: %u\n",
> > >  						val);
> > > -					fwnode_handle_put(ev_node);
> > > -					return -EINVAL;
> > > +					goto put_fwnode;
> > >  				}
> > > 
> > >  				iqs626->kp_type[ch_id][i] = val;
> > > @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  				dev_err(&client->dev,
> > >  					"Invalid %s channel hysteresis: %u\n",
> > >  					fwnode_get_name(ch_node), val);
> > > -				fwnode_handle_put(ev_node);
> > > -				return -EINVAL;
> > > +				goto put_fwnode;
> > >  			}
> > > 
> > >  			if (i == IQS626_EVENT_DEEP_DN ||
> > > @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  				dev_err(&client->dev,
> > >  					"Invalid %s channel threshold: %u\n",
> > >  					fwnode_get_name(ch_node), val);
> > > -				fwnode_handle_put(ev_node);
> > > -				return -EINVAL;
> > > +				goto put_fwnode;
> > >  			}
> > > 
> > >  			if (ch_id == IQS626_CH_HALL)
> > > @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  	}
> > > 
> > >  	return 0;
> > > +
> > > +put_fwnode:
> > > +	fwnode_handle_put(ev_node);
> > > +	return -EINVAL;
> > >  }
> > > 
> > >  static noinline_for_stack int
> > > --
> > > 2.44.0
> > > 
> > 
> > Thank you for this patch, but it seems like a NAK to me. I think this is
> > a matter of personal preference, and according to mine, it is much more
> > confusing to insert a goto label after a function's primary return path
> > than to have 2-3 calls to fwnode_handle_put().
> > 
> > If you feel strongly otherwise, then I would suggest a helper function as
> > recommended by Dmitry in another thread. However, maybe that helper should
> > live in the driver core, as I suspect this driver is not the only place we
> > can avoid calling fwnode_handle_put() in an error path that returns an int.
> 
> Yes, it should go into include/linux/fwnode.h, something like
> 
> DEFINE_FREE(fwnode, struct fwnode_handle *, if (_T) fwnode_hanlde_put(_T));
> 
> Then drivers can do:
> 
> 	struct fwnode_handle *ev_node __free(fwnode) =
> 		fwnode_get_named_child_node(ch_node, ev_name);
> 
> and have it automatically be "put" once execution leaves the variable
> scope.
> 
> Ah, we actually already have it defined in include/linux/property.h, all
> the better.

It's already there.

DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T))

I can send a patch for this.  You need to be a bit carefull to move
the declaration into the correct scope for this to work.  I should write
some Smatch rules for this...

regards,
dan carpenter
Markus Elfring March 4, 2024, 12:31 p.m. UTC | #4 
> DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T))
>
> I can send a patch for this.  You need to be a bit carefull to move
> the declaration into the correct scope for this to work.  I should write
> some Smatch rules for this...

I became also curious how available development tools will evolve further
for improved handling of scope-based resource management.

Regards,
Markus
diff mbox series

Patch

diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
index 0dab54d3a060..fa9570755f7b 100644
--- a/drivers/input/misc/iqs626a.c
+++ b/drivers/input/misc/iqs626a.c
@@ -530,8 +530,7 @@  iqs626_parse_events(struct iqs626_private *iqs626,
 					dev_err(&client->dev,
 						"Invalid input type: %u\n",
 						val);
-					fwnode_handle_put(ev_node);
-					return -EINVAL;
+					goto put_fwnode;
 				}

 				iqs626->kp_type[ch_id][i] = val;
@@ -545,8 +544,7 @@  iqs626_parse_events(struct iqs626_private *iqs626,
 				dev_err(&client->dev,
 					"Invalid %s channel hysteresis: %u\n",
 					fwnode_get_name(ch_node), val);
-				fwnode_handle_put(ev_node);
-				return -EINVAL;
+				goto put_fwnode;
 			}

 			if (i == IQS626_EVENT_DEEP_DN ||
@@ -566,8 +564,7 @@  iqs626_parse_events(struct iqs626_private *iqs626,
 				dev_err(&client->dev,
 					"Invalid %s channel threshold: %u\n",
 					fwnode_get_name(ch_node), val);
-				fwnode_handle_put(ev_node);
-				return -EINVAL;
+				goto put_fwnode;
 			}

 			if (ch_id == IQS626_CH_HALL)
@@ -580,6 +577,10 @@  iqs626_parse_events(struct iqs626_private *iqs626,
 	}

 	return 0;
+
+put_fwnode:
+	fwnode_handle_put(ev_node);
+	return -EINVAL;
 }

 static noinline_for_stack int