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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx
>
>
> 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.
--
Dmitry
