[V3,05/10] input: touchscreen: ili210x: Convert to threaded IRQ
diff mbox series

Message ID 20190103012938.7442-6-marex@denx.de
State Under Review
Headers show
Series
  • input: touchscreen: ili210x: Add ILI2511 support
Related show

Commit Message

Marek Vasut Jan. 3, 2019, 1:29 a.m. UTC
Convert the driver to devm_request_threaded_irq() and drop the related
unmanaged deregistration code.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Henrik Rydberg <rydberg@bitmath.org>
Cc: Olivier Sobrie <olivier@sobrie.be>
Cc: Philipp Puschmann <pp@emlix.com>
To: linux-input@vger.kernel.org
---
V2: Retain workqueue
V3: Reword the commit message
---
 drivers/input/touchscreen/ili210x.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Comments

Dmitry Torokhov Jan. 15, 2019, 12:33 a.m. UTC | #1
Hi Marek,

On Thu, Jan 03, 2019 at 02:29:33AM +0100, Marek Vasut wrote:
> Convert the driver to devm_request_threaded_irq() and drop the related
> unmanaged deregistration code.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Henrik Rydberg <rydberg@bitmath.org>
> Cc: Olivier Sobrie <olivier@sobrie.be>
> Cc: Philipp Puschmann <pp@emlix.com>
> To: linux-input@vger.kernel.org
> ---
> V2: Retain workqueue
> V3: Reword the commit message
> ---
>  drivers/input/touchscreen/ili210x.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 788f4140e302..436bfedb6769 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -5,7 +5,6 @@
>  #include <linux/input.h>
>  #include <linux/input/mt.h>
>  #include <linux/delay.h>
> -#include <linux/workqueue.h>
>  
>  #define MAX_TOUCHES		2
>  #define DEFAULT_POLL_PERIOD	20
> @@ -240,19 +239,19 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, priv);
>  
> -	error = request_irq(client->irq, ili210x_irq, 0,
> -			    client->name, priv);
> +	error = devm_request_threaded_irq(dev, client->irq, NULL, ili210x_irq,
> +				 IRQF_ONESHOT, client->name, priv);

What is the point of making it threaded given that the only thing you do
in now threaded interrupt handler is scheduling a work?

>  	if (error) {
>  		dev_err(dev, "Unable to request touchscreen IRQ, err: %d\n",
>  			error);
> -		goto err_free_mem;
> +		return error;
>  	}
>  
>  	error = sysfs_create_group(&dev->kobj, &ili210x_attr_group);
>  	if (error) {
>  		dev_err(dev, "Unable to create sysfs attributes, err: %d\n",
>  			error);
> -		goto err_free_irq;
> +		return error;
>  	}
>  
>  	error = input_register_device(priv->input);
> @@ -271,9 +270,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  
>  err_remove_sysfs:
>  	sysfs_remove_group(&dev->kobj, &ili210x_attr_group);
> -err_free_irq:
> -	free_irq(client->irq, priv);
> -err_free_mem:
>  	return error;
>  }
>  
> @@ -282,7 +278,6 @@ static int ili210x_i2c_remove(struct i2c_client *client)
>  	struct ili210x *priv = i2c_get_clientdata(client);
>  
>  	sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group);
> -	free_irq(priv->client->irq, priv);
>  	cancel_delayed_work_sync(&priv->dwork);

This is wrong as now can end up with work being scheduled again and
you'll end up with use after free.

Thanks.
Marek Vasut Jan. 15, 2019, 2:09 a.m. UTC | #2
On 1/15/19 1:33 AM, Dmitry Torokhov wrote:
> Hi Marek,
> 
> On Thu, Jan 03, 2019 at 02:29:33AM +0100, Marek Vasut wrote:
>> Convert the driver to devm_request_threaded_irq() and drop the related
>> unmanaged deregistration code.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Henrik Rydberg <rydberg@bitmath.org>
>> Cc: Olivier Sobrie <olivier@sobrie.be>
>> Cc: Philipp Puschmann <pp@emlix.com>
>> To: linux-input@vger.kernel.org
>> ---
>> V2: Retain workqueue
>> V3: Reword the commit message
>> ---
>>  drivers/input/touchscreen/ili210x.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
>> index 788f4140e302..436bfedb6769 100644
>> --- a/drivers/input/touchscreen/ili210x.c
>> +++ b/drivers/input/touchscreen/ili210x.c
>> @@ -5,7 +5,6 @@
>>  #include <linux/input.h>
>>  #include <linux/input/mt.h>
>>  #include <linux/delay.h>
>> -#include <linux/workqueue.h>
>>  
>>  #define MAX_TOUCHES		2
>>  #define DEFAULT_POLL_PERIOD	20
>> @@ -240,19 +239,19 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>>  
>>  	i2c_set_clientdata(client, priv);
>>  
>> -	error = request_irq(client->irq, ili210x_irq, 0,
>> -			    client->name, priv);
>> +	error = devm_request_threaded_irq(dev, client->irq, NULL, ili210x_irq,
>> +				 IRQF_ONESHOT, client->name, priv);
> 
> What is the point of making it threaded given that the only thing you do
> in now threaded interrupt handler is scheduling a work?
> 
>>  	if (error) {
>>  		dev_err(dev, "Unable to request touchscreen IRQ, err: %d\n",
>>  			error);
>> -		goto err_free_mem;
>> +		return error;
>>  	}
>>  
>>  	error = sysfs_create_group(&dev->kobj, &ili210x_attr_group);
>>  	if (error) {
>>  		dev_err(dev, "Unable to create sysfs attributes, err: %d\n",
>>  			error);
>> -		goto err_free_irq;
>> +		return error;
>>  	}
>>  
>>  	error = input_register_device(priv->input);
>> @@ -271,9 +270,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>>  
>>  err_remove_sysfs:
>>  	sysfs_remove_group(&dev->kobj, &ili210x_attr_group);
>> -err_free_irq:
>> -	free_irq(client->irq, priv);
>> -err_free_mem:
>>  	return error;
>>  }
>>  
>> @@ -282,7 +278,6 @@ static int ili210x_i2c_remove(struct i2c_client *client)
>>  	struct ili210x *priv = i2c_get_clientdata(client);
>>  
>>  	sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group);
>> -	free_irq(priv->client->irq, priv);
>>  	cancel_delayed_work_sync(&priv->dwork);
> 
> This is wrong as now can end up with work being scheduled again and
> you'll end up with use after free.

So shall I just drop this patch ?
Dmitry Torokhov Jan. 15, 2019, 3:03 a.m. UTC | #3
On Mon, Jan 14, 2019 at 6:17 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/15/19 1:33 AM, Dmitry Torokhov wrote:
> > Hi Marek,
> >
> > On Thu, Jan 03, 2019 at 02:29:33AM +0100, Marek Vasut wrote:
> >> Convert the driver to devm_request_threaded_irq() and drop the related
> >> unmanaged deregistration code.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> Cc: Henrik Rydberg <rydberg@bitmath.org>
> >> Cc: Olivier Sobrie <olivier@sobrie.be>
> >> Cc: Philipp Puschmann <pp@emlix.com>
> >> To: linux-input@vger.kernel.org
> >> ---
> >> V2: Retain workqueue
> >> V3: Reword the commit message
> >> ---
> >>  drivers/input/touchscreen/ili210x.c | 13 ++++---------
> >>  1 file changed, 4 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> >> index 788f4140e302..436bfedb6769 100644
> >> --- a/drivers/input/touchscreen/ili210x.c
> >> +++ b/drivers/input/touchscreen/ili210x.c
> >> @@ -5,7 +5,6 @@
> >>  #include <linux/input.h>
> >>  #include <linux/input/mt.h>
> >>  #include <linux/delay.h>
> >> -#include <linux/workqueue.h>
> >>
> >>  #define MAX_TOUCHES         2
> >>  #define DEFAULT_POLL_PERIOD 20
> >> @@ -240,19 +239,19 @@ static int ili210x_i2c_probe(struct i2c_client *client,
> >>
> >>      i2c_set_clientdata(client, priv);
> >>
> >> -    error = request_irq(client->irq, ili210x_irq, 0,
> >> -                        client->name, priv);
> >> +    error = devm_request_threaded_irq(dev, client->irq, NULL, ili210x_irq,
> >> +                             IRQF_ONESHOT, client->name, priv);
> >
> > What is the point of making it threaded given that the only thing you do
> > in now threaded interrupt handler is scheduling a work?
> >
> >>      if (error) {
> >>              dev_err(dev, "Unable to request touchscreen IRQ, err: %d\n",
> >>                      error);
> >> -            goto err_free_mem;
> >> +            return error;
> >>      }
> >>
> >>      error = sysfs_create_group(&dev->kobj, &ili210x_attr_group);
> >>      if (error) {
> >>              dev_err(dev, "Unable to create sysfs attributes, err: %d\n",
> >>                      error);
> >> -            goto err_free_irq;
> >> +            return error;
> >>      }
> >>
> >>      error = input_register_device(priv->input);
> >> @@ -271,9 +270,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
> >>
> >>  err_remove_sysfs:
> >>      sysfs_remove_group(&dev->kobj, &ili210x_attr_group);
> >> -err_free_irq:
> >> -    free_irq(client->irq, priv);
> >> -err_free_mem:
> >>      return error;
> >>  }
> >>
> >> @@ -282,7 +278,6 @@ static int ili210x_i2c_remove(struct i2c_client *client)
> >>      struct ili210x *priv = i2c_get_clientdata(client);
> >>
> >>      sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group);
> >> -    free_irq(priv->client->irq, priv);
> >>      cancel_delayed_work_sync(&priv->dwork);
> >
> > This is wrong as now can end up with work being scheduled again and
> > you'll end up with use after free.
>
> So shall I just drop this patch ?

Definitely want to keep the non-threaded variant, but can keep devm if
you add cancel as a custom devm action. There is also devm API to
create sysfs attributes, if you do that we can get rid of
ili210x_i2c_remove completely.

Thanks.

Patch
diff mbox series

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 788f4140e302..436bfedb6769 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -5,7 +5,6 @@ 
 #include <linux/input.h>
 #include <linux/input/mt.h>
 #include <linux/delay.h>
-#include <linux/workqueue.h>
 
 #define MAX_TOUCHES		2
 #define DEFAULT_POLL_PERIOD	20
@@ -240,19 +239,19 @@  static int ili210x_i2c_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, priv);
 
-	error = request_irq(client->irq, ili210x_irq, 0,
-			    client->name, priv);
+	error = devm_request_threaded_irq(dev, client->irq, NULL, ili210x_irq,
+				 IRQF_ONESHOT, client->name, priv);
 	if (error) {
 		dev_err(dev, "Unable to request touchscreen IRQ, err: %d\n",
 			error);
-		goto err_free_mem;
+		return error;
 	}
 
 	error = sysfs_create_group(&dev->kobj, &ili210x_attr_group);
 	if (error) {
 		dev_err(dev, "Unable to create sysfs attributes, err: %d\n",
 			error);
-		goto err_free_irq;
+		return error;
 	}
 
 	error = input_register_device(priv->input);
@@ -271,9 +270,6 @@  static int ili210x_i2c_probe(struct i2c_client *client,
 
 err_remove_sysfs:
 	sysfs_remove_group(&dev->kobj, &ili210x_attr_group);
-err_free_irq:
-	free_irq(client->irq, priv);
-err_free_mem:
 	return error;
 }
 
@@ -282,7 +278,6 @@  static int ili210x_i2c_remove(struct i2c_client *client)
 	struct ili210x *priv = i2c_get_clientdata(client);
 
 	sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group);
-	free_irq(priv->client->irq, priv);
 	cancel_delayed_work_sync(&priv->dwork);
 
 	return 0;