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]()
Thanks.
--
Dmitry
