Re: [PATCH V9] i2c: i2c-qcom-geni: Add shutdown callback for i2c
From: <hidden>
Date: 2021-05-11 12:58:30
Also in:
linux-arm-msm, linux-media, lkml
On 2021-05-08 13:26, Stephen Boyd wrote:
Quoting Stephen Boyd (2021-05-07 13:09:21)quoted
Quoting rojay@codeaurora.org (2021-05-07 03:07:42)quoted
On 2021-05-05 07:08, Stephen Boyd wrote:quoted
Quoting Roja Rani Yarubandi (2021-04-20 04:13:55)quoted
In fact, where is that code? I'd expect to see i2c_del_adapter() in here so we know the adapter can't accept transfers anymore. Maybe i2c_del_adapter() could be called, and then there's nothing to do after that? This whole patch is trying to rip the adapter out from under the i2c core framework, when we should take the opposite approach and remove it from the core framework so that it can't transfer anything anymore and thus the IOMMU can remove the mapping.IIUC about probe/remove/shutdown calls, during "remove" we will unplug the device with opposite calls to "probe's" plug operations. For example i2c_add_adapter() from 'probe' and i2c_del_adapter() from 'remove'. For "shutdown", as system is going to shutdown, there is no need of unplug operations to be done. And also, I had a glance on other upstream i2c drivers, and noticed "i2c-i801.c" driver has i2c_del_adapter() called from remove callback but not from shutdown callback.Sure, other drivers could also be broken.What does it have in the shutdown callback? I see that it is wrong to delete the adapter in shutdown because this problem happens. First shutdown is called for various i2c clients, then shutdown is called for the adapter. If the adapter shutdown calls i2c_del_adapter(), then remove is called for the various i2c clients. The i2c clients aren't expecting this and start doing double frees and stuff. It's really quite a mess. I suspect i2c shutdown should probably block remove from being called on it entirely. Either way, it's the wrong approach. Instead, I think we should merely suspend the i2c bus like this. Then we can hunt down the various drivers that try to access the bus after the i2c bus has been removed. I've already done that for rt5682 (see the patch link later).
Ok. I will proceed with the current approach only then (not calling i2c_del_adapter() in shutdown). I will post the patch with the other comments answered.
quoted hunk ↗ jump to hunk
----8<---diff --git a/drivers/i2c/busses/i2c-qcom-geni.cb/drivers/i2c/busses/i2c-qcom-geni.c index 20216e382b4c..af3ed808ba2e 100644--- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c@@ -655,6 +655,14 @@ static int geni_i2c_remove(struct platform_device*pdev) return 0; } +static void geni_i2c_shutdown(struct platform_device *pdev) +{ + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev); + + /* Make client i2c transfers start failing */ + i2c_mark_adapter_suspended(&gi2c->adap); +} + static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) { int ret;@@ -719,6 +727,7 @@ MODULE_DEVICE_TABLE(of, geni_i2c_dt_match); static struct platform_driver geni_i2c_driver = { .probe = geni_i2c_probe, .remove = geni_i2c_remove, + .shutdown = geni_i2c_shutdown, .driver = { .name = "geni_i2c", .pm = &geni_i2c_pm_ops,quoted
quoted
And actually I tried calling i2c_del_adapter() from geni_i2c_shutdown(), and it resulted in below WARNING trace [ 90.320282] Call trace: [ 90.322807] _regulator_put+0xc4/0xcc [ 90.326583] regulator_bulk_free+0x48/0x6c [ 90.330808] devm_regulator_bulk_release+0x20/0x2c [ 90.335744] release_nodes+0x1d0/0x244 [ 90.339609] devres_release_all+0x3c/0x54 [ 90.343735] device_release_driver_internal+0x108/0x194 [ 90.349109] device_release_driver+0x24/0x30 [ 90.353510] bus_remove_device+0xd0/0xf4 [ 90.357548] device_del+0x1a8/0x2f8 [ 90.361143] device_unregister+0x1c/0x34 [ 90.365181] __unregister_client+0x78/0x88 [ 90.369397] device_for_each_child+0x64/0xb4 [ 90.373797] i2c_del_adapter+0xf0/0x1d4 [ 90.377745] geni_i2c_shutdown+0x9c/0xc0 [ 90.381783] platform_drv_shutdown+0x28/0x34 [ 90.386182] device_shutdown+0x148/0x1f0 Can you please suggest me what might be missing here?It looks like some device that is on the i2c bus is putting a regulator in the remove path without disabling it. Can you print out which device driver it is and fix that driver to call regulator_disable() on the driver remove path? I'll try locally and see if I can find the driver too.I see that it's the rt5682 driver. I sent https://lore.kernel.org/r/20210508075151.1626903-2-swboyd@chromium.org (local) for this in case you want to look, but it won't be necessary.