atmel_mxt_ts fixes

atmel_mxt_ts fixes

[nick.dyer]

Hi Dmitry/Stephen-

I have tested and reworked the fix for the double free issue in atmel_mxt_ts
that Stephen identified. A lot more entertaining than I thought at first. Also
a minor change to reduce the verbosity of a warning message.

cheers

Nick

[PATCH 1/2] Input: atmel_mxt_ts - downgrade warning about empty interrupts

[nick.dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

In the case where the CHG/interrupt line mode is not configured correctly,
this warning is output to dmesg output for each interrupt. Downgrade the
message to debug.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index db178ed..d954b81 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -837,7 +837,12 @@ static irqreturn_t mxt_process_messages_t44(struct mxt_data *data)
 	count = data->msg_buf[0];
 
 	if (count == 0) {
-		dev_warn(dev, "Interrupt triggered but zero messages\n");
+		/*
+		 * This condition is caused by the CHG line being configured
+		 * in Mode 0. It results in unnecessary I2C operations but it
+		 * is benign.
+		 */
+		dev_dbg(dev, "Interrupt triggered but zero messages\n");
 		return IRQ_NONE;
 	} else if (count > data->max_reportid) {
 		dev_err(dev, "T44 count %d exceeded max report id\n", count);
-- 
1.9.1

[PATCH 2/2] Input: atmel_mxt_ts - fix double free of input device

[nick.dyer]

From: Stephen Warren <swarren@wwwdotorg.org>

[reworked after comments by Dmitry Torokhov. Move free of input device into
separate function. Only call in paths that require it. Move mxt_initialize
after sysfs init, because otherwise an error in the sysfs init may interfere
with the async return from the firmware loader. Add guards for sysfs
functions. ]
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 40 ++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index d954b81..65153c4 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1379,11 +1379,16 @@ static int mxt_get_info(struct mxt_data *data)
 	return 0;
 }
 
-static void mxt_free_object_table(struct mxt_data *data)
+static void mxt_free_input_device(struct mxt_data *data)
 {
-	input_unregister_device(data->input_dev);
-	data->input_dev = NULL;
+	if (data->input_dev) {
+		input_unregister_device(data->input_dev);
+		data->input_dev = NULL;
+	}
+}
 
+static void mxt_free_object_table(struct mxt_data *data)
+{
 	kfree(data->object_table);
 	data->object_table = NULL;
 	kfree(data->msg_buf);
@@ -1828,6 +1833,10 @@ static ssize_t mxt_fw_version_show(struct device *dev,
 {
 	struct mxt_data *data = dev_get_drvdata(dev);
 	struct mxt_info *info = &data->info;
+
+	if (!data->object_table)
+		return -EINVAL;
+
 	return scnprintf(buf, PAGE_SIZE, "%u.%u.%02X\n",
 			 info->version >> 4, info->version & 0xf, info->build);
 }
@@ -1838,6 +1847,10 @@ static ssize_t mxt_hw_version_show(struct device *dev,
 {
 	struct mxt_data *data = dev_get_drvdata(dev);
 	struct mxt_info *info = &data->info;
+
+	if (!data->object_table)
+		return -EINVAL;
+
 	return scnprintf(buf, PAGE_SIZE, "%u.%u\n",
 			 info->family_id, info->variant_id);
 }
@@ -1870,6 +1883,9 @@ static ssize_t mxt_object_show(struct device *dev,
 	int error;
 	u8 *obuf;
 
+	if (!data->object_table)
+		return -EINVAL;
+
 	/* Pre-allocate buffer large enough to hold max sized object. */
 	obuf = kmalloc(256, GFP_KERNEL);
 	if (!obuf)
@@ -1962,11 +1978,13 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 		ret = mxt_lookup_bootloader_address(data, 0);
 		if (ret)
 			goto release_firmware;
+
+		mxt_free_input_device(data);
+		mxt_free_object_table(data);
 	} else {
 		enable_irq(data->irq);
 	}
 
-	mxt_free_object_table(data);
 	reinit_completion(&data->bl_completion);
 
 	ret = mxt_check_bootloader(data, MXT_WAITING_BOOTLOAD_CMD, false);
@@ -2201,21 +2219,19 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	disable_irq(client->irq);
 
-	error = mxt_initialize(data);
-	if (error)
-		goto err_free_irq;
-
 	error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
 	if (error) {
 		dev_err(&client->dev, "Failure %d creating sysfs group\n",
 			error);
-		goto err_free_object;
+		goto err_free_irq;
 	}
 
+	error = mxt_initialize(data);
+	if (error)
+		goto err_free_irq;
+
 	return 0;
 
-err_free_object:
-	mxt_free_object_table(data);
 err_free_irq:
 	free_irq(client->irq, data);
 err_free_mem:
@@ -2229,7 +2245,7 @@ static int mxt_remove(struct i2c_client *client)
 
 	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
 	free_irq(data->irq, data);
-	input_unregister_device(data->input_dev);
+	mxt_free_input_device(data);
 	mxt_free_object_table(data);
 	kfree(data);
 
-- 
1.9.1

Re: [PATCH 1/2] Input: atmel_mxt_ts - downgrade warning about empty interrupts

[Dmitry Torokhov]

On Tue, Sep 09, 2014 at 03:50:48PM +0100, nick.dyer@itdev.co.uk wrote:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> In the case where the CHG/interrupt line mode is not configured correctly,
> this warning is output to dmesg output for each interrupt. Downgrade the
> message to debug.
> 
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>

Applied, thank you.

> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index db178ed..d954b81 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -837,7 +837,12 @@ static irqreturn_t mxt_process_messages_t44(struct mxt_data *data)
>  	count = data->msg_buf[0];
>  
>  	if (count == 0) {
> -		dev_warn(dev, "Interrupt triggered but zero messages\n");
> +		/*
> +		 * This condition is caused by the CHG line being configured
> +		 * in Mode 0. It results in unnecessary I2C operations but it
> +		 * is benign.
> +		 */
> +		dev_dbg(dev, "Interrupt triggered but zero messages\n");
>  		return IRQ_NONE;
>  	} else if (count > data->max_reportid) {
>  		dev_err(dev, "T44 count %d exceeded max report id\n", count);
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH 2/2] Input: atmel_mxt_ts - fix double free of input device

[Dmitry Torokhov]

On Tue, Sep 09, 2014 at 03:50:49PM +0100, nick.dyer@itdev.co.uk wrote:
> From: Stephen Warren <swarren@wwwdotorg.org>
> 
> [reworked after comments by Dmitry Torokhov. Move free of input device into
> separate function. Only call in paths that require it. Move mxt_initialize
> after sysfs init, because otherwise an error in the sysfs init may interfere
> with the async return from the firmware loader. Add guards for sysfs
> functions. ]

Ugh... there is still problem with asycn firmware loading: you need to
make sure it is done before you try to unbind the dveice. I also do not
see what stops several firmware update requests to happen
simultaneously. Once you add proper handling for that you can use the
same lock in sysfs read methods.

Another option is wait a bit and see what's the outcome of async probing
discussion on LKML is and maybe we can stop using
request_firmware_nowait() in probe path but rather have device core fire
off probe asynchronously.

I'd rather have fix for input device freeing be separate from
sysfs/firmware/config loading changes.


Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: atmel_mxt_ts - fix double free of input device

[Nick Dyer]

On 10/09/14 00:49, Dmitry Torokhov wrote:
> On Tue, Sep 09, 2014 at 03:50:49PM +0100, nick.dyer@itdev.co.uk wrote:
>> From: Stephen Warren <swarren@wwwdotorg.org>
>>
>> [reworked after comments by Dmitry Torokhov. Move free of input device into
>> separate function. Only call in paths that require it. Move mxt_initialize
>> after sysfs init, because otherwise an error in the sysfs init may interfere
>> with the async return from the firmware loader. Add guards for sysfs
>> functions. ]
> 
> Ugh... there is still problem with asycn firmware loading: you need to
> make sure it is done before you try to unbind the dveice. I also do not
> see what stops several firmware update requests to happen
> simultaneously. Once you add proper handling for that you can use the
> same lock in sysfs read methods.

Yes, I see what you mean. I will try and straighten it out.

> Another option is wait a bit and see what's the outcome of async probing
> discussion on LKML is and maybe we can stop using
> request_firmware_nowait() in probe path but rather have device core fire
> off probe asynchronously.
>
> I'd rather have fix for input device freeing be separate from
> sysfs/firmware/config loading changes.

I agree, it is better to fix the common issue with a straightforward patch.
I have split it apart and will send this patch now.

[PATCH 2/2 v2] Input: atmel_mxt_ts - fix double free of input device

[nick.dyer]

From: Stephen Warren <swarren@wwwdotorg.org>

[reworked after comments by Dmitry Torokhov. Move free of input device into
separate function. Only call in paths that require it.]
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index d954b81..aaacf8b 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1379,11 +1379,16 @@ static int mxt_get_info(struct mxt_data *data)
 	return 0;
 }
 
-static void mxt_free_object_table(struct mxt_data *data)
+static void mxt_free_input_device(struct mxt_data *data)
 {
-	input_unregister_device(data->input_dev);
-	data->input_dev = NULL;
+	if (data->input_dev) {
+		input_unregister_device(data->input_dev);
+		data->input_dev = NULL;
+	}
+}
 
+static void mxt_free_object_table(struct mxt_data *data)
+{
 	kfree(data->object_table);
 	data->object_table = NULL;
 	kfree(data->msg_buf);
@@ -1962,11 +1967,13 @@ static int mxt_load_fw(struct device *dev, const char *fn)
 		ret = mxt_lookup_bootloader_address(data, 0);
 		if (ret)
 			goto release_firmware;
+
+		mxt_free_input_device(data);
+		mxt_free_object_table(data);
 	} else {
 		enable_irq(data->irq);
 	}
 
-	mxt_free_object_table(data);
 	reinit_completion(&data->bl_completion);
 
 	ret = mxt_check_bootloader(data, MXT_WAITING_BOOTLOAD_CMD, false);
@@ -2215,6 +2222,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	return 0;
 
 err_free_object:
+	mxt_free_input_device(data);
 	mxt_free_object_table(data);
 err_free_irq:
 	free_irq(client->irq, data);
@@ -2229,7 +2237,7 @@ static int mxt_remove(struct i2c_client *client)
 
 	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
 	free_irq(data->irq, data);
-	input_unregister_device(data->input_dev);
+	mxt_free_input_device(data);
 	mxt_free_object_table(data);
 	kfree(data);
 
-- 
1.9.1

Re: [PATCH 2/2 v2] Input: atmel_mxt_ts - fix double free of input device

[Stephen Warren]

On 09/10/2014 08:33 AM, nick.dyer@itdev.co.uk wrote:
> From: Stephen Warren <swarren@wwwdotorg.org>
>
> [reworked after comments by Dmitry Torokhov. Move free of input device into
> separate function. Only call in paths that require it.]
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>

I re-tested this, and see no issues after inserting/removing the module 
a few times.

Re: [PATCH 2/2 v2] Input: atmel_mxt_ts - fix double free of input device

[Dmitry Torokhov]

On Wed, Sep 10, 2014 at 11:28:16AM -0600, Stephen Warren wrote:
> On 09/10/2014 08:33 AM, nick.dyer@itdev.co.uk wrote:
> >From: Stephen Warren <swarren@wwwdotorg.org>
> >
> >[reworked after comments by Dmitry Torokhov. Move free of input device into
> >separate function. Only call in paths that require it.]
> >Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> I re-tested this, and see no issues after inserting/removing the
> module a few times.

Applied, thank you.

-- 
Dmitry