Re: [PATCH] Input: synaptics-rmi4 - Fix error handling in I2C transport driver

From: Guenter Roeck
Date: Sat Oct 01 2016 - 13:46:44 EST

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@xxxxxxxxxx>

Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>

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

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:

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

Maybe I am missing something, but I don't see a devm_regulator_bulk_enable().
devm_regulator_bulk_get() is already used.


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