On 1/30/19 7:44 PM, Dmitry Torokhov wrote:
> On Tue, Jan 29, 2019 at 11:05 PM Marek Vasut <marex@xxxxxxx> wrote:
>>
>> On 1/15/19 4:03 AM, Dmitry Torokhov wrote:
>>> On Mon, Jan 14, 2019 at 6:17 PM Marek Vasut <marex@xxxxxxx> 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@xxxxxxx>
>>>>>> Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>>>>>> Cc: Henrik Rydberg <rydberg@xxxxxxxxxxx>
>>>>>> Cc: Olivier Sobrie <olivier@xxxxxxxxx>
>>>>>> Cc: Philipp Puschmann <pp@xxxxxxxxx>
>>>>>> To: linux-input@xxxxxxxxxxxxxxx
>>>>>> ---
>>>>>> 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.
--
Best regards,
Marek Vasut
