diff mbox series

[01/11] Input: iqs7222 - drop unused device node references

Message ID 20220908131548.48120-2-jeff@labundy.com
State New
Headers show
Series Additional fixes for Azoteq IQS7222A/B/C | expand

Commit Message

Jeff LaBundy Sept. 8, 2022, 1:15 p.m. UTC 
Each call to device/fwnode_get_named_child_node() must be matched
with a call to fwnode_handle_put() once the corresponding node is
no longer in use. This ensures a reference count remains balanced
in the case of dynamic device tree support.

Currently, the driver never calls fwnode_handle_put(). This patch
adds the missing calls.

Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/input/misc/iqs7222.c | 134 ++++++++++++++++++++++++-----------
 1 file changed, 91 insertions(+), 43 deletions(-)

Comments

Dmitry Torokhov Sept. 8, 2022, 9:17 p.m. UTC | #1 
On Thu, Sep 08, 2022 at 08:15:38AM -0500, Jeff LaBundy wrote:
> Each call to device/fwnode_get_named_child_node() must be matched
> with a call to fwnode_handle_put() once the corresponding node is
> no longer in use. This ensures a reference count remains balanced
> in the case of dynamic device tree support.
> 
> Currently, the driver never calls fwnode_handle_put(). This patch
> adds the missing calls.

Hmm, dev_fwnode() however does not do that, which means that
iqs7222_parse_props() has different refounting, depending on what is
being fetched. I think we need to start there.

Also, maybe we could avoid sprinkling gotos if we moved property reading
code into helpers?

Thanks.
Jeff LaBundy Sept. 9, 2022, 2:04 a.m. UTC | #2 
Hi Dmitry,

Thank you for taking a look.

On Thu, Sep 08, 2022 at 02:17:21PM -0700, Dmitry Torokhov wrote:
> On Thu, Sep 08, 2022 at 08:15:38AM -0500, Jeff LaBundy wrote:
> > Each call to device/fwnode_get_named_child_node() must be matched
> > with a call to fwnode_handle_put() once the corresponding node is
> > no longer in use. This ensures a reference count remains balanced
> > in the case of dynamic device tree support.
> > 
> > Currently, the driver never calls fwnode_handle_put(). This patch
> > adds the missing calls.
> 
> Hmm, dev_fwnode() however does not do that, which means that
> iqs7222_parse_props() has different refounting, depending on what is
> being fetched. I think we need to start there.

Right, but none of the callers that prompt iqs7222_parse_props() to
use dev_fwnode() follow with fwnode_handle_put().

> 
> Also, maybe we could avoid sprinkling gotos if we moved property reading
> code into helpers?

I like this idea; I will give it a try.

> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy
Dmitry Torokhov Sept. 9, 2022, 4:37 a.m. UTC | #3 
On Thu, Sep 08, 2022 at 09:04:06PM -0500, Jeff LaBundy wrote:
> Hi Dmitry,
> 
> Thank you for taking a look.
> 
> On Thu, Sep 08, 2022 at 02:17:21PM -0700, Dmitry Torokhov wrote:
> > On Thu, Sep 08, 2022 at 08:15:38AM -0500, Jeff LaBundy wrote:
> > > Each call to device/fwnode_get_named_child_node() must be matched
> > > with a call to fwnode_handle_put() once the corresponding node is
> > > no longer in use. This ensures a reference count remains balanced
> > > in the case of dynamic device tree support.
> > > 
> > > Currently, the driver never calls fwnode_handle_put(). This patch
> > > adds the missing calls.
> > 
> > Hmm, dev_fwnode() however does not do that, which means that
> > iqs7222_parse_props() has different refounting, depending on what is
> > being fetched. I think we need to start there.
> 
> Right, but none of the callers that prompt iqs7222_parse_props() to
> use dev_fwnode() follow with fwnode_handle_put().

I think this is a problem that code has to be aware of that and behave
differently. I'd recommend bumping up refcount in dev_fwnode() path so
that all callers would behave uniformly. 

Thanks.
Jeff LaBundy Sept. 10, 2022, midnight UTC | #4 
Hi Dmitry,

On Thu, Sep 08, 2022 at 09:37:57PM -0700, Dmitry Torokhov wrote:
> On Thu, Sep 08, 2022 at 09:04:06PM -0500, Jeff LaBundy wrote:
> > Hi Dmitry,
> > 
> > Thank you for taking a look.
> > 
> > On Thu, Sep 08, 2022 at 02:17:21PM -0700, Dmitry Torokhov wrote:
> > > On Thu, Sep 08, 2022 at 08:15:38AM -0500, Jeff LaBundy wrote:
> > > > Each call to device/fwnode_get_named_child_node() must be matched
> > > > with a call to fwnode_handle_put() once the corresponding node is
> > > > no longer in use. This ensures a reference count remains balanced
> > > > in the case of dynamic device tree support.
> > > > 
> > > > Currently, the driver never calls fwnode_handle_put(). This patch
> > > > adds the missing calls.
> > > 
> > > Hmm, dev_fwnode() however does not do that, which means that
> > > iqs7222_parse_props() has different refounting, depending on what is
> > > being fetched. I think we need to start there.
> > 
> > Right, but none of the callers that prompt iqs7222_parse_props() to
> > use dev_fwnode() follow with fwnode_handle_put().
> 
> I think this is a problem that code has to be aware of that and behave
> differently. I'd recommend bumping up refcount in dev_fwnode() path so
> that all callers would behave uniformly. 

Agreed, right now the problem is that not all callers have a node to
put. So, I think the solution is to more thoughtfully encapsulate all
of this such that the caller is always responsible for passing a node,
iqs7222_parse_props() bumps the refcount of whatever it is fetching,
and the caller is always responsible for dropping the node.

This way, callers of iqs7222_parse_props() need not be burdened with
what it chooses to do under the hood.

> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy
diff mbox series

Patch

diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
index b2e8097a2e6d..04c1050d845c 100644
--- a/drivers/input/misc/iqs7222.c
+++ b/drivers/input/misc/iqs7222.c
@@ -1703,7 +1703,7 @@  static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
 				    IQS7222_REG_GRP_CYCLE,
 				    IQS7222_REG_KEY_NONE);
 	if (error)
-		return error;
+		goto put_cycle_node;
 
 	if (!cycle_node)
 		return 0;
@@ -1714,17 +1714,19 @@  static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
 	 * CTx pins (CTx0-8).
 	 */
 	if (!fwnode_property_present(cycle_node, "azoteq,tx-enable"))
-		return 0;
+		goto put_cycle_node;
 
 	count = fwnode_property_count_u32(cycle_node, "azoteq,tx-enable");
 	if (count < 0) {
 		dev_err(&client->dev, "Failed to count %s CTx pins: %d\n",
 			fwnode_get_name(cycle_node), count);
-		return count;
+		error = count;
+		goto put_cycle_node;
 	} else if (count > ARRAY_SIZE(pins)) {
 		dev_err(&client->dev, "Invalid number of %s CTx pins\n",
 			fwnode_get_name(cycle_node));
-		return -EINVAL;
+		error = -EINVAL;
+		goto put_cycle_node;
 	}
 
 	error = fwnode_property_read_u32_array(cycle_node, "azoteq,tx-enable",
@@ -1732,7 +1734,7 @@  static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
 	if (error) {
 		dev_err(&client->dev, "Failed to read %s CTx pins: %d\n",
 			fwnode_get_name(cycle_node), error);
-		return error;
+		goto put_cycle_node;
 	}
 
 	cycle_setup[1] &= ~GENMASK(7 + ARRAY_SIZE(pins) - 1, 7);
@@ -1741,13 +1743,17 @@  static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
 		if (pins[i] > 8) {
 			dev_err(&client->dev, "Invalid %s CTx pin: %u\n",
 				fwnode_get_name(cycle_node), pins[i]);
-			return -EINVAL;
+			error = -EINVAL;
+			goto put_cycle_node;
 		}
 
 		cycle_setup[1] |= BIT(pins[i] + 7);
 	}
 
-	return 0;
+put_cycle_node:
+	fwnode_handle_put(cycle_node);
+
+	return error;
 }
 
 static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
@@ -1766,7 +1772,7 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 				    IQS7222_REG_GRP_CHAN,
 				    IQS7222_REG_KEY_NONE);
 	if (error)
-		return error;
+		goto put_chan_node;
 
 	if (!chan_node)
 		return 0;
@@ -1793,14 +1799,15 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 			dev_err(&client->dev,
 				"Failed to read %s reference channel: %d\n",
 				fwnode_get_name(chan_node), error);
-			return error;
+			goto put_chan_node;
 		}
 
 		if (val >= ext_chan) {
 			dev_err(&client->dev,
 				"Invalid %s reference channel: %u\n",
 				fwnode_get_name(chan_node), val);
-			return -EINVAL;
+			error = -EINVAL;
+			goto put_chan_node;
 		}
 
 		ref_setup = iqs7222->chan_setup[val];
@@ -1818,7 +1825,8 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 				dev_err(&client->dev,
 					"Invalid %s reference weight: %u\n",
 					fwnode_get_name(chan_node), val);
-				return -EINVAL;
+				error = -EINVAL;
+				goto put_chan_node;
 			}
 
 			chan_setup[5] = val;
@@ -1851,12 +1859,14 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 			dev_err(&client->dev,
 				"Failed to count %s CRx pins: %d\n",
 				fwnode_get_name(chan_node), count);
-			return count;
+			error = count;
+			goto put_chan_node;
 		} else if (count > ARRAY_SIZE(pins)) {
 			dev_err(&client->dev,
 				"Invalid number of %s CRx pins\n",
 				fwnode_get_name(chan_node));
-			return -EINVAL;
+			error = -EINVAL;
+			goto put_chan_node;
 		}
 
 		error = fwnode_property_read_u32_array(chan_node,
@@ -1866,7 +1876,7 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 			dev_err(&client->dev,
 				"Failed to read %s CRx pins: %d\n",
 				fwnode_get_name(chan_node), error);
-			return error;
+			goto put_chan_node;
 		}
 
 		chan_setup[0] &= ~GENMASK(4 + ARRAY_SIZE(pins) - 1, 4);
@@ -1878,7 +1888,8 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 				dev_err(&client->dev,
 					"Invalid %s CRx pin: %u\n",
 					fwnode_get_name(chan_node), pins[i]);
-				return -EINVAL;
+				error = -EINVAL;
+				goto put_chan_node;
 			}
 
 			chan_setup[0] |= BIT(pins[i] + 4 - min_crx);
@@ -1897,14 +1908,18 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 		error = iqs7222_parse_props(iqs7222, &event_node, chan_index,
 					    IQS7222_REG_GRP_BTN,
 					    iqs7222_kp_events[i].reg_key);
-		if (error)
-			return error;
+		if (error) {
+			fwnode_handle_put(event_node);
+			goto put_chan_node;
+		}
 
 		error = iqs7222_gpio_select(iqs7222, event_node,
 					    BIT(chan_index),
 					    dev_desc->touch_link - (i ? 0 : 2));
-		if (error)
-			return error;
+		if (error) {
+			fwnode_handle_put(event_node);
+			goto put_chan_node;
+		}
 
 		if (!fwnode_property_read_u32(event_node,
 					      "azoteq,timeout-press-ms",
@@ -1922,7 +1937,9 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 				dev_err(&client->dev,
 					"Invalid %s press timeout: %u\n",
 					fwnode_get_name(chan_node), val);
-				return -EINVAL;
+				error = -EINVAL;
+				fwnode_handle_put(event_node);
+				goto put_chan_node;
 			}
 
 			*setup &= ~(U8_MAX << i * 8);
@@ -1934,7 +1951,8 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 		if (error) {
 			dev_err(&client->dev, "Failed to read %s code: %d\n",
 				fwnode_get_name(chan_node), error);
-			return error;
+			fwnode_handle_put(event_node);
+			goto put_chan_node;
 		}
 
 		iqs7222->kp_code[chan_index][i] = val;
@@ -1948,19 +1966,24 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 				dev_err(&client->dev,
 					"Failed to read %s input type: %d\n",
 					fwnode_get_name(chan_node), error);
-				return error;
+				fwnode_handle_put(event_node);
+				goto put_chan_node;
 			}
 
 			if (val != EV_KEY && val != EV_SW) {
 				dev_err(&client->dev,
 					"Invalid %s input type: %u\n",
 					fwnode_get_name(chan_node), val);
-				return -EINVAL;
+				error = -EINVAL;
+				fwnode_handle_put(event_node);
+				goto put_chan_node;
 			}
 
 			iqs7222->kp_type[chan_index][i] = val;
 		}
 
+		fwnode_handle_put(event_node);
+
 		/*
 		 * Reference channels can opt out of event reporting by using
 		 * KEY_RESERVED in place of a true key or switch code.
@@ -1983,9 +2006,14 @@  static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 	 * The following call handles a special pair of properties that apply
 	 * to a channel node, but reside within the button (event) group.
 	 */
-	return iqs7222_parse_props(iqs7222, &chan_node, chan_index,
-				   IQS7222_REG_GRP_BTN,
-				   IQS7222_REG_KEY_DEBOUNCE);
+	error = iqs7222_parse_props(iqs7222, &chan_node, chan_index,
+				    IQS7222_REG_GRP_BTN,
+				    IQS7222_REG_KEY_DEBOUNCE);
+
+put_chan_node:
+	fwnode_handle_put(chan_node);
+
+	return error;
 }
 
 static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
@@ -2004,7 +2032,7 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 				    IQS7222_REG_GRP_SLDR,
 				    IQS7222_REG_KEY_NONE);
 	if (error)
-		return error;
+		goto put_sldr_node;
 
 	if (!sldr_node)
 		return 0;
@@ -2018,11 +2046,13 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 	if (count < 0) {
 		dev_err(&client->dev, "Failed to count %s channels: %d\n",
 			fwnode_get_name(sldr_node), count);
-		return count;
+		error = count;
+		goto put_sldr_node;
 	} else if (count < 3 || count > ARRAY_SIZE(chan_sel)) {
 		dev_err(&client->dev, "Invalid number of %s channels\n",
 			fwnode_get_name(sldr_node));
-		return -EINVAL;
+		error = -EINVAL;
+		goto put_sldr_node;
 	}
 
 	error = fwnode_property_read_u32_array(sldr_node,
@@ -2031,7 +2061,7 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 	if (error) {
 		dev_err(&client->dev, "Failed to read %s channels: %d\n",
 			fwnode_get_name(sldr_node), error);
-		return error;
+		goto put_sldr_node;
 	}
 
 	/*
@@ -2052,7 +2082,8 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 		if (chan_sel[i] >= ext_chan) {
 			dev_err(&client->dev, "Invalid %s channel: %u\n",
 				fwnode_get_name(sldr_node), chan_sel[i]);
-			return -EINVAL;
+			error = -EINVAL;
+			goto put_sldr_node;
 		}
 
 		/*
@@ -2071,7 +2102,8 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 		if (!val || val > dev_desc->sldr_res) {
 			dev_err(&client->dev, "Invalid %s size: %u\n",
 				fwnode_get_name(sldr_node), val);
-			return -EINVAL;
+			error = -EINVAL;
+			goto put_sldr_node;
 		}
 
 		if (reg_offset) {
@@ -2087,7 +2119,8 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 		if (val > (reg_offset ? U16_MAX : U8_MAX * 4)) {
 			dev_err(&client->dev, "Invalid %s top speed: %u\n",
 				fwnode_get_name(sldr_node), val);
-			return -EINVAL;
+			error = -EINVAL;
+			goto put_sldr_node;
 		}
 
 		if (reg_offset) {
@@ -2142,8 +2175,10 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 					    reg_offset ?
 					    IQS7222_REG_KEY_RESERVED :
 					    iqs7222_sl_events[i].reg_key);
-		if (error)
-			return error;
+		if (error) {
+			fwnode_handle_put(event_node);
+			goto put_sldr_node;
+		}
 
 		/*
 		 * The press/release event does not expose a direct GPIO link,
@@ -2155,8 +2190,10 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 					      : sldr_setup[3 + reg_offset],
 					    i ? 1568 + sldr_index * 30
 					      : sldr_setup[4 + reg_offset]);
-		if (error)
-			return error;
+		if (error) {
+			fwnode_handle_put(event_node);
+			goto put_sldr_node;
+		}
 
 		if (!reg_offset)
 			sldr_setup[9] |= iqs7222_sl_events[i].enable;
@@ -2166,12 +2203,15 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 		if (error) {
 			dev_err(&client->dev, "Failed to read %s code: %d\n",
 				fwnode_get_name(sldr_node), error);
-			return error;
+			fwnode_handle_put(event_node);
+			goto put_sldr_node;
 		}
 
 		iqs7222->sl_code[sldr_index][i] = val;
 		input_set_capability(iqs7222->keypad, EV_KEY, val);
 
+		fwnode_handle_put(event_node);
+
 		if (!dev_desc->event_offset)
 			continue;
 
@@ -2192,11 +2232,16 @@  static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 	 * The following call handles a special pair of properties that shift
 	 * to make room for a wheel enable control in the case of IQS7222C.
 	 */
-	return iqs7222_parse_props(iqs7222, &sldr_node, sldr_index,
-				   IQS7222_REG_GRP_SLDR,
-				   dev_desc->wheel_enable ?
-				   IQS7222_REG_KEY_WHEEL :
-				   IQS7222_REG_KEY_NO_WHEEL);
+	error = iqs7222_parse_props(iqs7222, &sldr_node, sldr_index,
+				    IQS7222_REG_GRP_SLDR,
+				    dev_desc->wheel_enable ?
+				    IQS7222_REG_KEY_WHEEL :
+				    IQS7222_REG_KEY_NO_WHEEL);
+
+put_sldr_node:
+	fwnode_handle_put(sldr_node);
+
+	return error;
 }
 
 static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
@@ -2232,6 +2277,9 @@  static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
 		error = iqs7222_parse_props(iqs7222, &gpio_node, i,
 					    IQS7222_REG_GRP_GPIO,
 					    IQS7222_REG_KEY_NONE);
+		if (gpio_node)
+			fwnode_handle_put(gpio_node);
+
 		if (error)
 			return error;