* [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. @ 2014-02-11 23:13 Christopher Heiny 2014-02-12 1:59 ` Courtney Cavin 2014-02-12 6:49 ` Dmitry Torokhov 0 siblings, 2 replies; 14+ messages in thread From: Christopher Heiny @ 2014-02-11 23:13 UTC (permalink / raw) To: Dmitry Torokhov Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina, Courtney Cavin 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 <cheiny@synaptics.com> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: Linux Walleij <linus.walleij@linaro.org> Cc: David Herrmann <dh.herrmann@gmail.com> Cc: Jiri Kosina <jkosina@suse.cz> Cc: Courtney Cavin <courtney.cavin@sonymobile.com> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-11 23:13 [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage Christopher Heiny @ 2014-02-12 1:59 ` Courtney Cavin 2014-02-12 2:17 ` Christopher Heiny 2014-02-12 6:49 ` Dmitry Torokhov 1 sibling, 1 reply; 14+ messages in thread From: Courtney Cavin @ 2014-02-12 1:59 UTC (permalink / raw) To: Christopher Heiny Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina 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 <cheiny@synaptics.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: Linux Walleij <linus.walleij@linaro.org> > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Jiri Kosina <jkosina@suse.cz> > Cc: Courtney Cavin <courtney.cavin@sonymobile.com> 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.... > > --- > > 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 [...] > @@ -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(). > + kfree(fn); > +} -Courtney ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-12 1:59 ` Courtney Cavin @ 2014-02-12 2:17 ` Christopher Heiny 2014-02-12 2:49 ` Courtney Cavin 0 siblings, 1 reply; 14+ messages in thread From: Christopher Heiny @ 2014-02-12 2:17 UTC (permalink / raw) To: Courtney Cavin Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina On 02/11/2014 05:59 PM, Courtney Cavin wrote: > 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 <cheiny@synaptics.com> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> >> Cc: Linux Walleij <linus.walleij@linaro.org> >> Cc: David Herrmann <dh.herrmann@gmail.com> >> Cc: Jiri Kosina <jkosina@suse.cz> >> Cc: Courtney Cavin <courtney.cavin@sonymobile.com> > > 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. > >> >> --- >> >> 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 > [...] >> @@ -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. > >> + kfree(fn); >> +} > > -Courtney ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-12 2:17 ` Christopher Heiny @ 2014-02-12 2:49 ` Courtney Cavin 2014-02-12 3:17 ` Christopher Heiny 0 siblings, 1 reply; 14+ messages in thread From: Courtney Cavin @ 2014-02-12 2:49 UTC (permalink / raw) To: Christopher Heiny Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote: > On 02/11/2014 05:59 PM, Courtney Cavin wrote: > > 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 <cheiny@synaptics.com> > >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > >> Cc: Linux Walleij <linus.walleij@linaro.org> > >> Cc: David Herrmann <dh.herrmann@gmail.com> > >> Cc: Jiri Kosina <jkosina@suse.cz> > >> Cc: Courtney Cavin <courtney.cavin@sonymobile.com> > > > > 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 <original.author@example.org> *BLURB* Signed-off-by: Original Author <original.author@example.org> [additional.author@example.org: changed x and y] Signed-off-by: Additional Author <additional.author@example.org> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-12 2:49 ` Courtney Cavin @ 2014-02-12 3:17 ` Christopher Heiny 2014-02-12 4:54 ` Courtney Cavin 0 siblings, 1 reply; 14+ messages in thread From: Christopher Heiny @ 2014-02-12 3:17 UTC (permalink / raw) To: Courtney Cavin Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina On 02/11/2014 06:49 PM, Courtney Cavin wrote: > On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote: >> On 02/11/2014 05:59 PM, Courtney Cavin wrote: >>> 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 <cheiny@synaptics.com> >>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> >>>> Cc: Linux Walleij <linus.walleij@linaro.org> >>>> Cc: David Herrmann <dh.herrmann@gmail.com> >>>> Cc: Jiri Kosina <jkosina@suse.cz> >>>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com> >>> >>> 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 <original.author@example.org> > > *BLURB* > > Signed-off-by: Original Author <original.author@example.org> > [additional.author@example.org: changed x and y] > Signed-off-by: Additional Author <additional.author@example.org> > > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-12 3:17 ` Christopher Heiny @ 2014-02-12 4:54 ` Courtney Cavin 2014-02-12 6:43 ` Dmitry Torokhov 0 siblings, 1 reply; 14+ messages in thread From: Courtney Cavin @ 2014-02-12 4:54 UTC (permalink / raw) To: Christopher Heiny Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina On Wed, Feb 12, 2014 at 04:17:59AM +0100, Christopher Heiny wrote: > On 02/11/2014 06:49 PM, Courtney Cavin wrote: > > On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote: > >> On 02/11/2014 05:59 PM, Courtney Cavin wrote: > >>> 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 <cheiny@synaptics.com> > >>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > >>>> Cc: Linux Walleij <linus.walleij@linaro.org> > >>>> Cc: David Herrmann <dh.herrmann@gmail.com> > >>>> Cc: Jiri Kosina <jkosina@suse.cz> > >>>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com> > >>> > >>> 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 <original.author@example.org> > > > > *BLURB* > > > > Signed-off-by: Original Author <original.author@example.org> > > [additional.author@example.org: changed x and y] > > Signed-off-by: Additional Author <additional.author@example.org> > > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-12 4:54 ` Courtney Cavin @ 2014-02-12 6:43 ` Dmitry Torokhov 2014-02-12 17:09 ` Courtney Cavin 0 siblings, 1 reply; 14+ messages in thread From: Dmitry Torokhov @ 2014-02-12 6:43 UTC (permalink / raw) 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 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: > > On 02/11/2014 06:49 PM, Courtney Cavin wrote: > > > On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote: > > >> On 02/11/2014 05:59 PM, Courtney Cavin wrote: > > >>> 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 <cheiny@synaptics.com> > > >>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > >>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > >>>> Cc: Linux Walleij <linus.walleij@linaro.org> > > >>>> Cc: David Herrmann <dh.herrmann@gmail.com> > > >>>> Cc: Jiri Kosina <jkosina@suse.cz> > > >>>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com> > > >>> > > >>> 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 <original.author@example.org> > > > > > > *BLURB* > > > > > > Signed-off-by: Original Author <original.author@example.org> > > > [additional.author@example.org: changed x and y] > > > Signed-off-by: Additional Author <additional.author@example.org> > > > > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-12 6:43 ` Dmitry Torokhov @ 2014-02-12 17:09 ` Courtney Cavin 0 siblings, 0 replies; 14+ messages in thread From: Courtney Cavin @ 2014-02-12 17:09 UTC (permalink / raw) To: Dmitry Torokhov 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 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: > > On Wed, Feb 12, 2014 at 04:17:59AM +0100, Christopher Heiny wrote: > > > On 02/11/2014 06:49 PM, Courtney Cavin wrote: > > > > On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote: > > > >> On 02/11/2014 05:59 PM, Courtney Cavin wrote: > > > >>> 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 <cheiny@synaptics.com> > > > >>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > >>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > >>>> Cc: Linux Walleij <linus.walleij@linaro.org> > > > >>>> Cc: David Herrmann <dh.herrmann@gmail.com> > > > >>>> Cc: Jiri Kosina <jkosina@suse.cz> > > > >>>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com> > > > >>> > > > >>> 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 <original.author@example.org> > > > > > > > > *BLURB* > > > > > > > > Signed-off-by: Original Author <original.author@example.org> > > > > [additional.author@example.org: changed x and y] > > > > Signed-off-by: Additional Author <additional.author@example.org> > > > > > > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-11 23:13 [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage Christopher Heiny 2014-02-12 1:59 ` Courtney Cavin @ 2014-02-12 6:49 ` Dmitry Torokhov 2014-02-13 2:31 ` Christopher Heiny 1 sibling, 1 reply; 14+ messages in thread From: Dmitry Torokhov @ 2014-02-12 6:49 UTC (permalink / raw) To: Christopher Heiny Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina, Courtney Cavin On Tue, Feb 11, 2014 at 03:13:30PM -0800, 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 <cheiny@synaptics.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: Linux Walleij <linus.walleij@linaro.org> > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Jiri Kosina <jkosina@suse.cz> > Cc: Courtney Cavin <courtney.cavin@sonymobile.com> > > --- > > 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); 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. > + 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(). Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-12 6:49 ` Dmitry Torokhov @ 2014-02-13 2:31 ` Christopher Heiny 2014-02-13 6:15 ` Dmitry Torokhov 0 siblings, 1 reply; 14+ messages in thread From: Christopher Heiny @ 2014-02-13 2:31 UTC (permalink / raw) To: Dmitry Torokhov Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina, Courtney Cavin On 02/11/2014 10:49 PM, Dmitry Torokhov wrote: > On Tue, Feb 11, 2014 at 03:13:30PM -0800, 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 <cheiny@synaptics.com> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> >> Cc: Linux Walleij <linus.walleij@linaro.org> >> Cc: David Herrmann <dh.herrmann@gmail.com> >> Cc: Jiri Kosina <jkosina@suse.cz> >> Cc: Courtney Cavin <courtney.cavin@sonymobile.com> >> >> --- >> >> 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] >> @@ -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. > >> + 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 <cheiny@synaptics.com> 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 <courtney.cavin@sonymobile.com> Signed-off-by: Christopher Heiny <cheiny@synaptics.com> --- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-13 2:31 ` Christopher Heiny @ 2014-02-13 6:15 ` Dmitry Torokhov 2014-02-13 21:59 ` Courtney Cavin 0 siblings, 1 reply; 14+ messages in thread From: Dmitry Torokhov @ 2014-02-13 6:15 UTC (permalink / raw) To: Christopher Heiny Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina, Courtney Cavin On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote: > Input: synaptics-rmi4 - Use put_device() and device_type.release() > to free storage. > > From: Christopher Heiny <cheiny@synaptics.com> > > 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 <courtney.cavin@sonymobile.com> > Signed-off-by: Christopher Heiny <cheiny@synaptics.com> > > --- > > 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); We also need to tear down debugfs here. > 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); 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 <cheiny@synaptics.com> 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 <courtney.cavin@sonymobile.com> Signed-off-by: Christopher Heiny <cheiny@synaptics.com> 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; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-13 6:15 ` Dmitry Torokhov @ 2014-02-13 21:59 ` Courtney Cavin 2014-02-13 22:10 ` Dmitry Torokhov 0 siblings, 1 reply; 14+ messages in thread From: Courtney Cavin @ 2014-02-13 21:59 UTC (permalink / raw) To: Dmitry Torokhov 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 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: > > Input: synaptics-rmi4 - Use put_device() and device_type.release() > > to free storage. > > > > From: Christopher Heiny <cheiny@synaptics.com> > > > > 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. [...] > Input: synaptics-rmi4 - use put_device() to free devices > > From: Christopher Heiny <cheiny@synaptics.com> > > 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 <courtney.cavin@sonymobile.com> > Signed-off-by: Christopher Heiny <cheiny@synaptics.com> > 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 [...] > @@ -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... [...] > 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... > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c [...] > 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... > 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. > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-13 21:59 ` Courtney Cavin @ 2014-02-13 22:10 ` Dmitry Torokhov 2014-02-21 23:29 ` Christopher Heiny 0 siblings, 1 reply; 14+ messages in thread From: Dmitry Torokhov @ 2014-02-13 22:10 UTC (permalink / raw) 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 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: > > On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote: > > > Input: synaptics-rmi4 - Use put_device() and device_type.release() > > > to free storage. > > > > > > From: Christopher Heiny <cheiny@synaptics.com> > > > > > > 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. > [...] > > Input: synaptics-rmi4 - use put_device() to free devices > > > > From: Christopher Heiny <cheiny@synaptics.com> > > > > 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 <courtney.cavin@sonymobile.com> > > Signed-off-by: Christopher Heiny <cheiny@synaptics.com> > > 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 > [...] > > @@ -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... > > [...] > > 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... > > > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c > [...] > > 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... > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage. 2014-02-13 22:10 ` Dmitry Torokhov @ 2014-02-21 23:29 ` Christopher Heiny 0 siblings, 0 replies; 14+ messages in thread From: Christopher Heiny @ 2014-02-21 23:29 UTC (permalink / raw) To: Dmitry Torokhov, Courtney Cavin Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina 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: > > On Wed, Feb 12, 2014 at 06:31:04PM -0800, Christopher Heiny wrote: > > > Input: synaptics-rmi4 - Use put_device() and device_type.release() > > > to free storage. > > > > > > From: Christopher Heiny <cheiny@synaptics.com> > > > > > > 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. > [...] > > Input: synaptics-rmi4 - use put_device() to free devices > > > > From: Christopher Heiny <cheiny@synaptics.com> > > > > 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 <courtney.cavin@sonymobile.com> > > Signed-off-by: Christopher Heiny <cheiny@synaptics.com> > > 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 > [...] > > @@ -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... > > [...] > > 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... > > > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c > [...] > > 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... > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-02-21 23:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-02-11 23:13 [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage Christopher Heiny 2014-02-12 1:59 ` Courtney Cavin 2014-02-12 2:17 ` Christopher Heiny 2014-02-12 2:49 ` Courtney Cavin 2014-02-12 3:17 ` Christopher Heiny 2014-02-12 4:54 ` Courtney Cavin 2014-02-12 6:43 ` Dmitry Torokhov 2014-02-12 17:09 ` Courtney Cavin 2014-02-12 6:49 ` Dmitry Torokhov 2014-02-13 2:31 ` Christopher Heiny 2014-02-13 6:15 ` Dmitry Torokhov 2014-02-13 21:59 ` Courtney Cavin 2014-02-13 22:10 ` Dmitry Torokhov 2014-02-21 23:29 ` Christopher Heiny
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.