| Message ID | 20140213061524.GA15260@core.coreip.homeip.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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 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
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
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; }