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

[Jeff LaBundy <jeff@labundy.com>]

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