[PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

---

 drivers/input/rmi4/rmi_bus.c    | 65 +++++++++++++++--------------------------
 drivers/input/rmi4/rmi_driver.c | 11 ++-----
 2 files changed, 25 insertions(+), 51 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..1b9ad80 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c

@@ -30,23 +30,6 @@ static struct dentry *rmi_debugfs_root;
  * purpose. For example F11 is a 2D touch sensor while F01 is a generic
  * function present in every RMI device.
  */
-
-static void rmi_release_device(struct device *dev)
-{
-	struct rmi_device *rmi_dev = to_rmi_device(dev);
-	kfree(rmi_dev);
-}
-
-struct device_type rmi_device_type = {
-	.name		= "rmi_sensor",
-	.release	= rmi_release_device,
-};
-
-bool rmi_is_physical_device(struct device *dev)
-{
-	return dev->type == &rmi_device_type;
-}
-
 #ifdef CONFIG_RMI4_DEBUG
 
 static void rmi_physical_setup_debugfs(struct rmi_device *rmi_dev)

@@ -94,8 +77,7 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
 		return -EINVAL;
 	}
 
-	rmi_dev = devm_kzalloc(xport->dev,
-				sizeof(struct rmi_device), GFP_KERNEL);
+	rmi_dev = kzalloc(sizeof(struct rmi_device), GFP_KERNEL);
 	if (!rmi_dev)
 		return -ENOMEM;
 

@@ -112,8 +94,10 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
 	rmi_physical_setup_debugfs(rmi_dev);
 
 	error = device_register(&rmi_dev->dev);
-	if (error)
+	if (error) {
+		put_device(&rmi_dev->dev);
 		return error;
+	}
 
 	dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
 		pdata->sensor_name, dev_name(&rmi_dev->dev));

@@ -131,7 +115,6 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
 {
 	struct rmi_device *rmi_dev = xport->rmi_dev;
 
-	rmi_physical_teardown_debugfs(rmi_dev);
 	device_unregister(&rmi_dev->dev);
 }
 EXPORT_SYMBOL(rmi_unregister_transport_device);

@@ -139,21 +122,6 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
 
 /* Function specific stuff */
 
-static void rmi_release_function(struct device *dev)
-{
-	struct rmi_function *fn = to_rmi_function(dev);
-	kfree(fn);
-}
-
-struct device_type rmi_function_type = {
-	.name		= "rmi_function",
-	.release	= rmi_release_function,
-};
-
-bool rmi_is_function_device(struct device *dev)
-{
-	return dev->type == &rmi_function_type;
-}
 
 #ifdef CONFIG_RMI4_DEBUG
 

@@ -185,6 +153,23 @@ static void rmi_function_teardown_debugfs(struct rmi_function *fn)
 }
 
 #endif
+static void rmi_release_function(struct device *dev)
+{
+	struct rmi_function *fn = to_rmi_function(dev);
+	rmi_function_teardown_debugfs(fn);
+	kfree(fn->irq_mask);
+	kfree(fn);
+}
+
+struct device_type rmi_function_type = {
+	.name		= "rmi_function",
+	.release	= rmi_release_function,
+};
+
+bool rmi_is_function_device(struct device *dev)
+{
+	return dev->type == &rmi_function_type;
+}
 
 static int rmi_function_match(struct device *dev, struct device_driver *drv)
 {

@@ -240,21 +225,17 @@ int rmi_register_function(struct rmi_function *fn)
 		dev_err(&rmi_dev->dev,
 			"Failed device_register function device %s\n",
 			dev_name(&fn->dev));
-		goto error_exit;
+		put_device(&fn->dev);
+		return error;
 	}
 
 	dev_dbg(&rmi_dev->dev, "Registered F%02X.\n", fn->fd.function_number);
 
 	return 0;
-
-error_exit:
-	rmi_function_teardown_debugfs(fn);
-	return error;
 }
 
 void rmi_unregister_function(struct rmi_function *fn)
 {
-	rmi_function_teardown_debugfs(fn);
 	device_unregister(&fn->dev);
 }
 

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 34f19e9..43575a1 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c

@@ -674,8 +674,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 	if (!fn->irq_mask) {
 		dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
 			__func__, pdt->function_number);
-		error = -ENOMEM;
-		goto err_free_mem;
+		return -ENOMEM;
 	}
 
 	for (i = 0; i < fn->num_of_irqs; i++)

@@ -683,7 +682,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 
 	error = rmi_register_function(fn);
 	if (error)
-		goto err_free_irq_mask;
+		return error;
 
 	if (pdt->function_number == 0x01)
 		data->f01_container = fn;

@@ -691,12 +690,6 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 	list_add_tail(&fn->node, &data->function_list);
 
 	return RMI_SCAN_CONTINUE;
-
-err_free_irq_mask:
-	kfree(fn->irq_mask);
-err_free_mem:
-	kfree(fn);
-	return error;
 }
 
 #ifdef CONFIG_PM_SLEEP

On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

I'm not a huge fan of you taking my patches, re-formatting them and
sending them as your own.  More out of principle then actually caring
about ownership.  You at least cc'd me on this one....

[...]  

If you are going to do this, then you need to remove the other call to
free this mask in rmi_free_function_list().

+	kfree(fn);
+}

-Courtney

On 02/11/2014 05:59 PM, Courtney Cavin wrote:

On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:

quoted

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

I'm not a huge fan of you taking my patches, re-formatting them and
sending them as your own.  More out of principle then actually caring
about ownership.  You at least cc'd me on this one....

Sorry - no slight was intended at all!  I wasn't sure what the protocol 
was for picking up an idea from someone else's patch and building on 
that idea, so I just went with the CC.  I definitely prefer to attribute 
sources correctly - if you could clarify what should be done (beyond the 
CC) to acknowledge the author of the original patch, I'd appreciate it.

quoted

---

  drivers/input/rmi4/rmi_bus.c    | 65 +++++++++++++++--------------------------
  drivers/input/rmi4/rmi_driver.c | 11 ++-----
  2 files changed, 25 insertions(+), 51 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..1b9ad80 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c

[...]

quoted

@@ -185,6 +153,23 @@ static void rmi_function_teardown_debugfs(struct rmi_function *fn)
  }

  #endif
+static void rmi_release_function(struct device *dev)
+{
+	struct rmi_function *fn = to_rmi_function(dev);
+	rmi_function_teardown_debugfs(fn);
+	kfree(fn->irq_mask);

If you are going to do this, then you need to remove the other call to
free this mask in rmi_free_function_list().

Okidoki.

quoted

+	kfree(fn);
+}

-Courtney

On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:

On 02/11/2014 05:59 PM, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:

quoted

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

I'm not a huge fan of you taking my patches, re-formatting them and
sending them as your own.  More out of principle then actually caring
about ownership.  You at least cc'd me on this one....

Sorry - no slight was intended at all!  I wasn't sure what the protocol 
was for picking up an idea from someone else's patch and building on 
that idea, so I just went with the CC.  I definitely prefer to attribute 
sources correctly - if you could clarify what should be done (beyond the 
CC) to acknowledge the author of the original patch, I'd appreciate it.

Sure.  In short, follow Documentation/SubmittingPatches , esp. section
12) Sign your work.

Generally the patch should read something like the following:

 From: Original Author [off-list ref]

 *BLURB*

 Signed-off-by: Original Author [off-list ref]
 [additional.author@example.org: changed x and y]
 Signed-off-by: Additional Author [off-list ref]

Assuming the original author actually signed-off the patch in the first
place, of course.  The square bracket part is optional, but can be
helpful for reviewers.

I'm somewhat surprised that you are not aware of this procedure, as this
is how Dmitry has replied to some of your patches in the past.

-Courtney

On 02/11/2014 06:49 PM, Courtney Cavin wrote:

On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:

quoted

On 02/11/2014 05:59 PM, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:

quoted

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

I'm not a huge fan of you taking my patches, re-formatting them and
sending them as your own.  More out of principle then actually caring
about ownership.  You at least cc'd me on this one....

Sorry - no slight was intended at all!  I wasn't sure what the protocol
was for picking up an idea from someone else's patch and building on
that idea, so I just went with the CC.  I definitely prefer to attribute
sources correctly - if you could clarify what should be done (beyond the
CC) to acknowledge the author of the original patch, I'd appreciate it.

Sure.  In short, follow Documentation/SubmittingPatches , esp. section
12) Sign your work.

Generally the patch should read something like the following:

  From: Original Author [off-list ref]

  *BLURB*

  Signed-off-by: Original Author [off-list ref]
  [additional.author@example.org: changed x and y]
  Signed-off-by: Additional Author [off-list ref]

Assuming the original author actually signed-off the patch in the first
place, of course.  The square bracket part is optional, but can be
helpful for reviewers.

I'm somewhat surprised that you are not aware of this procedure, as this
is how Dmitry has replied to some of your patches in the past.'

Thanks very much.

I was actually aware of that, but thought the work was sufficiently 
different from your original patch that applying your Signed-off-by: to 
it wouldn't be appropriate (I dislike being signed off on things I don't 
necessarily agree with as much as lack of attribution).  I'll be less 
paranoid about that in the future.

On Wed, Feb 12, 2014 at 04:17:59AM +0100, Christopher Heiny wrote:

On 02/11/2014 06:49 PM, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:

quoted

On 02/11/2014 05:59 PM, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:

quoted

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

I'm not a huge fan of you taking my patches, re-formatting them and
sending them as your own.  More out of principle then actually caring
about ownership.  You at least cc'd me on this one....

Sorry - no slight was intended at all!  I wasn't sure what the protocol
was for picking up an idea from someone else's patch and building on
that idea, so I just went with the CC.  I definitely prefer to attribute
sources correctly - if you could clarify what should be done (beyond the
CC) to acknowledge the author of the original patch, I'd appreciate it.

Sure.  In short, follow Documentation/SubmittingPatches , esp. section
12) Sign your work.

Generally the patch should read something like the following:

  From: Original Author [off-list ref]

  *BLURB*

  Signed-off-by: Original Author [off-list ref]
  [additional.author@example.org: changed x and y]
  Signed-off-by: Additional Author [off-list ref]

Assuming the original author actually signed-off the patch in the first
place, of course.  The square bracket part is optional, but can be
helpful for reviewers.

I'm somewhat surprised that you are not aware of this procedure, as this
is how Dmitry has replied to some of your patches in the past.'

Thanks very much.

I was actually aware of that, but thought the work was sufficiently 
different from your original patch that applying your Signed-off-by: to 
it wouldn't be appropriate (I dislike being signed off on things I don't 
necessarily agree with as much as lack of attribution).  I'll be less 
paranoid about that in the future.

I don't see how they were different enough, when clearly these two
patches attempt to fix the same bugs, using the same methods with
slightly modified flow.  Perhaps the patches may be small enough
to be interpreted either way, but at the very least reported-by (since
this is a bug-fix) or suggested-by is more appropriate than Cc.  This is
a public list, so I'm sure someone will tell you when you are wrong, if
nothing else.

Along the same topic, I guess I should also mention that it is typically
frowned upon to takeover someone else's patches without giving them
due-time to fix any outstanding review comments.

In both of these cases, you neither asked for me to submit the patches
separately, outside of my DT-series, nor to make any specific changes.
I was under the impression that you were still participating in the
discussion for that series.

While it is apparent that we have differing views on how this particular
driver development should proceed, and we should definitely discuss
them, please do not think that I'm not willing to apply my patches
individually to what's in tree now.

My main concern here is that I cannot actually properly test this driver
without DT, non-gpio irq, and regulator support.  Likewise, pre-3.7 is
ancient, and would require back-porting hundreds of changes.

-Courtney

On Tue, Feb 11, 2014 at 08:54:53PM -0800, Courtney Cavin wrote:

On Wed, Feb 12, 2014 at 04:17:59AM +0100, Christopher Heiny wrote:

quoted

On 02/11/2014 06:49 PM, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:

quoted

On 02/11/2014 05:59 PM, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:

quoted

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

I'm not a huge fan of you taking my patches, re-formatting them and
sending them as your own.  More out of principle then actually caring
about ownership.  You at least cc'd me on this one....

Sorry - no slight was intended at all!  I wasn't sure what the protocol
was for picking up an idea from someone else's patch and building on
that idea, so I just went with the CC.  I definitely prefer to attribute
sources correctly - if you could clarify what should be done (beyond the
CC) to acknowledge the author of the original patch, I'd appreciate it.

Sure.  In short, follow Documentation/SubmittingPatches , esp. section
12) Sign your work.

Generally the patch should read something like the following:

  From: Original Author [off-list ref]

  *BLURB*

  Signed-off-by: Original Author [off-list ref]
  [additional.author@example.org: changed x and y]
  Signed-off-by: Additional Author [off-list ref]

Assuming the original author actually signed-off the patch in the first
place, of course.  The square bracket part is optional, but can be
helpful for reviewers.

I'm somewhat surprised that you are not aware of this procedure, as this
is how Dmitry has replied to some of your patches in the past.'

Thanks very much.

I was actually aware of that, but thought the work was sufficiently 
different from your original patch that applying your Signed-off-by: to 
it wouldn't be appropriate (I dislike being signed off on things I don't 
necessarily agree with as much as lack of attribution).  I'll be less 
paranoid about that in the future.

I don't see how they were different enough, when clearly these two
patches attempt to fix the same bugs, using the same methods with
slightly modified flow.  Perhaps the patches may be small enough
to be interpreted either way, but at the very least reported-by (since
this is a bug-fix) or suggested-by is more appropriate than Cc.  This is
a public list, so I'm sure someone will tell you when you are wrong, if
nothing else.

Along the same topic, I guess I should also mention that it is typically
frowned upon to takeover someone else's patches without giving them
due-time to fix any outstanding review comments.

In both of these cases, you neither asked for me to submit the patches
separately, outside of my DT-series, nor to make any specific changes.
I was under the impression that you were still participating in the
discussion for that series.

While it is apparent that we have differing views on how this particular
driver development should proceed, and we should definitely discuss
them, please do not think that I'm not willing to apply my patches
individually to what's in tree now.

My main concern here is that I cannot actually properly test this driver
without DT, non-gpio irq, and regulator support.  Likewise, pre-3.7 is
ancient, and would require back-porting hundreds of changes.

I can rebase to something more recent; I just did not want to cause
additional work for Chris. Once he finishes pushing his code I was going
to rebase anyway.

Thanks.

-- 
Dmitry

On Wed, Feb 12, 2014 at 07:43:42AM +0100, Dmitry Torokhov wrote:

On Tue, Feb 11, 2014 at 08:54:53PM -0800, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 04:17:59AM +0100, Christopher Heiny wrote:

quoted

On 02/11/2014 06:49 PM, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:

quoted

On 02/11/2014 05:59 PM, Courtney Cavin wrote:

quoted

On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:

quoted

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

I'm not a huge fan of you taking my patches, re-formatting them and
sending them as your own.  More out of principle then actually caring
about ownership.  You at least cc'd me on this one....

Sorry - no slight was intended at all!  I wasn't sure what the protocol
was for picking up an idea from someone else's patch and building on
that idea, so I just went with the CC.  I definitely prefer to attribute
sources correctly - if you could clarify what should be done (beyond the
CC) to acknowledge the author of the original patch, I'd appreciate it.

Sure.  In short, follow Documentation/SubmittingPatches , esp. section
12) Sign your work.

Generally the patch should read something like the following:

  From: Original Author [off-list ref]

  *BLURB*

  Signed-off-by: Original Author [off-list ref]
  [additional.author@example.org: changed x and y]
  Signed-off-by: Additional Author [off-list ref]

Assuming the original author actually signed-off the patch in the first
place, of course.  The square bracket part is optional, but can be
helpful for reviewers.

I'm somewhat surprised that you are not aware of this procedure, as this
is how Dmitry has replied to some of your patches in the past.'

Thanks very much.

I was actually aware of that, but thought the work was sufficiently 
different from your original patch that applying your Signed-off-by: to 
it wouldn't be appropriate (I dislike being signed off on things I don't 
necessarily agree with as much as lack of attribution).  I'll be less 
paranoid about that in the future.

I don't see how they were different enough, when clearly these two
patches attempt to fix the same bugs, using the same methods with
slightly modified flow.  Perhaps the patches may be small enough
to be interpreted either way, but at the very least reported-by (since
this is a bug-fix) or suggested-by is more appropriate than Cc.  This is
a public list, so I'm sure someone will tell you when you are wrong, if
nothing else.

Along the same topic, I guess I should also mention that it is typically
frowned upon to takeover someone else's patches without giving them
due-time to fix any outstanding review comments.

In both of these cases, you neither asked for me to submit the patches
separately, outside of my DT-series, nor to make any specific changes.
I was under the impression that you were still participating in the
discussion for that series.

While it is apparent that we have differing views on how this particular
driver development should proceed, and we should definitely discuss
them, please do not think that I'm not willing to apply my patches
individually to what's in tree now.

My main concern here is that I cannot actually properly test this driver
without DT, non-gpio irq, and regulator support.  Likewise, pre-3.7 is
ancient, and would require back-porting hundreds of changes.

I can rebase to something more recent; I just did not want to cause
additional work for Chris. Once he finishes pushing his code I was going
to rebase anyway.

That would be great! Thanks.

-Courtney

On Tue, Feb 11, 2014 at 03:13:30PM -0800, Christopher Heiny wrote:

This is wrong, rmi_release_function should finish cleaning up resources,
however unregistration part should happen much earlier. If someone takes
reference to the device in debugfs the device may never go away as noone
will kick the user out.

Please put the calls to rmi_function_teardown_debugfs() back where they
originally were.

Unless you create rmi_allocate_function() and do device initialization
there (so that device and kobject are fully initialized and proper ktype
is assigned so that ->release() is set up) you still need to free memory
here if failure happens before calling rmi_register_function().

Thanks.

-- 
Dmitry

On 02/11/2014 10:49 PM, Dmitry Torokhov wrote:

On Tue, Feb 11, 2014 at 03:13:30PM -0800, Christopher Heiny wrote:

quoted

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Signed-off-by: Christopher Heiny <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Linux Walleij <redacted>
Cc: David Herrmann <redacted>
Cc: Jiri Kosina <redacted>
Cc: Courtney Cavin <redacted>

---

  drivers/input/rmi4/rmi_bus.c    | 65 +++++++++++++++--------------------------
  drivers/input/rmi4/rmi_driver.c | 11 ++-----
  2 files changed, 25 insertions(+), 51 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..1b9ad80 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c

[snip]

quoted

@@ -185,6 +153,23 @@ static void rmi_function_teardown_debugfs(struct rmi_function *fn)
  }

  #endif
+static void rmi_release_function(struct device *dev)
+{
+	struct rmi_function *fn = to_rmi_function(dev);
+	rmi_function_teardown_debugfs(fn);

This is wrong, rmi_release_function should finish cleaning up resources,
however unregistration part should happen much earlier. If someone takes
reference to the device in debugfs the device may never go away as noone
will kick the user out.

Please put the calls to rmi_function_teardown_debugfs() back where they
originally were.

OK.

quoted

+	kfree(fn->irq_mask);
+	kfree(fn);
+}
+
+struct device_type rmi_function_type = {
+	.name		= "rmi_function",
+	.release	= rmi_release_function,
+};
+
+bool rmi_is_function_device(struct device *dev)
+{
+	return dev->type == &rmi_function_type;
+}

  static int rmi_function_match(struct device *dev, struct device_driver *drv)
  {

@@ -240,21 +225,17 @@ int rmi_register_function(struct rmi_function *fn)
  		dev_err(&rmi_dev->dev,
  			"Failed device_register function device %s\n",
  			dev_name(&fn->dev));
-		goto error_exit;
+		put_device(&fn->dev);
+		return error;
  	}

  	dev_dbg(&rmi_dev->dev, "Registered F%02X.\n", fn->fd.function_number);

  	return 0;
-
-error_exit:
-	rmi_function_teardown_debugfs(fn);
-	return error;
  }

  void rmi_unregister_function(struct rmi_function *fn)
  {
-	rmi_function_teardown_debugfs(fn);
  	device_unregister(&fn->dev);
  }

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 34f19e9..43575a1 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c

@@ -674,8 +674,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
  	if (!fn->irq_mask) {
  		dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
  			__func__, pdt->function_number);
-		error = -ENOMEM;
-		goto err_free_mem;
+		return -ENOMEM;
  	}

  	for (i = 0; i < fn->num_of_irqs; i++)

@@ -683,7 +682,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,

  	error = rmi_register_function(fn);
  	if (error)
-		goto err_free_irq_mask;
+		return error;

  	if (pdt->function_number == 0x01)
  		data->f01_container = fn;

@@ -691,12 +690,6 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
  	list_add_tail(&fn->node, &data->function_list);

  	return RMI_SCAN_CONTINUE;
-
-err_free_irq_mask:
-	kfree(fn->irq_mask);
-err_free_mem:
-	kfree(fn);
-	return error;

Unless you create rmi_allocate_function() and do device initialization
there (so that device and kobject are fully initialized and proper ktype
is assigned so that ->release() is set up) you still need to free memory
here if failure happens before calling rmi_register_function().

Hmmm.  If we adopt the idea of allocating the irq_mask as an
array in the rmi_function structure, like you did in your
rmi_f01.c patch for the interrupt_enable mask, then there's no
point of failure bewteen allocating the storage and the call to
rmi_register_function().  rmi_register_function() doesn't have a
failure point prior to calling device_register(), and the
put_device() call in there should be able clean things up, like
Courtney proposed.

So making that change, along with some others you and Courtney
have suggested in other emails, we get the patch below.

					Chris

--

Input: synaptics-rmi4 - Use put_device() and device_type.release() to 
free storage.

From: Christopher Heiny <redacted>

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Allocate irq_mask as part of struct rmi_function.

Delete unused rmi_driver_irq_get_mask() function.

Suggested-by: Courtney Cavin <redacted>
Signed-off-by: Christopher Heiny <redacted>

---

  drivers/input/rmi4/rmi_bus.c    | 17 ++++++++-------
  drivers/input/rmi4/rmi_bus.h    |  2 +-
  drivers/input/rmi4/rmi_driver.c | 46 
+++++------------------------------------
  3 files changed, 15 insertions(+), 50 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..6342b45 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c

@@ -34,6 +34,7 @@ static struct dentry *rmi_debugfs_root;
  static void rmi_release_device(struct device *dev)
  {
  	struct rmi_device *rmi_dev = to_rmi_device(dev);
+
  	kfree(rmi_dev);
  }

@@ -94,8 +95,7 @@ int rmi_register_transport_device(struct 

rmi_transport_dev *xport)
  		return -EINVAL;
  	}

-	rmi_dev = devm_kzalloc(xport->dev,
-				sizeof(struct rmi_device), GFP_KERNEL);
+	rmi_dev = kzalloc(sizeof(struct rmi_device), GFP_KERNEL);
  	if (!rmi_dev)
  		return -ENOMEM;

@@ -112,8 +112,10 @@ int rmi_register_transport_device(struct 

rmi_transport_dev *xport)
  	rmi_physical_setup_debugfs(rmi_dev);

  	error = device_register(&rmi_dev->dev);
-	if (error)
+	if (error) {
+		put_device(&rmi_dev->dev);
  		return error;
+	}

  	dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
  		pdata->sensor_name, dev_name(&rmi_dev->dev));

@@ -142,6 +144,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
  static void rmi_release_function(struct device *dev)
  {
  	struct rmi_function *fn = to_rmi_function(dev);
+
  	kfree(fn);
  }

@@ -240,16 +243,14 @@ int rmi_register_function(struct rmi_function *fn)
  		dev_err(&rmi_dev->dev,
  			"Failed device_register function device %s\n",
  			dev_name(&fn->dev));
-		goto error_exit;
+		rmi_function_teardown_debugfs(fn);
+		put_device(&fn->dev);
+		return error;
  	}

  	dev_dbg(&rmi_dev->dev, "Registered F%02X.\n", fn->fd.function_number);

  	return 0;
-
-error_exit:
-	rmi_function_teardown_debugfs(fn);
-	return error;
  }

  void rmi_unregister_function(struct rmi_function *fn)

diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index decb479..a544c9c 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h

@@ -47,13 +47,13 @@ struct rmi_function {
  	struct device dev;
  	int num_of_irqs;
  	int irq_pos;
-	unsigned long *irq_mask;
  	void *data;
  	struct list_head node;

  #ifdef CONFIG_RMI4_DEBUG
  	struct dentry *debugfs_root;
  #endif
+	unsigned long irq_mask[];
  };

  #define to_rmi_function(d)	container_of(d, struct rmi_function, dev)

diff --git a/drivers/input/rmi4/rmi_driver.c 

b/drivers/input/rmi4/rmi_driver.c
index 34f19e9..a6a1aea 100644

--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c

@@ -185,7 +185,6 @@ static void rmi_free_function_list(struct rmi_device 

*rmi_dev)
  	list_for_each_entry_safe_reverse(fn, tmp,
  					 &data->function_list, node) {
  		list_del(&fn->node);
-		kfree(fn->irq_mask);
  		rmi_unregister_function(fn);
  	}
  }

@@ -456,28 +455,6 @@ static int rmi_driver_reset_handler(struct 

rmi_device *rmi_dev)
  	return 0;
  }

-/*
- * Construct a function's IRQ mask. This should be called once and stored.
- */
-int rmi_driver_irq_get_mask(struct rmi_device *rmi_dev,
-			   struct rmi_function *fn)
-{
-	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
-	int i;
-
-	/* call devm_kcalloc when it will be defined in kernel in future */
-	fn->irq_mask = devm_kzalloc(&rmi_dev->dev,
-			BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
-			GFP_KERNEL);
-
-	if (fn->irq_mask) {
-		for (i = 0; i < fn->num_of_irqs; i++)
-			set_bit(fn->irq_pos+i, fn->irq_mask);
-		return 0;
-	} else
-		return -ENOMEM;
-}
-
  int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry 
*entry,
  			u16 pdt_address)
  {

@@ -652,7 +629,10 @@ static int rmi_create_function(struct rmi_device 

*rmi_dev,
  	dev_dbg(dev, "Initializing F%02X for %s.\n",
  		pdt->function_number, pdata->sensor_name);

-	fn = kzalloc(sizeof(struct rmi_function), GFP_KERNEL);
+	fn = kzalloc(sizeof(struct rmi_function) +
+			(BITS_TO_LONGS(data->irq_count) *
+				sizeof(unsigned long)),
+			GFP_KERNEL);
  	if (!fn) {
  		dev_err(dev, "Failed to allocate memory for F%02X\n",
  			pdt->function_number);

@@ -668,22 +648,12 @@ static int rmi_create_function(struct rmi_device 

*rmi_dev,
  	fn->irq_pos = *current_irq_count;
  	*current_irq_count += fn->num_of_irqs;

-	fn->irq_mask = kzalloc(
-		BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
-		GFP_KERNEL);
-	if (!fn->irq_mask) {
-		dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
-			__func__, pdt->function_number);
-		error = -ENOMEM;
-		goto err_free_mem;
-	}
-
  	for (i = 0; i < fn->num_of_irqs; i++)
  		set_bit(fn->irq_pos + i, fn->irq_mask);

  	error = rmi_register_function(fn);
  	if (error)
-		goto err_free_irq_mask;
+		return error;

  	if (pdt->function_number == 0x01)
  		data->f01_container = fn;

@@ -691,12 +661,6 @@ static int rmi_create_function(struct rmi_device 

*rmi_dev,
  	list_add_tail(&fn->node, &data->function_list);

  	return RMI_SCAN_CONTINUE;
-
-err_free_irq_mask:
-	kfree(fn->irq_mask);
-err_free_mem:
-	kfree(fn);
-	return error;
  }

  #ifdef CONFIG_PM_SLEEP

On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote:

We also need to tear down debugfs here.

This is not proper place to free structures since
rmi_register_function() was not the one that allocated them.

OK, I split the rmi_driver_irq_get_mask() removal into a separate patch
and applied it, this leaves us with the patch below (BTW, your mailed
damaged - line wrapped - the patch so I had to reconstruct it).
I switched from using device_register/device_unregister to
device_initialize/device_add/device_del/put_device as it allows better
control over error unwinding and destroying the objects which is
important if you want to delete debugfs entries once all fucntion
handler have been unregister but before our memory is gone.

Thanks.

-- 
Dmitry

Input: synaptics-rmi4 - use put_device() to free devices

From: Christopher Heiny <redacted>

For rmi_sensor and rmi_function device_types, use put_device() and
the associated device_type->release() function to clean up related
structures and storage in the correct and safe order.

Allocate irq_mask as part of struct rmi_function.

Suggested-by: Courtney Cavin <redacted>
Signed-off-by: Christopher Heiny <redacted>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_bus.c    |   30 +++++++++++++++++++++---------
 drivers/input/rmi4/rmi_bus.h    |    7 ++++---
 drivers/input/rmi4/rmi_driver.c |   25 +++++++------------------
 3 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 96a76e7..7efe7ed 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c

@@ -34,6 +34,7 @@ static struct dentry *rmi_debugfs_root;
 static void rmi_release_device(struct device *dev)
 {
 	struct rmi_device *rmi_dev = to_rmi_device(dev);
+
 	kfree(rmi_dev);
 }
 

@@ -94,11 +95,12 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
 		return -EINVAL;
 	}
 
-	rmi_dev = devm_kzalloc(xport->dev,
-				sizeof(struct rmi_device), GFP_KERNEL);
+	rmi_dev = kzalloc(sizeof(struct rmi_device), GFP_KERNEL);
 	if (!rmi_dev)
 		return -ENOMEM;
 
+	device_initialize(&rmi_dev->dev);
+
 	rmi_dev->xport = xport;
 	rmi_dev->number = atomic_inc_return(&transport_device_count) - 1;
 

@@ -111,14 +113,19 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
 
 	rmi_physical_setup_debugfs(rmi_dev);
 
-	error = device_register(&rmi_dev->dev);
+	error = device_add(&rmi_dev->dev);
 	if (error)
-		return error;
+		goto err_put_device;
 
 	dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
 		pdata->sensor_name, dev_name(&rmi_dev->dev));
 
 	return 0;
+
+err_put_device:
+	rmi_physical_teardown_debugfs(rmi_dev);
+	put_device(&rmi_dev->dev);
+	return error;
 }
 EXPORT_SYMBOL_GPL(rmi_register_transport_device);
 

@@ -131,8 +138,9 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
 {
 	struct rmi_device *rmi_dev = xport->rmi_dev;
 
+	device_del(&rmi_dev->dev);
 	rmi_physical_teardown_debugfs(rmi_dev);
-	device_unregister(&rmi_dev->dev);
+	put_device(&rmi_dev->dev);
 }
 EXPORT_SYMBOL(rmi_unregister_transport_device);
 

@@ -142,6 +150,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
 static void rmi_release_function(struct device *dev)
 {
 	struct rmi_function *fn = to_rmi_function(dev);
+
 	kfree(fn);
 }
 

@@ -226,6 +235,8 @@ int rmi_register_function(struct rmi_function *fn)
 	struct rmi_device *rmi_dev = fn->rmi_dev;
 	int error;
 
+	device_initialize(&fn->dev);
+
 	dev_set_name(&fn->dev, "%s.fn%02x",
 		     dev_name(&rmi_dev->dev), fn->fd.function_number);
 

@@ -235,27 +246,28 @@ int rmi_register_function(struct rmi_function *fn)
 
 	rmi_function_setup_debugfs(fn);
 
-	error = device_register(&fn->dev);
+	error = device_add(&fn->dev);
 	if (error) {
 		dev_err(&rmi_dev->dev,
 			"Failed device_register function device %s\n",
 			dev_name(&fn->dev));
-		goto error_exit;
+		goto err_teardown_debugfs;
 	}
 
 	dev_dbg(&rmi_dev->dev, "Registered F%02X.\n", fn->fd.function_number);
 
 	return 0;
 
-error_exit:
+err_teardown_debugfs:
 	rmi_function_teardown_debugfs(fn);
 	return error;
 }
 
 void rmi_unregister_function(struct rmi_function *fn)
 {
+	device_del(&fn->dev);
 	rmi_function_teardown_debugfs(fn);
-	device_unregister(&fn->dev);
+	put_device(&fn->dev);
 }
 
 /**

diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 5ad94c6..1672b05 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h

@@ -45,14 +45,15 @@ struct rmi_function {
 	struct rmi_function_descriptor fd;
 	struct rmi_device *rmi_dev;
 	struct device dev;
-	int num_of_irqs;
-	int irq_pos;
-	unsigned long *irq_mask;
 	struct list_head node;
 
 #ifdef CONFIG_RMI4_DEBUG
 	struct dentry *debugfs_root;
 #endif
+
+	unsigned int num_of_irqs;
+	unsigned int irq_pos;
+	unsigned long irq_mask[];
 };
 
 #define to_rmi_function(d)	container_of(d, struct rmi_function, dev)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 473efbc..788343a 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c

@@ -185,7 +185,6 @@ static void rmi_free_function_list(struct rmi_device *rmi_dev)
 	list_for_each_entry_safe_reverse(fn, tmp,
 					 &data->function_list, node) {
 		list_del(&fn->node);
-		kfree(fn->irq_mask);
 		rmi_unregister_function(fn);
 	}
 }

@@ -617,7 +616,7 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
 }
 
 static int rmi_create_function(struct rmi_device *rmi_dev,
-			      void *ctx, const struct pdt_entry *pdt)
+			       void *ctx, const struct pdt_entry *pdt)
 {
 	struct device *dev = &rmi_dev->dev;
 	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);

@@ -630,7 +629,9 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 	dev_dbg(dev, "Initializing F%02X for %s.\n",
 		pdt->function_number, pdata->sensor_name);
 
-	fn = kzalloc(sizeof(struct rmi_function), GFP_KERNEL);
+	fn = kzalloc(sizeof(struct rmi_function) +
+			BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
+		     GFP_KERNEL);
 	if (!fn) {
 		dev_err(dev, "Failed to allocate memory for F%02X\n",
 			pdt->function_number);

@@ -646,22 +647,12 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 	fn->irq_pos = *current_irq_count;
 	*current_irq_count += fn->num_of_irqs;
 
-	fn->irq_mask = kzalloc(
-		BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
-		GFP_KERNEL);
-	if (!fn->irq_mask) {
-		dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
-			__func__, pdt->function_number);
-		error = -ENOMEM;
-		goto err_free_mem;
-	}
-
 	for (i = 0; i < fn->num_of_irqs; i++)
 		set_bit(fn->irq_pos + i, fn->irq_mask);
 
 	error = rmi_register_function(fn);
 	if (error)
-		goto err_free_irq_mask;
+		goto err_put_fn;
 
 	if (pdt->function_number == 0x01)
 		data->f01_container = fn;

@@ -670,10 +661,8 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 
 	return RMI_SCAN_CONTINUE;
 
-err_free_irq_mask:
-	kfree(fn->irq_mask);
-err_free_mem:
-	kfree(fn);
+err_put_fn:
+	put_device(&fn->dev);
 	return error;
 }
 

On Thu, Feb 13, 2014 at 07:15:24AM +0100, Dmitry Torokhov wrote:

On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote:

quoted

Input: synaptics-rmi4 - Use put_device() and device_type.release()
to free storage.

From: Christopher Heiny <redacted>

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Allocate irq_mask as part of struct rmi_function.

Delete unused rmi_driver_irq_get_mask() function.

[...]

[...]  

Ownership of this memory is a bit weird...

[...]

 void rmi_unregister_function(struct rmi_function *fn)
 {
+	device_del(&fn->dev);
 	rmi_function_teardown_debugfs(fn);
-	device_unregister(&fn->dev);
+	put_device(&fn->dev);
 }

Here clearly the bus code owns it...

[...]  

But it's allocated in the chip driver...

And the chip driver now is expected to know it's a device, and trust
that the bus code knows how to free the memory.

 	return error;
 }
 

As this clearly fixes a bug or two, I say we should take this patch
as-is and worry about proper ownership at some other time.

-Courtney

On Thu, Feb 13, 2014 at 01:59:31PM -0800, Courtney Cavin wrote:

On Thu, Feb 13, 2014 at 07:15:24AM +0100, Dmitry Torokhov wrote:

quoted

On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote:

quoted

Input: synaptics-rmi4 - Use put_device() and device_type.release()
to free storage.

From: Christopher Heiny <redacted>

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Allocate irq_mask as part of struct rmi_function.

Delete unused rmi_driver_irq_get_mask() function.

[...]

quoted

Input: synaptics-rmi4 - use put_device() to free devices

From: Christopher Heiny <redacted>

For rmi_sensor and rmi_function device_types, use put_device() and
the associated device_type->release() function to clean up related
structures and storage in the correct and safe order.

Allocate irq_mask as part of struct rmi_function.

Suggested-by: Courtney Cavin <redacted>
Signed-off-by: Christopher Heiny <redacted>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_bus.c    |   30 +++++++++++++++++++++---------
 drivers/input/rmi4/rmi_bus.h    |    7 ++++---
 drivers/input/rmi4/rmi_driver.c |   25 +++++++------------------
 3 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c

[...]  

quoted

@@ -142,6 +150,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
 static void rmi_release_function(struct device *dev)
 {
 	struct rmi_function *fn = to_rmi_function(dev);
+
 	kfree(fn);
 }

Ownership of this memory is a bit weird...

[...]

quoted

 void rmi_unregister_function(struct rmi_function *fn)
 {
+	device_del(&fn->dev);
 	rmi_function_teardown_debugfs(fn);
-	device_unregister(&fn->dev);
+	put_device(&fn->dev);
 }

Here clearly the bus code owns it...

quoted

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c

[...]  

quoted

 static int rmi_create_function(struct rmi_device *rmi_dev,
-			      void *ctx, const struct pdt_entry *pdt)
+			       void *ctx, const struct pdt_entry *pdt)
 {
 	struct device *dev = &rmi_dev->dev;
 	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);

@@ -630,7 +629,9 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 	dev_dbg(dev, "Initializing F%02X for %s.\n",
 		pdt->function_number, pdata->sensor_name);
 
-	fn = kzalloc(sizeof(struct rmi_function), GFP_KERNEL);
+	fn = kzalloc(sizeof(struct rmi_function) +
+			BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
+		     GFP_KERNEL);

But it's allocated in the chip driver...

quoted

 	if (!fn) {
 		dev_err(dev, "Failed to allocate memory for F%02X\n",
 			pdt->function_number);

@@ -646,22 +647,12 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 	fn->irq_pos = *current_irq_count;
 	*current_irq_count += fn->num_of_irqs;
 
-	fn->irq_mask = kzalloc(
-		BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
-		GFP_KERNEL);
-	if (!fn->irq_mask) {
-		dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
-			__func__, pdt->function_number);
-		error = -ENOMEM;
-		goto err_free_mem;
-	}
-
 	for (i = 0; i < fn->num_of_irqs; i++)
 		set_bit(fn->irq_pos + i, fn->irq_mask);
 
 	error = rmi_register_function(fn);
 	if (error)
-		goto err_free_irq_mask;
+		goto err_put_fn;
 
 	if (pdt->function_number == 0x01)
 		data->f01_container = fn;

@@ -670,10 +661,8 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 
 	return RMI_SCAN_CONTINUE;
 
-err_free_irq_mask:
-	kfree(fn->irq_mask);
-err_free_mem:
-	kfree(fn);
+err_put_fn:
+	put_device(&fn->dev);

And the chip driver now is expected to know it's a device, and trust
that the bus code knows how to free the memory.

Yeah. That is why for input devices I have a separate
input_allocate_device and input_free_device... But given that RMI is
pretty-much self-contained I think we can live with this.

As this clearly fixes a bug or two, I say we should take this patch
as-is and worry about proper ownership at some other time.

-Courtney

-- 
Dmitry

Sorry for top posting - using web mail right now.

I think something like allocate_device and free_device will be needed sooner rather than later. I'll get a patch out for that in the next couple of days.

Chris

________________________________________
From: linux-input-owner@vger.kernel.org [linux-input-owner@vger.kernel.org] on behalf of Dmitry Torokhov [dmitry.torokhov@gmail.com]
Sent: Thursday, February 13, 2014 2:10 PM
To: Courtney Cavin
Cc: Christopher Heiny; Linux Input; Andrew Duggan; Vincent Huang; Vivian Ly; Daniel Rosenberg; Jean Delvare; Joerie de Gram; Linus Walleij; Benjamin Tissoires; David Herrmann; Jiri Kosina
Subject: Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.

On Thu, Feb 13, 2014 at 01:59:31PM -0800, Courtney Cavin wrote:

On Thu, Feb 13, 2014 at 07:15:24AM +0100, Dmitry Torokhov wrote:

quoted

On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote:

quoted

Input: synaptics-rmi4 - Use put_device() and device_type.release()
to free storage.

From: Christopher Heiny <redacted>

For rmi_sensor and rmi_function device_types, use put_device() and
the assocated device_type.release() function to clean up related
structures and storage in the correct and safe order.

Allocate irq_mask as part of struct rmi_function.

Delete unused rmi_driver_irq_get_mask() function.

[...]

quoted

Input: synaptics-rmi4 - use put_device() to free devices

From: Christopher Heiny <redacted>

For rmi_sensor and rmi_function device_types, use put_device() and
the associated device_type->release() function to clean up related
structures and storage in the correct and safe order.

Allocate irq_mask as part of struct rmi_function.

Suggested-by: Courtney Cavin <redacted>
Signed-off-by: Christopher Heiny <redacted>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_bus.c    |   30 +++++++++++++++++++++---------
 drivers/input/rmi4/rmi_bus.h    |    7 ++++---
 drivers/input/rmi4/rmi_driver.c |   25 +++++++------------------
 3 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c

[...]

quoted

@@ -142,6 +150,7 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
 static void rmi_release_function(struct device *dev)
 {
    struct rmi_function *fn = to_rmi_function(dev);
+
    kfree(fn);
 }

Ownership of this memory is a bit weird...

[...]

quoted

 void rmi_unregister_function(struct rmi_function *fn)
 {
+   device_del(&fn->dev);
    rmi_function_teardown_debugfs(fn);
-   device_unregister(&fn->dev);
+   put_device(&fn->dev);
 }

Here clearly the bus code owns it...

quoted

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c

[...]

quoted

 static int rmi_create_function(struct rmi_device *rmi_dev,
-                         void *ctx, const struct pdt_entry *pdt)
+                          void *ctx, const struct pdt_entry *pdt)
 {
    struct device *dev = &rmi_dev->dev;
    struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);

@@ -630,7 +629,9 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
    dev_dbg(dev, "Initializing F%02X for %s.\n",
            pdt->function_number, pdata->sensor_name);

-   fn = kzalloc(sizeof(struct rmi_function), GFP_KERNEL);
+   fn = kzalloc(sizeof(struct rmi_function) +
+                   BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
+                GFP_KERNEL);

But it's allocated in the chip driver...

quoted

    if (!fn) {
            dev_err(dev, "Failed to allocate memory for F%02X\n",
                    pdt->function_number);

@@ -646,22 +647,12 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
    fn->irq_pos = *current_irq_count;
    *current_irq_count += fn->num_of_irqs;

-   fn->irq_mask = kzalloc(
-           BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
-           GFP_KERNEL);
-   if (!fn->irq_mask) {
-           dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
-                   __func__, pdt->function_number);
-           error = -ENOMEM;
-           goto err_free_mem;
-   }
-
    for (i = 0; i < fn->num_of_irqs; i++)
            set_bit(fn->irq_pos + i, fn->irq_mask);

    error = rmi_register_function(fn);
    if (error)
-           goto err_free_irq_mask;
+           goto err_put_fn;

    if (pdt->function_number == 0x01)
            data->f01_container = fn;

@@ -670,10 +661,8 @@ static int rmi_create_function(struct rmi_device *rmi_dev,

    return RMI_SCAN_CONTINUE;

-err_free_irq_mask:
-   kfree(fn->irq_mask);
-err_free_mem:
-   kfree(fn);
+err_put_fn:
+   put_device(&fn->dev);

And the chip driver now is expected to know it's a device, and trust
that the bus code knows how to free the memory.

Yeah. That is why for input devices I have a separate
input_allocate_device and input_free_device... But given that RMI is
pretty-much self-contained I think we can live with this.

As this clearly fixes a bug or two, I say we should take this patch
as-is and worry about proper ownership at some other time.

-Courtney

--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html