* [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events()
@ 2024-03-02 11:42 Markus Elfring
2024-03-04 3:12 ` Jeff LaBundy
0 siblings, 1 reply; 11+ messages in thread
From: Markus Elfring @ 2024-03-02 11:42 UTC (permalink / raw)
To: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy,
Rob Herring, Uwe Kleine-König
Cc: LKML
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events()
2024-03-02 11:42 [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events() Markus Elfring
@ 2024-03-04 3:12 ` Jeff LaBundy
2024-03-04 4:29 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Jeff LaBundy @ 2024-03-04 3:12 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-input, kernel-janitors, Dmitry Torokhov, Rob Herring,
Uwe Kleine-König, LKML
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events()
2024-03-04 3:12 ` Jeff LaBundy
@ 2024-03-04 4:29 ` Dmitry Torokhov
2024-03-04 8:18 ` Dan Carpenter
2024-03-04 10:52 ` [PATCH v2] Input: iqs626a - Use scope-based resource management " Markus Elfring
0 siblings, 2 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2024-03-04 4:29 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Markus Elfring, linux-input, kernel-janitors, Rob Herring,
Uwe Kleine-König, LKML
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.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events()
2024-03-04 4:29 ` Dmitry Torokhov
@ 2024-03-04 8:18 ` Dan Carpenter
2024-03-04 12:31 ` Markus Elfring
2024-03-04 10:52 ` [PATCH v2] Input: iqs626a - Use scope-based resource management " Markus Elfring
1 sibling, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2024-03-04 8:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jeff LaBundy, Markus Elfring, linux-input, kernel-janitors,
Rob Herring, Uwe Kleine-König, LKML
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
2024-03-04 4:29 ` Dmitry Torokhov
2024-03-04 8:18 ` Dan Carpenter
@ 2024-03-04 10:52 ` Markus Elfring
2024-03-04 10:55 ` Julia Lawall
1 sibling, 1 reply; 11+ messages in thread
From: Markus Elfring @ 2024-03-04 10:52 UTC (permalink / raw)
To: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy,
Rob Herring, Uwe Kleine-König
Cc: LKML, Jonathan Cameron
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 4 Mar 2024 11:40:04 +0100
Scope-based resource management became supported also for this software
area by contributions of Jonathan Cameron on 2024-02-17.
device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
* Thus use the attribute “__free(fwnode_handle)”.
* Reduce the scope for the local variable “ev_node” into a for loop.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
v2:
An other cleanup technique was applied as requested by Dmitry Torokhov
and Jeff LaBundy.
drivers/input/misc/iqs626a.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
index 0dab54d3a060..86fcb5134f45 100644
--- a/drivers/input/misc/iqs626a.c
+++ b/drivers/input/misc/iqs626a.c
@@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
{
struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
struct i2c_client *client = iqs626->client;
- struct fwnode_handle *ev_node;
const char *ev_name;
u8 *thresh, *hyst;
unsigned int val;
@@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
if (!iqs626_channels[ch_id].events[i])
continue;
+ struct fwnode_handle *ev_node __free(fwnode_handle);
+
if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
/*
* Trackpad touch events are simply described under the
@@ -530,7 +531,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
dev_err(&client->dev,
"Invalid input type: %u\n",
val);
- fwnode_handle_put(ev_node);
return -EINVAL;
}
@@ -545,7 +545,6 @@ 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;
}
@@ -566,7 +565,6 @@ 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;
}
@@ -575,8 +573,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
else
*(thresh + iqs626_events[i].th_offs) = val;
}
-
- fwnode_handle_put(ev_node);
}
return 0;
--
2.44.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
2024-03-04 10:52 ` [PATCH v2] Input: iqs626a - Use scope-based resource management " Markus Elfring
@ 2024-03-04 10:55 ` Julia Lawall
2024-03-04 11:32 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Julia Lawall @ 2024-03-04 10:55 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy,
Rob Herring, Uwe Kleine-König, LKML, Jonathan Cameron
[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]
On Mon, 4 Mar 2024, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 4 Mar 2024 11:40:04 +0100
>
> Scope-based resource management became supported also for this software
> area by contributions of Jonathan Cameron on 2024-02-17.
>
> device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
> https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
>
>
> * Thus use the attribute “__free(fwnode_handle)”.
>
> * Reduce the scope for the local variable “ev_node” into a for loop.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>
> v2:
> An other cleanup technique was applied as requested by Dmitry Torokhov
> and Jeff LaBundy.
>
>
> drivers/input/misc/iqs626a.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> index 0dab54d3a060..86fcb5134f45 100644
> --- a/drivers/input/misc/iqs626a.c
> +++ b/drivers/input/misc/iqs626a.c
> @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> {
> struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> struct i2c_client *client = iqs626->client;
> - struct fwnode_handle *ev_node;
> const char *ev_name;
> u8 *thresh, *hyst;
> unsigned int val;
> @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> if (!iqs626_channels[ch_id].events[i])
> continue;
>
> + struct fwnode_handle *ev_node __free(fwnode_handle);
Doesn't this need to be initialized?
julia
> +
> if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
> /*
> * Trackpad touch events are simply described under the
> @@ -530,7 +531,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> dev_err(&client->dev,
> "Invalid input type: %u\n",
> val);
> - fwnode_handle_put(ev_node);
> return -EINVAL;
> }
>
> @@ -545,7 +545,6 @@ 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;
> }
>
> @@ -566,7 +565,6 @@ 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;
> }
>
> @@ -575,8 +573,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> else
> *(thresh + iqs626_events[i].th_offs) = val;
> }
> -
> - fwnode_handle_put(ev_node);
> }
>
> return 0;
> --
> 2.44.0
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
2024-03-04 10:55 ` Julia Lawall
@ 2024-03-04 11:32 ` Dan Carpenter
2024-03-04 13:30 ` Markus Elfring
2024-03-04 12:10 ` [PATCH v2] " Markus Elfring
2024-03-04 17:16 ` Dmitry Torokhov
2 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2024-03-04 11:32 UTC (permalink / raw)
To: Julia Lawall
Cc: Markus Elfring, linux-input, kernel-janitors, Dmitry Torokhov,
Jeff LaBundy, Rob Herring, Uwe Kleine-König, LKML,
Jonathan Cameron
On Mon, Mar 04, 2024 at 11:55:23AM +0100, Julia Lawall wrote:
>
>
> On Mon, 4 Mar 2024, Markus Elfring wrote:
>
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 4 Mar 2024 11:40:04 +0100
> >
> > Scope-based resource management became supported also for this software
> > area by contributions of Jonathan Cameron on 2024-02-17.
> >
> > device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
> > https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
> >
> >
> > * Thus use the attribute “__free(fwnode_handle)”.
> >
> > * Reduce the scope for the local variable “ev_node” into a for loop.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >
> > v2:
> > An other cleanup technique was applied as requested by Dmitry Torokhov
> > and Jeff LaBundy.
> >
> >
> > drivers/input/misc/iqs626a.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > index 0dab54d3a060..86fcb5134f45 100644
> > --- a/drivers/input/misc/iqs626a.c
> > +++ b/drivers/input/misc/iqs626a.c
> > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > {
> > struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> > struct i2c_client *client = iqs626->client;
> > - struct fwnode_handle *ev_node;
> > const char *ev_name;
> > u8 *thresh, *hyst;
> > unsigned int val;
> > @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > if (!iqs626_channels[ch_id].events[i])
> > continue;
> >
> > + struct fwnode_handle *ev_node __free(fwnode_handle);
>
> Doesn't this need to be initialized?
>
Only if you declared before the continue; statement. Generally kernel
style is that you have to declare variables at the start of the block...
But that's becoming less universal now that it's not a compile error.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
2024-03-04 10:55 ` Julia Lawall
2024-03-04 11:32 ` Dan Carpenter
@ 2024-03-04 12:10 ` Markus Elfring
2024-03-04 17:16 ` Dmitry Torokhov
2 siblings, 0 replies; 11+ messages in thread
From: Markus Elfring @ 2024-03-04 12:10 UTC (permalink / raw)
To: Julia Lawall, linux-input, kernel-janitors, Dmitry Torokhov,
Jeff LaBundy, Rob Herring, Uwe Kleine-König
Cc: LKML, Jonathan Cameron
>> Scope-based resource management became supported also for this software
>> area by contributions of Jonathan Cameron on 2024-02-17.
>>
>> device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
>> https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
>>
>>
>> * Thus use the attribute “__free(fwnode_handle)”.
>>
>> * Reduce the scope for the local variable “ev_node” into a for loop.
…
>> +++ b/drivers/input/misc/iqs626a.c
>> @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>> {
>> struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
>> struct i2c_client *client = iqs626->client;
>> - struct fwnode_handle *ev_node;
>> const char *ev_name;
>> u8 *thresh, *hyst;
>> unsigned int val;
>> @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>> if (!iqs626_channels[ch_id].events[i])
>> continue;
>>
>> + struct fwnode_handle *ev_node __free(fwnode_handle);
>
> Doesn't this need to be initialized?
This variable should usually be set in both branches of the subsequent if statement,
shouldn't it?
Please take another look at the proposed scope reduction
for the affected variable.
May additional curly brackets be omitted for this source code transformation?
Regards,
Markus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Input: iqs626a - Use common error handling code in iqs626_parse_events()
2024-03-04 8:18 ` Dan Carpenter
@ 2024-03-04 12:31 ` Markus Elfring
0 siblings, 0 replies; 11+ messages in thread
From: Markus Elfring @ 2024-03-04 12:31 UTC (permalink / raw)
To: Dan Carpenter, Dmitry Torokhov, Jeff LaBundy, linux-input,
kernel-janitors
Cc: Rob Herring, Uwe Kleine-König, LKML
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
2024-03-04 11:32 ` Dan Carpenter
@ 2024-03-04 13:30 ` Markus Elfring
0 siblings, 0 replies; 11+ messages in thread
From: Markus Elfring @ 2024-03-04 13:30 UTC (permalink / raw)
To: Dan Carpenter, Julia Lawall, linux-input, kernel-janitors,
Dmitry Torokhov, Jeff LaBundy, Rob Herring, Uwe Kleine-König
Cc: LKML, Jonathan Cameron
> Generally kernel style is that you have to declare variables at the start of the block...
> But that's becoming less universal now that it's not a compile error.
Will it become more feasible to adjust the scope for further (local) variables?
Regards,
Markus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events()
2024-03-04 10:55 ` Julia Lawall
2024-03-04 11:32 ` Dan Carpenter
2024-03-04 12:10 ` [PATCH v2] " Markus Elfring
@ 2024-03-04 17:16 ` Dmitry Torokhov
2 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2024-03-04 17:16 UTC (permalink / raw)
To: Julia Lawall
Cc: Markus Elfring, linux-input, kernel-janitors, Jeff LaBundy,
Rob Herring, Uwe Kleine-König, LKML, Jonathan Cameron
On Mon, Mar 04, 2024 at 11:55:23AM +0100, Julia Lawall wrote:
>
>
> On Mon, 4 Mar 2024, Markus Elfring wrote:
>
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 4 Mar 2024 11:40:04 +0100
> >
> > Scope-based resource management became supported also for this software
> > area by contributions of Jonathan Cameron on 2024-02-17.
> >
> > device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
> > https://lore.kernel.org/r/20240217164249.921878-3-jic23@kernel.org
> >
> >
> > * Thus use the attribute “__free(fwnode_handle)”.
> >
> > * Reduce the scope for the local variable “ev_node” into a for loop.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >
> > v2:
> > An other cleanup technique was applied as requested by Dmitry Torokhov
> > and Jeff LaBundy.
> >
> >
> > drivers/input/misc/iqs626a.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > index 0dab54d3a060..86fcb5134f45 100644
> > --- a/drivers/input/misc/iqs626a.c
> > +++ b/drivers/input/misc/iqs626a.c
> > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > {
> > struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> > struct i2c_client *client = iqs626->client;
> > - struct fwnode_handle *ev_node;
> > const char *ev_name;
> > u8 *thresh, *hyst;
> > unsigned int val;
> > @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > if (!iqs626_channels[ch_id].events[i])
> > continue;
> >
> > + struct fwnode_handle *ev_node __free(fwnode_handle);
>
> Doesn't this need to be initialized?
It would be better if we had a helper here, iqs626_get_ev_node() or
something.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-03-04 17:16 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-02 11:42 [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events() Markus Elfring
2024-03-04 3:12 ` Jeff LaBundy
2024-03-04 4:29 ` Dmitry Torokhov
2024-03-04 8:18 ` Dan Carpenter
2024-03-04 12:31 ` Markus Elfring
2024-03-04 10:52 ` [PATCH v2] Input: iqs626a - Use scope-based resource management " Markus Elfring
2024-03-04 10:55 ` Julia Lawall
2024-03-04 11:32 ` Dan Carpenter
2024-03-04 13:30 ` Markus Elfring
2024-03-04 12:10 ` [PATCH v2] " Markus Elfring
2024-03-04 17:16 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).