| Message ID | 20200522101327.13456-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | i2c: core: fix NULL pointer dereference in suspend/resume callbacks | expand |
Hi All, On 22.05.2020 12:13, Marek Szyprowski wrote: > Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in > adapter") added generic suspend and resume functions for i2c devices. > Those functions unconditionally access an i2c_client structure assigned > to the given i2c device. However, there exist i2c devices in the system > without a valid i2c_client. Add the needed check before accessing the > i2c_client. Just one more comment. The devices without i2c_client structure are the i2c 'devices' associated with the respective i2c bus. They are visible in /sys: ls -l /sys/bus/i2c/devices/i2c-* I wonder if this patch has been ever tested with system suspend/resume, as those devices are always available in the system... > ... Best regards
On Fri, May 22, 2020 at 01:15:12PM +0200, Marek Szyprowski wrote: > Hi All, > > On 22.05.2020 12:13, Marek Szyprowski wrote: > > Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in > > adapter") added generic suspend and resume functions for i2c devices. > > Those functions unconditionally access an i2c_client structure assigned > > to the given i2c device. However, there exist i2c devices in the system > > without a valid i2c_client. Add the needed check before accessing the > > i2c_client. > > Just one more comment. The devices without i2c_client structure are the > i2c 'devices' associated with the respective i2c bus. They are visible > in /sys: > > ls -l /sys/bus/i2c/devices/i2c-* > > I wonder if this patch has been ever tested with system suspend/resume, > as those devices are always available in the system... There was another issue with this patch. Although it is not clear yet, if the patch itself is the culprit or if it just unshadows something else, however, I am considering to just revert it until these issues are fixed.
Hi Marek, On Fri, May 22, 2020 at 1:15 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi All, > > On 22.05.2020 12:13, Marek Szyprowski wrote: > > Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in > > adapter") added generic suspend and resume functions for i2c devices. > > Those functions unconditionally access an i2c_client structure assigned > > to the given i2c device. However, there exist i2c devices in the system > > without a valid i2c_client. Add the needed check before accessing the > > i2c_client. > > Just one more comment. The devices without i2c_client structure are the > i2c 'devices' associated with the respective i2c bus. They are visible > in /sys: > > ls -l /sys/bus/i2c/devices/i2c-* > > I wonder if this patch has been ever tested with system suspend/resume, > as those devices are always available in the system... Sorry for the trouble and thanks a lot for the fix. We'll make sure to do more thorough testing, including suspend/resume before relanding this change. Since the patch was reverted, can we squash your fix with the next revision together with your Co-developed-by and Signed-off-by tags? Best regards, Tomasz
Hi Tomasz On 25.05.2020 14:28, Tomasz Figa wrote: > On Fri, May 22, 2020 at 1:15 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 22.05.2020 12:13, Marek Szyprowski wrote: >>> Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in >>> adapter") added generic suspend and resume functions for i2c devices. >>> Those functions unconditionally access an i2c_client structure assigned >>> to the given i2c device. However, there exist i2c devices in the system >>> without a valid i2c_client. Add the needed check before accessing the >>> i2c_client. >> Just one more comment. The devices without i2c_client structure are the >> i2c 'devices' associated with the respective i2c bus. They are visible >> in /sys: >> >> ls -l /sys/bus/i2c/devices/i2c-* >> >> I wonder if this patch has been ever tested with system suspend/resume, >> as those devices are always available in the system... > Sorry for the trouble and thanks a lot for the fix. We'll make sure to > do more thorough testing, including suspend/resume before relanding > this change. > > Since the patch was reverted, can we squash your fix with the next > revision together with your Co-developed-by and Signed-off-by tags? Sure, no problem. The fix is trivial. Best regards
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 5be24bf8a194..b531f5ad06b2 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -454,11 +454,13 @@ static int i2c_device_remove(struct device *dev) static int i2c_resume_early(struct device *dev) { struct i2c_client *client = i2c_verify_client(dev); - struct i2c_adapter *adap = client->adapter; int err; + if (!client) + return 0; + if (!pm_runtime_status_suspended(&client->dev)) { - err = regulator_enable(adap->bus_regulator); + err = regulator_enable(client->adapter->bus_regulator); if (err) return err; } @@ -469,15 +471,17 @@ static int i2c_resume_early(struct device *dev) static int i2c_suspend_late(struct device *dev) { struct i2c_client *client = i2c_verify_client(dev); - struct i2c_adapter *adap = client->adapter; int err; + if (!client) + return 0; + err = pm_generic_suspend_late(&client->dev); if (err) return err; if (!pm_runtime_status_suspended(&client->dev)) - return regulator_disable(adap->bus_regulator); + return regulator_disable(client->adapter->bus_regulator); return 0; } @@ -487,10 +491,12 @@ static int i2c_suspend_late(struct device *dev) static int i2c_runtime_resume(struct device *dev) { struct i2c_client *client = i2c_verify_client(dev); - struct i2c_adapter *adap = client->adapter; int err; - err = regulator_enable(adap->bus_regulator); + if (!client) + return 0; + + err = regulator_enable(client->adapter->bus_regulator); if (err) return err; @@ -500,14 +506,16 @@ static int i2c_runtime_resume(struct device *dev) static int i2c_runtime_suspend(struct device *dev) { struct i2c_client *client = i2c_verify_client(dev); - struct i2c_adapter *adap = client->adapter; int err; + if (!client) + return 0; + err = pm_generic_runtime_suspend(&client->dev); if (err) return err; - return regulator_disable(adap->bus_regulator); + return regulator_disable(client->adapter->bus_regulator); } #endif
Commit 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in adapter") added generic suspend and resume functions for i2c devices. Those functions unconditionally access an i2c_client structure assigned to the given i2c device. However, there exist i2c devices in the system without a valid i2c_client. Add the needed check before accessing the i2c_client. This fixes the following issue observed on Samsung Exynos4412-based Odroid U3 board: 8<--- cut here --- Unable to handle kernel NULL pointer dereference at virtual address 00000018 pgd = 2aed198a [00000018] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 1 PID: 1295 Comm: rtcwake Not tainted 5.7.0-rc6-02700-g4773d1324da6 #739 Hardware name: Samsung Exynos (Flattened Device Tree) PC is at i2c_suspend_late+0x20/0x48 LR is at dpm_run_callback+0xb4/0x3fc pc : [<c07b404c>] lr : [<c064b7cc>] psr: 20000053 ... Process rtcwake (pid: 1295, stack limit = 0x7f1885cf) Stack: (0xec8f3d70 to 0xec8f4000) ... [<c07b404c>] (i2c_suspend_late) from [<c064b7cc>] (dpm_run_callback+0xb4/0x3fc) [<c064b7cc>] (dpm_run_callback) from [<c064ce04>] (__device_suspend_late+0xcc/0x16c) [<c064ce04>] (__device_suspend_late) from [<c064f0b0>] (dpm_suspend_late+0x10c/0x568) [<c064f0b0>] (dpm_suspend_late) from [<c01996f0>] (suspend_devices_and_enter+0x31c/0xc70) [<c01996f0>] (suspend_devices_and_enter) from [<c019a43c>] (pm_suspend+0x3f8/0x480) [<c019a43c>] (pm_suspend) from [<c0198174>] (state_store+0x6c/0xc8) [<c0198174>] (state_store) from [<c035cf4c>] (kernfs_fop_write+0x10c/0x228) [<c035cf4c>] (kernfs_fop_write) from [<c02b94a4>] (__vfs_write+0x30/0x1d0) [<c02b94a4>] (__vfs_write) from [<c02bc444>] (vfs_write+0xa4/0x170) [<c02bc444>] (vfs_write) from [<c02bc690>] (ksys_write+0x60/0xd8) [<c02bc690>] (ksys_write) from [<c0100060>] (ret_fast_syscall+0x0/0x28) Exception stack(0xec8f3fa8 to 0xec8f3ff0) ... ---[ end trace a43afef431782f37 ]--- Fixes: 6fe12cdbcfe3 ("i2c: core: support bus regulator controlling in adapter") Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- This fixes suspend/resume issue observed on various board with linux-next from 20200521. --- drivers/i2c/i2c-core-base.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)