| Message ID | 20240904044814.1048062-1-dmitry.torokhov@gmail.com |
|---|---|
| State | Accepted |
| Commit | 52776177d8b7ea4ffd71c6728d472f7aba8855c5 |
| Headers | show |
| Series | [01/22] Input: ad714x - use guard notation when acquiring mutex | expand |
Hi Dmitry, On Tue, Sep 03, 2024 at 09:48:13PM -0700, Dmitry Torokhov wrote: > Use __free(fwnode_handle) cleanup facility to ensure that references to > acquired fwnodes are dropped at appropriate times automatically. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/misc/iqs626a.c | 22 ++++++---------------- > 1 file changed, 6 insertions(+), 16 deletions(-) > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > index 0dab54d3a060..7a6e6927f331 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,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > if (!iqs626_channels[ch_id].events[i]) > continue; > > + struct fwnode_handle *ev_node __free(fwnode_handle) = NULL; This seems to deviate from what I understand to be a more conventional style of declaring variables at the top of the scope, and separate from actual code, like below: for (i = 0; i < ARRAY_SIZE(iqs626_events); i++) { struct fwnode_handle *ev_node __free(fwnode_handle); if (!iqs626_channels[ch_id].events[i]) continue; I also did not see any reason to explicitly declare the variable as NULL; let me know in case I have misunderstood. > if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) { > /* > * Trackpad touch events are simply described under the > @@ -530,7 +530,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 +544,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 +564,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 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > else > *(thresh + iqs626_events[i].th_offs) = val; > } > - > - fwnode_handle_put(ev_node); > } > > return 0; > @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626, > for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) { > u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base; > u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh; > - struct fwnode_handle *tc_node; > char tc_name[10]; > > snprintf(tc_name, sizeof(tc_name), "channel-%d", i); > > - tc_node = fwnode_get_named_child_node(ch_node, tc_name); > + struct fwnode_handle *tc_node __free(fwnode_handle) = > + fwnode_get_named_child_node(ch_node, tc_name); Same here. > if (!tc_node) > continue; > > @@ -790,7 +785,6 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626, > dev_err(&client->dev, > "Invalid %s %s ATI base: %u\n", > fwnode_get_name(ch_node), tc_name, val); > - fwnode_handle_put(tc_node); > return -EINVAL; > } > > @@ -803,14 +797,11 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626, > dev_err(&client->dev, > "Invalid %s %s threshold: %u\n", > fwnode_get_name(ch_node), tc_name, val); > - fwnode_handle_put(tc_node); > return -EINVAL; > } > > *thresh = val; > } > - > - fwnode_handle_put(tc_node); > } > > if (!fwnode_property_present(ch_node, "linux,keycodes")) > @@ -1233,7 +1224,6 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626) > { > struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg; > struct i2c_client *client = iqs626->client; > - struct fwnode_handle *ch_node; > unsigned int val; > int error, i; > u16 general; > @@ -1375,13 +1365,13 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626) > sys_reg->active = 0; > > for (i = 0; i < ARRAY_SIZE(iqs626_channels); i++) { > - ch_node = device_get_named_child_node(&client->dev, > - iqs626_channels[i].name); > + struct fwnode_handle *ch_node __free(fwnode_handle) = > + device_get_named_child_node(&client->dev, > + iqs626_channels[i].name); > if (!ch_node) > continue; > > error = iqs626_parse_channel(iqs626, ch_node, i); > - fwnode_handle_put(ch_node); > if (error) > return error; > > -- > 2.46.0.469.g59c65b2a67-goog > Kind regards, Jeff LaBundy
Hi Jeff, On Sun, Sep 08, 2024 at 07:02:41PM -0500, Jeff LaBundy wrote: > Hi Dmitry, > > On Tue, Sep 03, 2024 at 09:48:13PM -0700, Dmitry Torokhov wrote: > > Use __free(fwnode_handle) cleanup facility to ensure that references to > > acquired fwnodes are dropped at appropriate times automatically. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/input/misc/iqs626a.c | 22 ++++++---------------- > > 1 file changed, 6 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > > index 0dab54d3a060..7a6e6927f331 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,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > if (!iqs626_channels[ch_id].events[i]) > > continue; > > > > + struct fwnode_handle *ev_node __free(fwnode_handle) = NULL; > > This seems to deviate from what I understand to be a more conventional > style of declaring variables at the top of the scope, and separate from > actual code, like below: This is follows Linus' guidance on combining declaration and initialization for variables using __free() cleanup annotations (where possible). These annotations are the reason for dropping -Wdeclaration-after-statement from our makefiles. See b5ec6fd286df ("kbuild: Drop -Wdeclaration-after-statement") and discussion in https://lore.kernel.org/all/CAHk-=wi-RyoUhbChiVaJZoZXheAwnJ7OO=Gxe85BkPAd93TwDA@mail.gmail.com > > > for (i = 0; i < ARRAY_SIZE(iqs626_events); i++) { > struct fwnode_handle *ev_node __free(fwnode_handle); > > if (!iqs626_channels[ch_id].events[i]) > continue; This will result in attempt to "put" a garbage pointer if we follow "continue" code path. In general __free() pointers have to be initialized, either to NULL or to a valid object (assuming that cleanup function can deal with NULLs). > > I also did not see any reason to explicitly declare the variable as NULL; > let me know in case I have misunderstood. See the above. Yes, in this particular case it will get a value in both branches, but I feel it is too fragile and may get messed up if someone refactors code. > > > if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) { > > /* > > * Trackpad touch events are simply described under the > > @@ -530,7 +530,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 +544,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 +564,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 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > else > > *(thresh + iqs626_events[i].th_offs) = val; > > } > > - > > - fwnode_handle_put(ev_node); > > } > > > > return 0; > > @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626, > > for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) { > > u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base; > > u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh; > > - struct fwnode_handle *tc_node; > > char tc_name[10]; > > > > snprintf(tc_name, sizeof(tc_name), "channel-%d", i); > > > > - tc_node = fwnode_get_named_child_node(ch_node, tc_name); > > + struct fwnode_handle *tc_node __free(fwnode_handle) = > > + fwnode_get_named_child_node(ch_node, tc_name); > > Same here. Yes, combining declaration and initialization is preferred for such pointers. Thanks.
Hi Dmitry, On Sun, Sep 08, 2024 at 06:31:26PM -0700, Dmitry Torokhov wrote: > Hi Jeff, > > On Sun, Sep 08, 2024 at 07:02:41PM -0500, Jeff LaBundy wrote: > > Hi Dmitry, > > > > On Tue, Sep 03, 2024 at 09:48:13PM -0700, Dmitry Torokhov wrote: > > > Use __free(fwnode_handle) cleanup facility to ensure that references to > > > acquired fwnodes are dropped at appropriate times automatically. > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > --- > > > drivers/input/misc/iqs626a.c | 22 ++++++---------------- > > > 1 file changed, 6 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > > > index 0dab54d3a060..7a6e6927f331 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,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > > if (!iqs626_channels[ch_id].events[i]) > > > continue; > > > > > > + struct fwnode_handle *ev_node __free(fwnode_handle) = NULL; > > > > This seems to deviate from what I understand to be a more conventional > > style of declaring variables at the top of the scope, and separate from > > actual code, like below: > > This is follows Linus' guidance on combining declaration and > initialization for variables using __free() cleanup annotations (where > possible). These annotations are the reason for dropping > -Wdeclaration-after-statement from our makefiles. See b5ec6fd286df > ("kbuild: Drop -Wdeclaration-after-statement") and discussion in > https://lore.kernel.org/all/CAHk-=wi-RyoUhbChiVaJZoZXheAwnJ7OO=Gxe85BkPAd93TwDA@mail.gmail.com Understood; thank you for the reference. > > > > > > > for (i = 0; i < ARRAY_SIZE(iqs626_events); i++) { > > struct fwnode_handle *ev_node __free(fwnode_handle); > > > > if (!iqs626_channels[ch_id].events[i]) > > continue; > > This will result in attempt to "put" a garbage pointer if we follow > "continue" code path. In general __free() pointers have to be > initialized, either to NULL or to a valid object (assuming that cleanup > function can deal with NULLs). Great catch; I missed the fact that fwnode_handle_put() is implicitly being called in all exit paths now. > > > > > I also did not see any reason to explicitly declare the variable as NULL; > > let me know in case I have misunderstood. > > See the above. Yes, in this particular case it will get a value in both > branches, but I feel it is too fragile and may get messed up if someone > refactors code. Based on the above point, I agree with your approach. > > > > > > if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) { > > > /* > > > * Trackpad touch events are simply described under the > > > @@ -530,7 +530,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 +544,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 +564,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 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > > else > > > *(thresh + iqs626_events[i].th_offs) = val; > > > } > > > - > > > - fwnode_handle_put(ev_node); > > > } > > > > > > return 0; > > > @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626, > > > for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) { > > > u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base; > > > u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh; > > > - struct fwnode_handle *tc_node; > > > char tc_name[10]; > > > > > > snprintf(tc_name, sizeof(tc_name), "channel-%d", i); > > > > > > - tc_node = fwnode_get_named_child_node(ch_node, tc_name); > > > + struct fwnode_handle *tc_node __free(fwnode_handle) = > > > + fwnode_get_named_child_node(ch_node, tc_name); > > > > Same here. > > Yes, combining declaration and initialization is preferred for such > pointers. ACK. Please feel free to add: Reviewed-by: Jeff LaBundy <jeff@labundy.com> > > Thanks. > > -- > Dmitry Thank you for the discussion! Kind regards, Jeff LaBundy
diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c index 0dab54d3a060..7a6e6927f331 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,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, if (!iqs626_channels[ch_id].events[i]) continue; + struct fwnode_handle *ev_node __free(fwnode_handle) = NULL; if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) { /* * Trackpad touch events are simply described under the @@ -530,7 +530,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 +544,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 +564,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 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, else *(thresh + iqs626_events[i].th_offs) = val; } - - fwnode_handle_put(ev_node); } return 0; @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626, for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) { u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base; u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh; - struct fwnode_handle *tc_node; char tc_name[10]; snprintf(tc_name, sizeof(tc_name), "channel-%d", i); - tc_node = fwnode_get_named_child_node(ch_node, tc_name); + struct fwnode_handle *tc_node __free(fwnode_handle) = + fwnode_get_named_child_node(ch_node, tc_name); if (!tc_node) continue; @@ -790,7 +785,6 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626, dev_err(&client->dev, "Invalid %s %s ATI base: %u\n", fwnode_get_name(ch_node), tc_name, val); - fwnode_handle_put(tc_node); return -EINVAL; } @@ -803,14 +797,11 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626, dev_err(&client->dev, "Invalid %s %s threshold: %u\n", fwnode_get_name(ch_node), tc_name, val); - fwnode_handle_put(tc_node); return -EINVAL; } *thresh = val; } - - fwnode_handle_put(tc_node); } if (!fwnode_property_present(ch_node, "linux,keycodes")) @@ -1233,7 +1224,6 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626) { struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg; struct i2c_client *client = iqs626->client; - struct fwnode_handle *ch_node; unsigned int val; int error, i; u16 general; @@ -1375,13 +1365,13 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626) sys_reg->active = 0; for (i = 0; i < ARRAY_SIZE(iqs626_channels); i++) { - ch_node = device_get_named_child_node(&client->dev, - iqs626_channels[i].name); + struct fwnode_handle *ch_node __free(fwnode_handle) = + device_get_named_child_node(&client->dev, + iqs626_channels[i].name); if (!ch_node) continue; error = iqs626_parse_channel(iqs626, ch_node, i); - fwnode_handle_put(ch_node); if (error) return error;
Use __free(fwnode_handle) cleanup facility to ensure that references to acquired fwnodes are dropped at appropriate times automatically. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/misc/iqs626a.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-)