Re: [PATCH 14/22] Input: iqs626a - use cleanup facility for fwnodes

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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.

-- 
Dmitry