| Message ID | 1392160410-8293-1-git-send-email-cheiny@synaptics.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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. -- 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
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 -- 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
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.
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.
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 -- 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
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
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(-) -- 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