Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1493067 > unrolled thread
| Started by | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| First post | 2016-09-29 02:40 +0200 |
| Last post | 2016-10-01 05:10 +0200 |
| Articles | 11 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Guenter Roeck <linux@roeck-us.net> - 2016-09-29 02:40 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-09-29 20:00 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-01 01:00 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-01 01:10 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Guenter Roeck <linux@roeck-us.net> - 2016-10-01 05:50 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Andrew Duggan <andrew@duggan.us> - 2016-10-01 19:40 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Guenter Roeck <linux@roeck-us.net> - 2016-10-01 19:50 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Andrew Duggan <andrew@duggan.us> - 2016-10-01 20:00 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-01 21:10 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Guenter Roeck <linux@roeck-us.net> - 2016-10-01 22:50 +0200
Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver Guenter Roeck <linux@roeck-us.net> - 2016-10-01 05:10 +0200
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-09-29 02:40 +0200 |
| Subject | [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <smxyh-5qo-9@gated-at.bofh.it> |
Instantiating the rmi4 I2C transport driver without interrupts assigned
(for example using manual i2c instantiation from the command line)
caused the driver to fail to load, but it does not clean up its
regulator or transport device registrations. Result is a crash at a later
time, for example when rebooting the system.
Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Andrew Duggan <aduggan@synaptics.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/input/rmi4/rmi_i2c.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index 6f2e0e4f0296..d57b227ccd25 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -285,23 +285,30 @@ static int rmi_i2c_probe(struct i2c_client *client,
retval = rmi_set_page(rmi_i2c, 0);
if (retval) {
dev_err(&client->dev, "Failed to set page select to 0.\n");
- return retval;
+ goto error_disable;
}
retval = rmi_register_transport_device(&rmi_i2c->xport);
if (retval) {
dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
client->addr);
- return retval;
+ goto error_disable;
}
retval = rmi_i2c_init_irq(client);
if (retval < 0)
- return retval;
+ goto error_unregister;
dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
client->addr);
return 0;
+
+error_unregister:
+ rmi_unregister_transport_device(&rmi_i2c->xport);
+error_disable:
+ regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
+ rmi_i2c->supplies);
+ return retval;
}
static int rmi_i2c_remove(struct i2c_client *client)
--
2.5.0
[toc] | [next] | [standalone]
| From | Bjorn Andersson <bjorn.andersson@linaro.org> |
|---|---|
| Date | 2016-09-29 20:00 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <smNMJ-7br-11@gated-at.bofh.it> |
| In reply to | #1493067 |
On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
> Instantiating the rmi4 I2C transport driver without interrupts assigned
> (for example using manual i2c instantiation from the command line)
> caused the driver to fail to load, but it does not clean up its
> regulator or transport device registrations. Result is a crash at a later
> time, for example when rebooting the system.
>
> Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
Sorry for that.
> Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> Cc: Andrew Duggan <aduggan@synaptics.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/input/rmi4/rmi_i2c.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index 6f2e0e4f0296..d57b227ccd25 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -285,23 +285,30 @@ static int rmi_i2c_probe(struct i2c_client *client,
> retval = rmi_set_page(rmi_i2c, 0);
> if (retval) {
> dev_err(&client->dev, "Failed to set page select to 0.\n");
> - return retval;
> + goto error_disable;
> }
>
> retval = rmi_register_transport_device(&rmi_i2c->xport);
> if (retval) {
> dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
> client->addr);
> - return retval;
> + goto error_disable;
> }
>
> retval = rmi_i2c_init_irq(client);
> if (retval < 0)
> - return retval;
> + goto error_unregister;
>
> dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
> client->addr);
> return 0;
> +
> +error_unregister:
> + rmi_unregister_transport_device(&rmi_i2c->xport);
> +error_disable:
> + regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> + rmi_i2c->supplies);
> + return retval;
> }
>
> static int rmi_i2c_remove(struct i2c_client *client)
> --
> 2.5.0
>
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2016-10-01 01:00 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <sneWB-8fw-5@gated-at.bofh.it> |
| In reply to | #1493627 |
On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
> On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
>
> > Instantiating the rmi4 I2C transport driver without interrupts assigned
> > (for example using manual i2c instantiation from the command line)
> > caused the driver to fail to load, but it does not clean up its
> > regulator or transport device registrations. Result is a crash at a later
> > time, for example when rebooting the system.
> >
> > Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
>
> Sorry for that.
>
> > Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Applied, thank you.
It looks like we have similar issue in rmi_spi.c. Can I get another
patch?
>
> Regards,
> Bjorn
>
> > Cc: Andrew Duggan <aduggan@synaptics.com>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > drivers/input/rmi4/rmi_i2c.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> > index 6f2e0e4f0296..d57b227ccd25 100644
> > --- a/drivers/input/rmi4/rmi_i2c.c
> > +++ b/drivers/input/rmi4/rmi_i2c.c
> > @@ -285,23 +285,30 @@ static int rmi_i2c_probe(struct i2c_client *client,
> > retval = rmi_set_page(rmi_i2c, 0);
> > if (retval) {
> > dev_err(&client->dev, "Failed to set page select to 0.\n");
> > - return retval;
> > + goto error_disable;
> > }
> >
> > retval = rmi_register_transport_device(&rmi_i2c->xport);
> > if (retval) {
> > dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
> > client->addr);
> > - return retval;
> > + goto error_disable;
> > }
> >
> > retval = rmi_i2c_init_irq(client);
> > if (retval < 0)
> > - return retval;
> > + goto error_unregister;
> >
> > dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
> > client->addr);
> > return 0;
> > +
> > +error_unregister:
> > + rmi_unregister_transport_device(&rmi_i2c->xport);
> > +error_disable:
> > + regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> > + rmi_i2c->supplies);
> > + return retval;
> > }
> >
> > static int rmi_i2c_remove(struct i2c_client *client)
> > --
> > 2.5.0
> >
--
Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2016-10-01 01:10 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <snf6h-6w-23@gated-at.bofh.it> |
| In reply to | #1494319 |
On Fri, Sep 30, 2016 at 03:54:03PM -0700, Dmitry Torokhov wrote:
> On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
> > On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
> >
> > > Instantiating the rmi4 I2C transport driver without interrupts assigned
> > > (for example using manual i2c instantiation from the command line)
> > > caused the driver to fail to load, but it does not clean up its
> > > regulator or transport device registrations. Result is a crash at a later
> > > time, for example when rebooting the system.
> > >
> > > Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
> >
> > Sorry for that.
> >
> > > Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> >
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> Applied, thank you.
I take it back. rmi_i2c_init_irq() uses devm* so this whole thing mixes
up devm* and manual unregistering and unwind order is completely
broken.
1. Why do we register interrupt from transport drivers and not make it
part of rmi_register_transport_device()?
2. If we need to use some non-devm-ised resources we should use
devm_add_action[_or_reset] to work these operations into devm stream.
Thanks,
>
> It looks like we have similar issue in rmi_spi.c. Can I get another
> patch?
>
> >
> > Regards,
> > Bjorn
> >
> > > Cc: Andrew Duggan <aduggan@synaptics.com>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > > drivers/input/rmi4/rmi_i2c.c | 13 ++++++++++---
> > > 1 file changed, 10 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> > > index 6f2e0e4f0296..d57b227ccd25 100644
> > > --- a/drivers/input/rmi4/rmi_i2c.c
> > > +++ b/drivers/input/rmi4/rmi_i2c.c
> > > @@ -285,23 +285,30 @@ static int rmi_i2c_probe(struct i2c_client *client,
> > > retval = rmi_set_page(rmi_i2c, 0);
> > > if (retval) {
> > > dev_err(&client->dev, "Failed to set page select to 0.\n");
> > > - return retval;
> > > + goto error_disable;
> > > }
> > >
> > > retval = rmi_register_transport_device(&rmi_i2c->xport);
> > > if (retval) {
> > > dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
> > > client->addr);
> > > - return retval;
> > > + goto error_disable;
> > > }
> > >
> > > retval = rmi_i2c_init_irq(client);
> > > if (retval < 0)
> > > - return retval;
> > > + goto error_unregister;
> > >
> > > dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
> > > client->addr);
> > > return 0;
> > > +
> > > +error_unregister:
> > > + rmi_unregister_transport_device(&rmi_i2c->xport);
> > > +error_disable:
> > > + regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> > > + rmi_i2c->supplies);
> > > + return retval;
> > > }
> > >
> > > static int rmi_i2c_remove(struct i2c_client *client)
> > > --
> > > 2.5.0
> > >
>
> --
> Dmitry
--
Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-10-01 05:50 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <snjtf-2OO-3@gated-at.bofh.it> |
| In reply to | #1494321 |
On 09/30/2016 04:02 PM, Dmitry Torokhov wrote:
> On Fri, Sep 30, 2016 at 03:54:03PM -0700, Dmitry Torokhov wrote:
>> On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
>>> On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
>>>
>>>> Instantiating the rmi4 I2C transport driver without interrupts assigned
>>>> (for example using manual i2c instantiation from the command line)
>>>> caused the driver to fail to load, but it does not clean up its
>>>> regulator or transport device registrations. Result is a crash at a later
>>>> time, for example when rebooting the system.
>>>>
>>>> Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
>>>
>>> Sorry for that.
>>>
>>>> Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
>>>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>
>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>
>> Applied, thank you.
>
> I take it back. rmi_i2c_init_irq() uses devm* so this whole thing mixes
> up devm* and manual unregistering and unwind order is completely
> broken.
>
Oops ...
> 1. Why do we register interrupt from transport drivers and not make it
> part of rmi_register_transport_device()?
rmi_register_transport_device() doesn't take dev as parameter.
> 2. If we need to use some non-devm-ised resources we should use
> devm_add_action[_or_reset] to work these operations into devm stream.
Ok, no problem.
Guenter
>
> Thanks,
>
>>
>> It looks like we have similar issue in rmi_spi.c. Can I get another
>> patch?
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> Cc: Andrew Duggan <aduggan@synaptics.com>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> drivers/input/rmi4/rmi_i2c.c | 13 ++++++++++---
>>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
>>>> index 6f2e0e4f0296..d57b227ccd25 100644
>>>> --- a/drivers/input/rmi4/rmi_i2c.c
>>>> +++ b/drivers/input/rmi4/rmi_i2c.c
>>>> @@ -285,23 +285,30 @@ static int rmi_i2c_probe(struct i2c_client *client,
>>>> retval = rmi_set_page(rmi_i2c, 0);
>>>> if (retval) {
>>>> dev_err(&client->dev, "Failed to set page select to 0.\n");
>>>> - return retval;
>>>> + goto error_disable;
>>>> }
>>>>
>>>> retval = rmi_register_transport_device(&rmi_i2c->xport);
>>>> if (retval) {
>>>> dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
>>>> client->addr);
>>>> - return retval;
>>>> + goto error_disable;
>>>> }
>>>>
>>>> retval = rmi_i2c_init_irq(client);
>>>> if (retval < 0)
>>>> - return retval;
>>>> + goto error_unregister;
>>>>
>>>> dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
>>>> client->addr);
>>>> return 0;
>>>> +
>>>> +error_unregister:
>>>> + rmi_unregister_transport_device(&rmi_i2c->xport);
>>>> +error_disable:
>>>> + regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
>>>> + rmi_i2c->supplies);
>>>> + return retval;
>>>> }
>>>>
>>>> static int rmi_i2c_remove(struct i2c_client *client)
>>>> --
>>>> 2.5.0
>>>>
>>
>> --
>> Dmitry
>
[toc] | [prev] | [next] | [standalone]
| From | Andrew Duggan <andrew@duggan.us> |
|---|---|
| Date | 2016-10-01 19:40 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <snwqu-3fm-19@gated-at.bofh.it> |
| In reply to | #1494348 |
On Fri, Sep 30, 2016, at 08:44 PM, Guenter Roeck wrote:
> On 09/30/2016 04:02 PM, Dmitry Torokhov wrote:
> > On Fri, Sep 30, 2016 at 03:54:03PM -0700, Dmitry Torokhov wrote:
> >> On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
> >>> On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
> >>>
> >>>> Instantiating the rmi4 I2C transport driver without interrupts assigned
> >>>> (for example using manual i2c instantiation from the command line)
> >>>> caused the driver to fail to load, but it does not clean up its
> >>>> regulator or transport device registrations. Result is a crash at a later
> >>>> time, for example when rebooting the system.
> >>>>
> >>>> Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
> >>>
> >>> Sorry for that.
> >>>
> >>>> Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
> >>>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>
> >>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>
> >> Applied, thank you.
> >
> > I take it back. rmi_i2c_init_irq() uses devm* so this whole thing mixes
> > up devm* and manual unregistering and unwind order is completely
> > broken.
> >
> Oops ...
>
> > 1. Why do we register interrupt from transport drivers and not make it
> > part of rmi_register_transport_device()?
Not all RMI devices will have access to interrupts (ie HID and SMBus).
The same goes for regulators. Here is a reference to a previous
discussion regarding both:
https://lkml.org/lkml/2016/5/9/1055
>
> rmi_register_transport_device() doesn't take dev as parameter.
>
> > 2. If we need to use some non-devm-ised resources we should use
> > devm_add_action[_or_reset] to work these operations into devm stream.
>
Since the regulator functions have their own devm_ versions I would
suggest switching to those functions to avoid dealing with
unregistering.
Registering and unregistering the transport device is a bit more
complicated since these functions add and put the rmi_dev device. But,
it sounds like we can handle the unregister using
devm_add_action_or_reset().
Andrew
> Ok, no problem.
>
> Guenter
>
> >
> > Thanks,
> >
> >>
> >> It looks like we have similar issue in rmi_spi.c. Can I get another
> >> patch?
> >>
> >>>
> >>> Regards,
> >>> Bjorn
> >>>
> >>>> Cc: Andrew Duggan <aduggan@synaptics.com>
> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>>> ---
> >>>> drivers/input/rmi4/rmi_i2c.c | 13 ++++++++++---
> >>>> 1 file changed, 10 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> >>>> index 6f2e0e4f0296..d57b227ccd25 100644
> >>>> --- a/drivers/input/rmi4/rmi_i2c.c
> >>>> +++ b/drivers/input/rmi4/rmi_i2c.c
> >>>> @@ -285,23 +285,30 @@ static int rmi_i2c_probe(struct i2c_client *client,
> >>>> retval = rmi_set_page(rmi_i2c, 0);
> >>>> if (retval) {
> >>>> dev_err(&client->dev, "Failed to set page select to 0.\n");
> >>>> - return retval;
> >>>> + goto error_disable;
> >>>> }
> >>>>
> >>>> retval = rmi_register_transport_device(&rmi_i2c->xport);
> >>>> if (retval) {
> >>>> dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
> >>>> client->addr);
> >>>> - return retval;
> >>>> + goto error_disable;
> >>>> }
> >>>>
> >>>> retval = rmi_i2c_init_irq(client);
> >>>> if (retval < 0)
> >>>> - return retval;
> >>>> + goto error_unregister;
> >>>>
> >>>> dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
> >>>> client->addr);
> >>>> return 0;
> >>>> +
> >>>> +error_unregister:
> >>>> + rmi_unregister_transport_device(&rmi_i2c->xport);
> >>>> +error_disable:
> >>>> + regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> >>>> + rmi_i2c->supplies);
> >>>> + return retval;
> >>>> }
> >>>>
> >>>> static int rmi_i2c_remove(struct i2c_client *client)
> >>>> --
> >>>> 2.5.0
> >>>>
> >>
> >> --
> >> Dmitry
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[toc] | [prev] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-10-01 19:50 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <snwA9-3jl-5@gated-at.bofh.it> |
| In reply to | #1494446 |
On 10/01/2016 10:27 AM, Andrew Duggan wrote:
>
>
> On Fri, Sep 30, 2016, at 08:44 PM, Guenter Roeck wrote:
>> On 09/30/2016 04:02 PM, Dmitry Torokhov wrote:
>>> On Fri, Sep 30, 2016 at 03:54:03PM -0700, Dmitry Torokhov wrote:
>>>> On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
>>>>> On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
>>>>>
>>>>>> Instantiating the rmi4 I2C transport driver without interrupts assigned
>>>>>> (for example using manual i2c instantiation from the command line)
>>>>>> caused the driver to fail to load, but it does not clean up its
>>>>>> regulator or transport device registrations. Result is a crash at a later
>>>>>> time, for example when rebooting the system.
>>>>>>
>>>>>> Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
>>>>>
>>>>> Sorry for that.
>>>>>
>>>>>> Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
>>>>>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>>
>>>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>
>>>> Applied, thank you.
>>>
>>> I take it back. rmi_i2c_init_irq() uses devm* so this whole thing mixes
>>> up devm* and manual unregistering and unwind order is completely
>>> broken.
>>>
>> Oops ...
>>
>>> 1. Why do we register interrupt from transport drivers and not make it
>>> part of rmi_register_transport_device()?
>
> Not all RMI devices will have access to interrupts (ie HID and SMBus).
> The same goes for regulators. Here is a reference to a previous
> discussion regarding both:
> https://lkml.org/lkml/2016/5/9/1055
>
>>
>> rmi_register_transport_device() doesn't take dev as parameter.
>>
>>> 2. If we need to use some non-devm-ised resources we should use
>>> devm_add_action[_or_reset] to work these operations into devm stream.
>>
>
> Since the regulator functions have their own devm_ versions I would
> suggest switching to those functions to avoid dealing with
> unregistering.
>
Maybe I am missing something, but I don't see a devm_regulator_bulk_enable().
devm_regulator_bulk_get() is already used.
Guenter
> Registering and unregistering the transport device is a bit more
> complicated since these functions add and put the rmi_dev device. But,
> it sounds like we can handle the unregister using
> devm_add_action_or_reset().
>
[toc] | [prev] | [next] | [standalone]
| From | Andrew Duggan <andrew@duggan.us> |
|---|---|
| Date | 2016-10-01 20:00 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <snwJP-3no-7@gated-at.bofh.it> |
| In reply to | #1494448 |
On Sat, Oct 1, 2016, at 10:45 AM, Guenter Roeck wrote:
> On 10/01/2016 10:27 AM, Andrew Duggan wrote:
> >
> >
> > On Fri, Sep 30, 2016, at 08:44 PM, Guenter Roeck wrote:
> >> On 09/30/2016 04:02 PM, Dmitry Torokhov wrote:
> >>> On Fri, Sep 30, 2016 at 03:54:03PM -0700, Dmitry Torokhov wrote:
> >>>> On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
> >>>>> On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
> >>>>>
> >>>>>> Instantiating the rmi4 I2C transport driver without interrupts assigned
> >>>>>> (for example using manual i2c instantiation from the command line)
> >>>>>> caused the driver to fail to load, but it does not clean up its
> >>>>>> regulator or transport device registrations. Result is a crash at a later
> >>>>>> time, for example when rebooting the system.
> >>>>>>
> >>>>>> Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
> >>>>>
> >>>>> Sorry for that.
> >>>>>
> >>>>>> Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
> >>>>>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>>>
> >>>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>>
> >>>> Applied, thank you.
> >>>
> >>> I take it back. rmi_i2c_init_irq() uses devm* so this whole thing mixes
> >>> up devm* and manual unregistering and unwind order is completely
> >>> broken.
> >>>
> >> Oops ...
> >>
> >>> 1. Why do we register interrupt from transport drivers and not make it
> >>> part of rmi_register_transport_device()?
> >
> > Not all RMI devices will have access to interrupts (ie HID and SMBus).
> > The same goes for regulators. Here is a reference to a previous
> > discussion regarding both:
> > https://lkml.org/lkml/2016/5/9/1055
> >
> >>
> >> rmi_register_transport_device() doesn't take dev as parameter.
> >>
> >>> 2. If we need to use some non-devm-ised resources we should use
> >>> devm_add_action[_or_reset] to work these operations into devm stream.
> >>
> >
> > Since the regulator functions have their own devm_ versions I would
> > suggest switching to those functions to avoid dealing with
> > unregistering.
> >
> Maybe I am missing something, but I don't see a
> devm_regulator_bulk_enable().
> devm_regulator_bulk_get() is already used.
>
Yeah, I was just double checking that too. You're right, they are
already using the devm version.
Andrew
> Guenter
>
> > Registering and unregistering the transport device is a bit more
> > complicated since these functions add and put the rmi_dev device. But,
> > it sounds like we can handle the unregister using
> > devm_add_action_or_reset().
> >
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2016-10-01 21:10 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <snxPz-4mk-5@gated-at.bofh.it> |
| In reply to | #1494446 |
On Sat, Oct 01, 2016 at 10:27:42AM -0700, Andrew Duggan wrote:
>
>
> On Fri, Sep 30, 2016, at 08:44 PM, Guenter Roeck wrote:
> > On 09/30/2016 04:02 PM, Dmitry Torokhov wrote:
> > > On Fri, Sep 30, 2016 at 03:54:03PM -0700, Dmitry Torokhov wrote:
> > >> On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
> > >>> On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
> > >>>
> > >>>> Instantiating the rmi4 I2C transport driver without interrupts assigned
> > >>>> (for example using manual i2c instantiation from the command line)
> > >>>> caused the driver to fail to load, but it does not clean up its
> > >>>> regulator or transport device registrations. Result is a crash at a later
> > >>>> time, for example when rebooting the system.
> > >>>>
> > >>>> Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
> > >>>
> > >>> Sorry for that.
> > >>>
> > >>>> Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
> > >>>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > >>>
> > >>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > >>
> > >> Applied, thank you.
> > >
> > > I take it back. rmi_i2c_init_irq() uses devm* so this whole thing mixes
> > > up devm* and manual unregistering and unwind order is completely
> > > broken.
> > >
> > Oops ...
> >
> > > 1. Why do we register interrupt from transport drivers and not make it
> > > part of rmi_register_transport_device()?
>
> Not all RMI devices will have access to interrupts (ie HID and SMBus).
> The same goes for regulators. Here is a reference to a previous
> discussion regarding both:
> https://lkml.org/lkml/2016/5/9/1055
Yeah, I am wondering if we should not revisit this and have SMBus (and
possibly HID) actually provide us with an interrupt.
In the meantime we can just ignore interrupt value if it is set to 0.
>
> >
> > rmi_register_transport_device() doesn't take dev as parameter.
> >
> > > 2. If we need to use some non-devm-ised resources we should use
> > > devm_add_action[_or_reset] to work these operations into devm stream.
> >
>
> Since the regulator functions have their own devm_ versions I would
> suggest switching to those functions to avoid dealing with
> unregistering.
There is no devm* API for enabling regulators, just acquiring them.
Thanks,
--
Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-10-01 22:50 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <snzol-5go-9@gated-at.bofh.it> |
| In reply to | #1494460 |
On 10/01/2016 12:04 PM, Dmitry Torokhov wrote:
> On Sat, Oct 01, 2016 at 10:27:42AM -0700, Andrew Duggan wrote:
>>
>>
>> On Fri, Sep 30, 2016, at 08:44 PM, Guenter Roeck wrote:
>>> On 09/30/2016 04:02 PM, Dmitry Torokhov wrote:
>>>> On Fri, Sep 30, 2016 at 03:54:03PM -0700, Dmitry Torokhov wrote:
>>>>> On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
>>>>>> On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
>>>>>>
>>>>>>> Instantiating the rmi4 I2C transport driver without interrupts assigned
>>>>>>> (for example using manual i2c instantiation from the command line)
>>>>>>> caused the driver to fail to load, but it does not clean up its
>>>>>>> regulator or transport device registrations. Result is a crash at a later
>>>>>>> time, for example when rebooting the system.
>>>>>>>
>>>>>>> Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
>>>>>>
>>>>>> Sorry for that.
>>>>>>
>>>>>>> Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
>>>>>>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>>>
>>>>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>>
>>>>> Applied, thank you.
>>>>
>>>> I take it back. rmi_i2c_init_irq() uses devm* so this whole thing mixes
>>>> up devm* and manual unregistering and unwind order is completely
>>>> broken.
>>>>
>>> Oops ...
>>>
>>>> 1. Why do we register interrupt from transport drivers and not make it
>>>> part of rmi_register_transport_device()?
>>
>> Not all RMI devices will have access to interrupts (ie HID and SMBus).
>> The same goes for regulators. Here is a reference to a previous
>> discussion regarding both:
>> https://lkml.org/lkml/2016/5/9/1055
>
> Yeah, I am wondering if we should not revisit this and have SMBus (and
> possibly HID) actually provide us with an interrupt.
>
> In the meantime we can just ignore interrupt value if it is set to 0.
>
I have another follow-up patch doing that (I have a test setup which
doesn't support interrupts); I just wasn't sure if there was interest,
so I did not yet send it out. I'll be happy to do that, though I would
prefer to keep it separate (it isn't a bug fix, after all).
Thanks,
Guenter
[toc] | [prev] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-10-01 05:10 +0200 |
| Subject | Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver |
| Message-ID | <sniQx-2AT-3@gated-at.bofh.it> |
| In reply to | #1494319 |
On 09/30/2016 03:54 PM, Dmitry Torokhov wrote:
> On Thu, Sep 29, 2016 at 10:55:40AM -0700, Bjorn Andersson wrote:
>> On Wed 28 Sep 17:37 PDT 2016, Guenter Roeck wrote:
>>
>>> Instantiating the rmi4 I2C transport driver without interrupts assigned
>>> (for example using manual i2c instantiation from the command line)
>>> caused the driver to fail to load, but it does not clean up its
>>> regulator or transport device registrations. Result is a crash at a later
>>> time, for example when rebooting the system.
>>>
>>> Fixes: 946c8432aab0 ("Input: synaptics-rmi4 - support regulator supplies")
>>
>> Sorry for that.
>>
>>> Fixes: fdf51604f104 ("Input: synaptics-rmi4 - add I2C transport driver")
>>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> Applied, thank you.
>
> It looks like we have similar issue in rmi_spi.c. Can I get another
> patch?
>
Sure.
Guenter
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web