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

Message ID 20190103012938.7442-6-marex@denx.de
State Superseded
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.
Marek Vasut Jan. 30, 2019, 6:43 a.m. UTC | #4
On 1/15/19 4:03 AM, Dmitry Torokhov wrote:
> 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.

Which sysfs api is that ? git grep for sysfs_create_group only shows me
the non-devm variants.
Dmitry Torokhov Jan. 30, 2019, 6:44 p.m. UTC | #5
On Tue, Jan 29, 2019 at 11:05 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/15/19 4:03 AM, Dmitry Torokhov wrote:
> > 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.
>
> Which sysfs api is that ? git grep for sysfs_create_group only shows me
> the non-devm variants.

devm_device_add_group[s]()

Thanks.
Marek Vasut Jan. 31, 2019, 3:20 p.m. UTC | #6
On 1/30/19 7:44 PM, Dmitry Torokhov wrote:
> On Tue, Jan 29, 2019 at 11:05 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/15/19 4:03 AM, Dmitry Torokhov wrote:
>>> 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.
>>
>> Which sysfs api is that ? git grep for sysfs_create_group only shows me
>> the non-devm variants.
> 
> devm_device_add_group[s]()

OK, I'll fix that in a subsequent patch if that's OK.

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;