Re: [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Jul 11 2017 or thereabouts, Phil Reid wrote:
> On 28/06/2017 20:45, Benjamin Tissoires wrote:
> > On Jun 28 2017 or thereabouts, Phil Reid wrote:
> > > On 23/06/2017 20:19, Benjamin Tissoires wrote:
> > > > On Jun 19 2017 or thereabouts, Wolfram Sang wrote:
> > > > > On Thu, Jun 15, 2017 at 09:59:33PM +0800, Phil Reid wrote:
> > > > > > Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
> > > > > > so the the smbalert driver can be registered.
> > > > > > 
> > > > > > Signed-off-by: Phil Reid <preid@electromag.com.au>
> > > > > 
> > > > > CCing Benjamin
> > > > > 
> > > > > > ---
> > > > > >    drivers/i2c/i2c-core.c | 4 ++++
> > > > > >    1 file changed, 4 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > > > index d2402bb..626471b 100644
> > > > > > --- a/drivers/i2c/i2c-core.c
> > > > > > +++ b/drivers/i2c/i2c-core.c
> > > > > > @@ -40,6 +40,7 @@
> > > > > >    #include <linux/gpio.h>
> > > > > >    #include <linux/hardirq.h>
> > > > > >    #include <linux/i2c.h>
> > > > > > +#include <linux/i2c-smbus.h>
> > > > > >    #include <linux/idr.h>
> > > > > >    #include <linux/init.h>
> > > > > >    #include <linux/irqflags.h>
> > > > > > @@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
> > > > > >    		dev_warn(&adap->dev,
> > > > > >    			 "Failed to create compatibility class link\n");
> > > > > >    #endif
> > > > > > +	res = of_i2c_setup_smbus_alert(adap);
> > > > > > +	if (res)
> > > > > > +		goto out_list;
> > > > 
> > > > See my concerns in patch 4/10.
> > > > 
> > > > In addition, shouldn't this be placed before device_register() for the
> > > > least? pm_runtime_enable() would require a matching pm_runtime_disable(),
> > > > and device_register() some unregistering behavior too.
> > > > 
> > > 
> > > G'day Ben,
> > > 
> > > Thanks for the review.
> > > Yes this makes sense. I tried having it before the device_register and I get an error
> > > about a kobject not being initialised in the a call from of_i2c_setup_smbus_alert.
> > > 
> > > Having a look at what I'm doing in of_i2c_setup_smbus_alert now I'm not sure there's
> > > a need to bail out on an error now. Originally I was registering the irq in the setup call.
> > > Which need to handle probe defer. Now this should be handled in the alert probe call.
> > > 
> > > WDYR?
> > 
> > Well, of_i2c_setup_smbus_alert() returns an error code, so it has to be
> > accounted for. If the error is not a reason to fail, you should at least
> > shout some error messages and act accordingly.
> > 
> > However, looking at of_i2c_setup_smbus_alert() again, I wonder if we
> > should not split it in 2: one part that checks for the OF flag presence
> > and bail out early if an error is encountered (before device
> > registration), and one part once the device is registered that calls
> > i2c_setup_smbus_alert(). In case of a failure here, we need to
> > unregister the adapter because we don't have all the elements at hands
> > (assuming we consider that smbus-alert should be mandatory).
> > 
> 
> G'day Benjamin,

Hi Phil,

[sorry for the lag, been busy with other topics + public holidays]

> 
> I've tried a couple of different ways of handling errors from of_i2c_setup_smbus_alert()
> and not having much success.
> 
> Call of_i2c_setup_smbus_alert() before the device_register() call results in a kernel panic.

Yeah, i2c_setup_smbus_alert() basically creates an I2C device, so you
need to have the adapter registered before you can call it.

> 
> Call after device_register() and injecting an error into the of_i2c_setup_smbus_alert()
> also results in a kernel panic.

I wonder if this is just not required at all to have a failure path that
will remove the smbusalert device. Because this is an I2C device, when
we unregister the adapter, it should properly remove all of its attached
devices.

> 
> On error I'm calling device_unregister()
> 
> I also tried spliting the device_unregister() call into
>   device_initialize(dev);
>   of_i2c_setup_smbus_alert()
>   device_add(dev);
> 
> but that results in a crash in of_i2c_setup_smbus_alert()
> 
> 
> If i2c_new_device() is not called then I get:
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
>  PC is at __wake_up_common+0x2c/0x90
> 
> [<8015d940>] (__wake_up_common) from [<8015d9c8>] (__wake_up_locked+0x24/0x2c)
> [<8015d9c8>] (__wake_up_locked) from [<8015e680>] (complete+0x48/0x58)
> [<8015e680>] (complete) from [<805ba6c0>] (i2c_adapter_dev_release+0x1c/0x20)
> [<805ba6c0>] (i2c_adapter_dev_release) from [<804d22f8>] (device_release+0x3c/0xa0)
> [<804d22f8>] (device_release) from [<80419ab0>] (kobject_put+0xac/0x208)
> [<80419ab0>] (kobject_put) from [<804d3778>] (device_unregister+0x64/0x70)
> [<804d3778>] (device_unregister) from [<805bb234>] (i2c_register_adapter+0x2a4/0x480)
> [<805bb234>] (i2c_register_adapter) from [<805bc5b4>] (i2c_add_adapter+0x8c/0xd0)
> [<805bc5b4>] (i2c_add_adapter) from [<805c0690>] (i2c_mux_add_adapter+0x278/0x428)
> [<805c0690>] (i2c_mux_add_adapter) from [<805c4b48>] (pca954x_probe+0x2b0/0x394)
> [<805c4b48>] (pca954x_probe) from [<805b9c44>] (i2c_device_probe+0x200/0x2dc)
> [<805b9c44>] (i2c_device_probe) from [<804d7e0c>] (driver_probe_device+0x338/0x460)
> [<804d7e0c>] (driver_probe_device) from [<804d814c>] (__device_attach_driver+0xa4/0x128)
> [<804d814c>] (__device_attach_driver) from [<804d5ad8>] (bus_for_each_drv+0x68/0x9c)
> [<804d5ad8>] (bus_for_each_drv) from [<804d792c>] (__device_attach+0xc0/0x14c)
> [<804d792c>] (__device_attach) from [<804d81ec>] (device_initial_probe+0x1c/0x20)
> [<804d81ec>] (device_initial_probe) from [<804d6cf4>] (bus_probe_device+0x94/0x9c)
> [<804d6cf4>] (bus_probe_device) from [<804d72dc>] (deferred_probe_work_func+0xa0/0xd4)
> [<804d72dc>] (deferred_probe_work_func) from [<8013d7f0>] (process_one_work+0x14c/0x4c4)
> [<8013d7f0>] (process_one_work) from [<8013dd94>] (worker_thread+0x22c/0x514)
> [<8013dd94>] (worker_thread) from [<80143a20>] (kthread+0x140/0x170)
> [<80143a20>] (kthread) from [<80108e38>] (ret_from_fork+0x14/0x3c)
> 
> And the system hangs.
> 
> 
> If i2c_new_device() is called and I return an error after I get:
> This one probably wouldn't happen.
> 
> WARNING: CPU: 0 PID: 70 at fs/proc/generic.c:580 remove_proc_entry+0x124/0x168
> remove_proc_entry: removing non-empty directory 'irq/193', leaking at least 'smbus_alert'
> Modules linked in:
> CPU: 0 PID: 70 Comm: kworker/0:2 Not tainted 4.12.0-rc6+ #141
> Hardware name: Altera SOCFPGA
> Workqueue: events deferred_probe_work_func
> [<801145a4>] (unwind_backtrace) from [<8010df44>] (show_stack+0x20/0x24)
> [<8010df44>] (show_stack) from [<80418308>] (dump_stack+0x8c/0xa0)
> [<80418308>] (dump_stack) from [<80123518>] (__warn+0xf8/0x110)
> [<80123518>] (__warn) from [<80123578>] (warn_slowpath_fmt+0x48/0x50)
> [<80123578>] (warn_slowpath_fmt) from [<802acfa0>] (remove_proc_entry+0x124/0x168)
> [<802acfa0>] (remove_proc_entry) from [<80174d38>] (unregister_irq_proc+0xac/0xb4)
> [<80174d38>] (unregister_irq_proc) from [<8016b404>] (free_desc+0x40/0x78)
> [<8016b404>] (free_desc) from [<8016b494>] (irq_free_descs+0x58/0x90)
> [<8016b494>] (irq_free_descs) from [<80174128>] (irq_dispose_mapping+0x54/0x7c)
> [<80174128>] (irq_dispose_mapping) from [<805c45ac>] (pca954x_cleanup+0x4c/0x70)
> [<805c45ac>] (pca954x_cleanup) from [<805c4ac0>] (pca954x_probe+0x210/0x394)
> [<805c4ac0>] (pca954x_probe) from [<805b9c44>] (i2c_device_probe+0x200/0x2dc)
> [<805b9c44>] (i2c_device_probe) from [<804d7e0c>] (driver_probe_device+0x338/0x460)
> [<804d7e0c>] (driver_probe_device) from [<804d814c>] (__device_attach_driver+0xa4/0x128)
> [<804d814c>] (__device_attach_driver) from [<804d5ad8>] (bus_for_each_drv+0x68/0x9c)
> [<804d5ad8>] (bus_for_each_drv) from [<804d792c>] (__device_attach+0xc0/0x14c)
> [<804d792c>] (__device_attach) from [<804d81ec>] (device_initial_probe+0x1c/0x20)
> [<804d81ec>] (device_initial_probe) from [<804d6cf4>] (bus_probe_device+0x94/0x9c)
> [<804d6cf4>] (bus_probe_device) from [<804d72dc>] (deferred_probe_work_func+0xa0/0xd4)
> [<804d72dc>] (deferred_probe_work_func) from [<8013d7f0>] (process_one_work+0x14c/0x4c4)
> [<8013d7f0>] (process_one_work) from [<8013dd94>] (worker_thread+0x22c/0x514)
> [<8013dd94>] (worker_thread) from [<80143a20>] (kthread+0x140/0x170)
> [<80143a20>] (kthread) from [<80108e38>] (ret_from_fork+0x14/0x3c)
> 
> 
> I'm at a real loss on how to safely handle the error from of_i2c_setup_smbus_alert()
> 
> Any hints as to what the correct way is to unregister the device?

I'd say try to register the smbus_alert device after the registering of
the adapter, and if it fails simply call the unregister from the
adapter. On remove, there might not need to do anything given that the
smbus_alert is an I2C client like every others.

Cheers,
Benjamin

PS: sorry if I am completely off-topic

> 
> 
> -- 
> Regards
> Phil Reid
>