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.
Thanks.
--
Dmitry
