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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Christopher Heiny <cheiny@xxxxxxxxxxxxx>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
