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.
--
Best regards,
Marek Vasut
