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@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
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@xxxxxxxxxxx>
>
> Thanks.
>
> --
> Dmitry
Thank you for the discussion!
Kind regards,
Jeff LaBundy
