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@xxxxxxxxxxxxxxxxx>
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,
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.
Call after device_register() and injecting an error into the of_i2c_setup_smbus_alert()
also results in a kernel panic.
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?
--
Regards
Phil Reid
