| Message ID | 20190807203153.11778-1-festevam@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: imx: Fix endlessly clk warnings | expand |
On Wed, Aug 7, 2019 at 5:31 PM Fabio Estevam <festevam@gmail.com> wrote: > > When booting with the DMA driver as module (such as CONFIG_FSL_EDMA=m) > the following endlessly clk warnings are seen: > > [ 153.077831] ------------[ cut here ]------------ > [ 153.082528] WARNING: CPU: 0 PID: 15 at drivers/clk/clk.c:924 clk_core_disable_lock+0x18/0x24 > [ 153.093077] i2c0 already disabled > [ 153.096416] Modules linked in: > [ 153.099521] CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G W 5.2.0+ #321 > [ 153.107290] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree) > [ 153.113772] Workqueue: events deferred_probe_work_func > [ 153.118979] [<c0019560>] (unwind_backtrace) from [<c0014734>] (show_stack+0x10/0x14) > [ 153.126778] [<c0014734>] (show_stack) from [<c083f8dc>] (dump_stack+0x9c/0xd4) > [ 153.134051] [<c083f8dc>] (dump_stack) from [<c0031154>] (__warn+0xf8/0x124) > [ 153.141056] [<c0031154>] (__warn) from [<c0031248>] (warn_slowpath_fmt+0x38/0x48) > [ 153.148580] [<c0031248>] (warn_slowpath_fmt) from [<c040fde0>] (clk_core_disable_lock+0x18/0x24) > [ 153.157413] [<c040fde0>] (clk_core_disable_lock) from [<c058f520>] (i2c_imx_probe+0x554/0x6ec) > [ 153.166076] [<c058f520>] (i2c_imx_probe) from [<c04b9178>] (platform_drv_probe+0x48/0x98) > [ 153.174297] [<c04b9178>] (platform_drv_probe) from [<c04b7298>] (really_probe+0x1d8/0x2c0) > [ 153.182605] [<c04b7298>] (really_probe) from [<c04b7554>] (driver_probe_device+0x5c/0x174) > [ 153.190909] [<c04b7554>] (driver_probe_device) from [<c04b58c8>] (bus_for_each_drv+0x44/0x8c) > [ 153.199480] [<c04b58c8>] (bus_for_each_drv) from [<c04b746c>] (__device_attach+0xa0/0x108) > [ 153.207782] [<c04b746c>] (__device_attach) from [<c04b65a4>] (bus_probe_device+0x88/0x90) > [ 153.215999] [<c04b65a4>] (bus_probe_device) from [<c04b6a04>] (deferred_probe_work_func+0x60/0x90) > [ 153.225003] [<c04b6a04>] (deferred_probe_work_func) from [<c004f190>] (process_one_work+0x204/0x634) > [ 153.234178] [<c004f190>] (process_one_work) from [<c004f618>] (worker_thread+0x20/0x484) > [ 153.242315] [<c004f618>] (worker_thread) from [<c0055c2c>] (kthread+0x118/0x150) > [ 153.249758] [<c0055c2c>] (kthread) from [<c00090b4>] (ret_from_fork+0x14/0x20) > [ 153.257006] Exception stack(0xdde43fb0 to 0xdde43ff8) > [ 153.262095] 3fa0: 00000000 00000000 00000000 00000000 > [ 153.270306] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > [ 153.278520] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > [ 153.285159] irq event stamp: 3323022 > [ 153.288787] hardirqs last enabled at (3323021): [<c0861c4c>] _raw_spin_unlock_irq+0x24/0x2c > [ 153.297261] hardirqs last disabled at (3323022): [<c040d7a0>] clk_enable_lock+0x10/0x124 > [ 153.305392] softirqs last enabled at (3322092): [<c000a504>] __do_softirq+0x344/0x540 > [ 153.313352] softirqs last disabled at (3322081): [<c00385c0>] irq_exit+0x10c/0x128 > [ 153.320946] ---[ end trace a506731ccd9bd703 ]--- > > When the DMA driver is built as module then dma_request_chan() returns > -EPROBE_DEFER, so let's treat this case in the same way as > "there is no DMA support" and make i2c_imx_dma_request() return success. > > Reported-by: Russell King <linux@armlinux.org.uk> > Fixes: e1ab9a468e3b ("i2c: imx: improve the error handling in i2c_imx_dma_request()") > Signed-off-by: Fabio Estevam <festevam@gmail.com> Just learned that Andrey has already submitted a fix for this same problem: https://lore.kernel.org/linux-arm-kernel/20190609055658.3446-1-andrew.smirnov@gmail.com/ I think we can go with Andrey's solution. Maybe Andrey could resend it with the Fixes tag so that it can be backported to stable. Thanks
On Wed, Aug 7, 2019 at 2:00 PM Fabio Estevam <festevam@gmail.com> wrote: > > On Wed, Aug 7, 2019 at 5:31 PM Fabio Estevam <festevam@gmail.com> wrote: > > > > When booting with the DMA driver as module (such as CONFIG_FSL_EDMA=m) > > the following endlessly clk warnings are seen: > > > > [ 153.077831] ------------[ cut here ]------------ > > [ 153.082528] WARNING: CPU: 0 PID: 15 at drivers/clk/clk.c:924 clk_core_disable_lock+0x18/0x24 > > [ 153.093077] i2c0 already disabled > > [ 153.096416] Modules linked in: > > [ 153.099521] CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G W 5.2.0+ #321 > > [ 153.107290] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree) > > [ 153.113772] Workqueue: events deferred_probe_work_func > > [ 153.118979] [<c0019560>] (unwind_backtrace) from [<c0014734>] (show_stack+0x10/0x14) > > [ 153.126778] [<c0014734>] (show_stack) from [<c083f8dc>] (dump_stack+0x9c/0xd4) > > [ 153.134051] [<c083f8dc>] (dump_stack) from [<c0031154>] (__warn+0xf8/0x124) > > [ 153.141056] [<c0031154>] (__warn) from [<c0031248>] (warn_slowpath_fmt+0x38/0x48) > > [ 153.148580] [<c0031248>] (warn_slowpath_fmt) from [<c040fde0>] (clk_core_disable_lock+0x18/0x24) > > [ 153.157413] [<c040fde0>] (clk_core_disable_lock) from [<c058f520>] (i2c_imx_probe+0x554/0x6ec) > > [ 153.166076] [<c058f520>] (i2c_imx_probe) from [<c04b9178>] (platform_drv_probe+0x48/0x98) > > [ 153.174297] [<c04b9178>] (platform_drv_probe) from [<c04b7298>] (really_probe+0x1d8/0x2c0) > > [ 153.182605] [<c04b7298>] (really_probe) from [<c04b7554>] (driver_probe_device+0x5c/0x174) > > [ 153.190909] [<c04b7554>] (driver_probe_device) from [<c04b58c8>] (bus_for_each_drv+0x44/0x8c) > > [ 153.199480] [<c04b58c8>] (bus_for_each_drv) from [<c04b746c>] (__device_attach+0xa0/0x108) > > [ 153.207782] [<c04b746c>] (__device_attach) from [<c04b65a4>] (bus_probe_device+0x88/0x90) > > [ 153.215999] [<c04b65a4>] (bus_probe_device) from [<c04b6a04>] (deferred_probe_work_func+0x60/0x90) > > [ 153.225003] [<c04b6a04>] (deferred_probe_work_func) from [<c004f190>] (process_one_work+0x204/0x634) > > [ 153.234178] [<c004f190>] (process_one_work) from [<c004f618>] (worker_thread+0x20/0x484) > > [ 153.242315] [<c004f618>] (worker_thread) from [<c0055c2c>] (kthread+0x118/0x150) > > [ 153.249758] [<c0055c2c>] (kthread) from [<c00090b4>] (ret_from_fork+0x14/0x20) > > [ 153.257006] Exception stack(0xdde43fb0 to 0xdde43ff8) > > [ 153.262095] 3fa0: 00000000 00000000 00000000 00000000 > > [ 153.270306] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > > [ 153.278520] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > [ 153.285159] irq event stamp: 3323022 > > [ 153.288787] hardirqs last enabled at (3323021): [<c0861c4c>] _raw_spin_unlock_irq+0x24/0x2c > > [ 153.297261] hardirqs last disabled at (3323022): [<c040d7a0>] clk_enable_lock+0x10/0x124 > > [ 153.305392] softirqs last enabled at (3322092): [<c000a504>] __do_softirq+0x344/0x540 > > [ 153.313352] softirqs last disabled at (3322081): [<c00385c0>] irq_exit+0x10c/0x128 > > [ 153.320946] ---[ end trace a506731ccd9bd703 ]--- > > > > When the DMA driver is built as module then dma_request_chan() returns > > -EPROBE_DEFER, so let's treat this case in the same way as > > "there is no DMA support" and make i2c_imx_dma_request() return success. > > > > Reported-by: Russell King <linux@armlinux.org.uk> > > Fixes: e1ab9a468e3b ("i2c: imx: improve the error handling in i2c_imx_dma_request()") > > Signed-off-by: Fabio Estevam <festevam@gmail.com> > > Just learned that Andrey has already submitted a fix for this same problem: > https://lore.kernel.org/linux-arm-kernel/20190609055658.3446-1-andrew.smirnov@gmail.com/ > > I think we can go with Andrey's solution. > > Maybe Andrey could resend it with the Fixes tag so that it can be > backported to stable. > Sure, will do. Thanks, Andrey Smirnov
On Wed, Aug 7, 2019 at 6:02 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > Maybe Andrey could resend it with the Fixes tag so that it can be > > backported to stable. > > > > Sure, will do. When you resend it, please add: Tested-by: Fabio Estevam <festevam@gmail.com> Thanks
On Wed, Aug 07, 2019 at 06:00:44PM -0300, Fabio Estevam wrote: > On Wed, Aug 7, 2019 at 5:31 PM Fabio Estevam <festevam@gmail.com> wrote: > > > > When booting with the DMA driver as module (such as CONFIG_FSL_EDMA=m) > > the following endlessly clk warnings are seen: > > > > [ 153.077831] ------------[ cut here ]------------ > > [ 153.082528] WARNING: CPU: 0 PID: 15 at drivers/clk/clk.c:924 clk_core_disable_lock+0x18/0x24 > > [ 153.093077] i2c0 already disabled > > [ 153.096416] Modules linked in: > > [ 153.099521] CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G W 5.2.0+ #321 > > [ 153.107290] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree) > > [ 153.113772] Workqueue: events deferred_probe_work_func > > [ 153.118979] [<c0019560>] (unwind_backtrace) from [<c0014734>] (show_stack+0x10/0x14) > > [ 153.126778] [<c0014734>] (show_stack) from [<c083f8dc>] (dump_stack+0x9c/0xd4) > > [ 153.134051] [<c083f8dc>] (dump_stack) from [<c0031154>] (__warn+0xf8/0x124) > > [ 153.141056] [<c0031154>] (__warn) from [<c0031248>] (warn_slowpath_fmt+0x38/0x48) > > [ 153.148580] [<c0031248>] (warn_slowpath_fmt) from [<c040fde0>] (clk_core_disable_lock+0x18/0x24) > > [ 153.157413] [<c040fde0>] (clk_core_disable_lock) from [<c058f520>] (i2c_imx_probe+0x554/0x6ec) > > [ 153.166076] [<c058f520>] (i2c_imx_probe) from [<c04b9178>] (platform_drv_probe+0x48/0x98) > > [ 153.174297] [<c04b9178>] (platform_drv_probe) from [<c04b7298>] (really_probe+0x1d8/0x2c0) > > [ 153.182605] [<c04b7298>] (really_probe) from [<c04b7554>] (driver_probe_device+0x5c/0x174) > > [ 153.190909] [<c04b7554>] (driver_probe_device) from [<c04b58c8>] (bus_for_each_drv+0x44/0x8c) > > [ 153.199480] [<c04b58c8>] (bus_for_each_drv) from [<c04b746c>] (__device_attach+0xa0/0x108) > > [ 153.207782] [<c04b746c>] (__device_attach) from [<c04b65a4>] (bus_probe_device+0x88/0x90) > > [ 153.215999] [<c04b65a4>] (bus_probe_device) from [<c04b6a04>] (deferred_probe_work_func+0x60/0x90) > > [ 153.225003] [<c04b6a04>] (deferred_probe_work_func) from [<c004f190>] (process_one_work+0x204/0x634) > > [ 153.234178] [<c004f190>] (process_one_work) from [<c004f618>] (worker_thread+0x20/0x484) > > [ 153.242315] [<c004f618>] (worker_thread) from [<c0055c2c>] (kthread+0x118/0x150) > > [ 153.249758] [<c0055c2c>] (kthread) from [<c00090b4>] (ret_from_fork+0x14/0x20) > > [ 153.257006] Exception stack(0xdde43fb0 to 0xdde43ff8) > > [ 153.262095] 3fa0: 00000000 00000000 00000000 00000000 > > [ 153.270306] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > > [ 153.278520] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > [ 153.285159] irq event stamp: 3323022 > > [ 153.288787] hardirqs last enabled at (3323021): [<c0861c4c>] _raw_spin_unlock_irq+0x24/0x2c > > [ 153.297261] hardirqs last disabled at (3323022): [<c040d7a0>] clk_enable_lock+0x10/0x124 > > [ 153.305392] softirqs last enabled at (3322092): [<c000a504>] __do_softirq+0x344/0x540 > > [ 153.313352] softirqs last disabled at (3322081): [<c00385c0>] irq_exit+0x10c/0x128 > > [ 153.320946] ---[ end trace a506731ccd9bd703 ]--- > > > > When the DMA driver is built as module then dma_request_chan() returns > > -EPROBE_DEFER, so let's treat this case in the same way as > > "there is no DMA support" and make i2c_imx_dma_request() return success. > > > > Reported-by: Russell King <linux@armlinux.org.uk> > > Fixes: e1ab9a468e3b ("i2c: imx: improve the error handling in i2c_imx_dma_request()") > > Signed-off-by: Fabio Estevam <festevam@gmail.com> > > Just learned that Andrey has already submitted a fix for this same problem: > https://lore.kernel.org/linux-arm-kernel/20190609055658.3446-1-andrew.smirnov@gmail.com/ > > I think we can go with Andrey's solution. > > Maybe Andrey could resend it with the Fixes tag so that it can be > backported to stable. Well, the question is what behaviour do we want. The driver's original behaviour prior to it breaking was to ignore DMA if it wasn't found (e.g. when the DMA driver is a module.) Your patch restores that behaviour, which is the safest approach. Audrey's patch has the effect that none of the I2C buses will be registered if the DMA driver is a module, until the DMA driver is loaded. Since an I2C bus _may_ have the systems RTC on it, not probing the I2C bus will cause the RTC not to be read by the kernel at boot, leading to the system booting with a unixtime of 0. I haven't checked what the situation is wrt RTCs on the ZII boards, whether they do indeed have external I2C RTCs on them.
Hi Russell, On Wed, Aug 7, 2019 at 6:13 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > Well, the question is what behaviour do we want. The driver's original > behaviour prior to it breaking was to ignore DMA if it wasn't found > (e.g. when the DMA driver is a module.) > > Your patch restores that behaviour, which is the safest approach. Thanks for the feedback. > Audrey's patch has the effect that none of the I2C buses will be > registered if the DMA driver is a module, until the DMA driver is > loaded. Since an I2C bus _may_ have the systems RTC on it, not > probing the I2C bus will cause the RTC not to be read by the kernel > at boot, leading to the system booting with a unixtime of 0. That's a very good point. > I haven't checked what the situation is wrt RTCs on the ZII boards, > whether they do indeed have external I2C RTCs on them. Yes, they do have external I2C RTCs. Please check this commit for example: 09fc0dacc ("ARM: dts: imx6qdl-zii-rdu2: Disable the internal RTC") If Wolfram agrees, I think we can go with my patch then. Thanks
On Wed, Aug 7, 2019 at 7:15 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Russell, > > On Wed, Aug 7, 2019 at 6:13 PM Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > Well, the question is what behaviour do we want. The driver's original > > behaviour prior to it breaking was to ignore DMA if it wasn't found > > (e.g. when the DMA driver is a module.) > > > > Your patch restores that behaviour, which is the safest approach. > > Thanks for the feedback. > > > Audrey's patch has the effect that none of the I2C buses will be > > registered if the DMA driver is a module, until the DMA driver is > > loaded. Since an I2C bus _may_ have the systems RTC on it, not > > probing the I2C bus will cause the RTC not to be read by the kernel > > at boot, leading to the system booting with a unixtime of 0. > > That's a very good point. > > > I haven't checked what the situation is wrt RTCs on the ZII boards, > > whether they do indeed have external I2C RTCs on them. > > Yes, they do have external I2C RTCs. Please check this commit for example: > 09fc0dacc ("ARM: dts: imx6qdl-zii-rdu2: Disable the internal RTC") And here is another one for the vf610-zii: 8da0af5d6d39 ("ARM: dts: vf610-zii: Disable SNVS RTC") > If Wolfram agrees, I think we can go with my patch then. > > Thanks
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk> > Sent: Thursday, August 8, 2019 5:13 AM > > On Wed, Aug 07, 2019 at 06:00:44PM -0300, Fabio Estevam wrote: > > On Wed, Aug 7, 2019 at 5:31 PM Fabio Estevam <festevam@gmail.com> > wrote: > > > > > > When booting with the DMA driver as module (such as > > > CONFIG_FSL_EDMA=m) the following endlessly clk warnings are seen: > > > > > > [ 153.077831] ------------[ cut here ]------------ [ 153.082528] > > > WARNING: CPU: 0 PID: 15 at drivers/clk/clk.c:924 > > > clk_core_disable_lock+0x18/0x24 [ 153.093077] i2c0 already disabled > > > [ 153.096416] Modules linked in: > > > [ 153.099521] CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G W > 5.2.0+ #321 > > > [ 153.107290] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device > > > Tree) [ 153.113772] Workqueue: events deferred_probe_work_func [ > > > 153.118979] [<c0019560>] (unwind_backtrace) from [<c0014734>] > > > (show_stack+0x10/0x14) [ 153.126778] [<c0014734>] (show_stack) from > > > [<c083f8dc>] (dump_stack+0x9c/0xd4) [ 153.134051] [<c083f8dc>] > > > (dump_stack) from [<c0031154>] (__warn+0xf8/0x124) [ 153.141056] > > > [<c0031154>] (__warn) from [<c0031248>] > > > (warn_slowpath_fmt+0x38/0x48) [ 153.148580] [<c0031248>] > > > (warn_slowpath_fmt) from [<c040fde0>] > > > (clk_core_disable_lock+0x18/0x24) [ 153.157413] [<c040fde0>] > > > (clk_core_disable_lock) from [<c058f520>] > > > (i2c_imx_probe+0x554/0x6ec) [ 153.166076] [<c058f520>] > > > (i2c_imx_probe) from [<c04b9178>] (platform_drv_probe+0x48/0x98) [ > > > 153.174297] [<c04b9178>] (platform_drv_probe) from [<c04b7298>] > > > (really_probe+0x1d8/0x2c0) [ 153.182605] [<c04b7298>] > > > (really_probe) from [<c04b7554>] (driver_probe_device+0x5c/0x174) [ > > > 153.190909] [<c04b7554>] (driver_probe_device) from [<c04b58c8>] > > > (bus_for_each_drv+0x44/0x8c) [ 153.199480] [<c04b58c8>] > > > (bus_for_each_drv) from [<c04b746c>] (__device_attach+0xa0/0x108) [ > > > 153.207782] [<c04b746c>] (__device_attach) from [<c04b65a4>] > > > (bus_probe_device+0x88/0x90) [ 153.215999] [<c04b65a4>] > > > (bus_probe_device) from [<c04b6a04>] > > > (deferred_probe_work_func+0x60/0x90) > > > [ 153.225003] [<c04b6a04>] (deferred_probe_work_func) from > > > [<c004f190>] (process_one_work+0x204/0x634) [ 153.234178] > > > [<c004f190>] (process_one_work) from [<c004f618>] > > > (worker_thread+0x20/0x484) [ 153.242315] [<c004f618>] > > > (worker_thread) from [<c0055c2c>] (kthread+0x118/0x150) [ 153.249758] > [<c0055c2c>] (kthread) from [<c00090b4>] (ret_from_fork+0x14/0x20) > [ 153.257006] Exception stack(0xdde43fb0 to 0xdde43ff8) > > > [ 153.262095] 3fa0: 00000000 > 00000000 00000000 00000000 > > > [ 153.270306] 3fc0: 00000000 00000000 00000000 00000000 > 00000000 > > > 00000000 00000000 00000000 [ 153.278520] 3fe0: 00000000 > 00000000 > > > 00000000 00000000 00000013 00000000 [ 153.285159] irq event > stamp: > > > 3323022 [ 153.288787] hardirqs last enabled at (3323021): > > > [<c0861c4c>] _raw_spin_unlock_irq+0x24/0x2c [ 153.297261] hardirqs > > > last disabled at (3323022): [<c040d7a0>] clk_enable_lock+0x10/0x124 > > > [ 153.305392] softirqs last enabled at (3322092): [<c000a504>] > > > __do_softirq+0x344/0x540 [ 153.313352] softirqs last disabled at > > > (3322081): [<c00385c0>] irq_exit+0x10c/0x128 [ 153.320946] ---[ end > > > trace a506731ccd9bd703 ]--- > > > > > > When the DMA driver is built as module then dma_request_chan() > > > returns -EPROBE_DEFER, so let's treat this case in the same way as > > > "there is no DMA support" and make i2c_imx_dma_request() return > success. > > > > > > Reported-by: Russell King <linux@armlinux.org.uk> > > > Fixes: e1ab9a468e3b ("i2c: imx: improve the error handling in > > > i2c_imx_dma_request()") > > > Signed-off-by: Fabio Estevam <festevam@gmail.com> > > > > Just learned that Andrey has already submitted a fix for this same problem: > > > > I think we can go with Andrey's solution. > > > > Maybe Andrey could resend it with the Fixes tag so that it can be > > backported to stable. > > Well, the question is what behaviour do we want. The driver's original > behaviour prior to it breaking was to ignore DMA if it wasn't found (e.g. when > the DMA driver is a module.) > > Your patch restores that behaviour, which is the safest approach. > > Audrey's patch has the effect that none of the I2C buses will be registered if > the DMA driver is a module, until the DMA driver is loaded. Since an I2C bus > _may_ have the systems RTC on it, not probing the I2C bus will cause the RTC > not to be read by the kernel at boot, leading to the system booting with a > unixtime of 0. > > I haven't checked what the situation is wrt RTCs on the ZII boards, whether > they do indeed have external I2C RTCs on them. > I think the problem of this patch is that it breaks the defer probe of DMA enablement which seems like a normal case in kernel. e.g. DMA driver probed later than I2C. I wonder if there's a real customer requirement that wanting the I2C slave devices (e.g. RTC) to be probed early but using I2C with DMA and build DMA as module. If there is that requirement, maybe the correct way to fix this issue to explicitly disable I2C DMA in DT rather than simply fallback to polling mode for all the cases including the reasonable defer probe. Regards Aisheng > -- > RMK's Patch system: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar > mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=02%7C01%7Caisheng. > dong%40nxp.com%7Cd05089effd2a4cd9513d08d71b7c1779%7C686ea1d3bc > 2b4c6fa92cd99c5c301635%7C0%7C0%7C637008092101104779&sdata= > bc0BH%2Bn6O20a%2FiIh1328o1M%2Bl9%2BNM16PVxhuVXzIsIc%3D&re > served=0 > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps > up According to speedtest.net: 11.9Mbps down 500kbps up
On Thu, Aug 08, 2019 at 11:02:40AM +0000, Aisheng Dong wrote: > > From: Russell King - ARM Linux admin <linux@armlinux.org.uk> > > Sent: Thursday, August 8, 2019 5:13 AM > > > > On Wed, Aug 07, 2019 at 06:00:44PM -0300, Fabio Estevam wrote: > > > On Wed, Aug 7, 2019 at 5:31 PM Fabio Estevam <festevam@gmail.com> > > wrote: > > > > > > > > When booting with the DMA driver as module (such as > > > > CONFIG_FSL_EDMA=m) the following endlessly clk warnings are seen: > > > > > > > > [ 153.077831] ------------[ cut here ]------------ [ 153.082528] > > > > WARNING: CPU: 0 PID: 15 at drivers/clk/clk.c:924 > > > > clk_core_disable_lock+0x18/0x24 [ 153.093077] i2c0 already disabled > > > > [ 153.096416] Modules linked in: > > > > [ 153.099521] CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G W > > 5.2.0+ #321 > > > > [ 153.107290] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device > > > > Tree) [ 153.113772] Workqueue: events deferred_probe_work_func [ > > > > 153.118979] [<c0019560>] (unwind_backtrace) from [<c0014734>] > > > > (show_stack+0x10/0x14) [ 153.126778] [<c0014734>] (show_stack) from > > > > [<c083f8dc>] (dump_stack+0x9c/0xd4) [ 153.134051] [<c083f8dc>] > > > > (dump_stack) from [<c0031154>] (__warn+0xf8/0x124) [ 153.141056] > > > > [<c0031154>] (__warn) from [<c0031248>] > > > > (warn_slowpath_fmt+0x38/0x48) [ 153.148580] [<c0031248>] > > > > (warn_slowpath_fmt) from [<c040fde0>] > > > > (clk_core_disable_lock+0x18/0x24) [ 153.157413] [<c040fde0>] > > > > (clk_core_disable_lock) from [<c058f520>] > > > > (i2c_imx_probe+0x554/0x6ec) [ 153.166076] [<c058f520>] > > > > (i2c_imx_probe) from [<c04b9178>] (platform_drv_probe+0x48/0x98) [ > > > > 153.174297] [<c04b9178>] (platform_drv_probe) from [<c04b7298>] > > > > (really_probe+0x1d8/0x2c0) [ 153.182605] [<c04b7298>] > > > > (really_probe) from [<c04b7554>] (driver_probe_device+0x5c/0x174) [ > > > > 153.190909] [<c04b7554>] (driver_probe_device) from [<c04b58c8>] > > > > (bus_for_each_drv+0x44/0x8c) [ 153.199480] [<c04b58c8>] > > > > (bus_for_each_drv) from [<c04b746c>] (__device_attach+0xa0/0x108) [ > > > > 153.207782] [<c04b746c>] (__device_attach) from [<c04b65a4>] > > > > (bus_probe_device+0x88/0x90) [ 153.215999] [<c04b65a4>] > > > > (bus_probe_device) from [<c04b6a04>] > > > > (deferred_probe_work_func+0x60/0x90) > > > > [ 153.225003] [<c04b6a04>] (deferred_probe_work_func) from > > > > [<c004f190>] (process_one_work+0x204/0x634) [ 153.234178] > > > > [<c004f190>] (process_one_work) from [<c004f618>] > > > > (worker_thread+0x20/0x484) [ 153.242315] [<c004f618>] > > > > (worker_thread) from [<c0055c2c>] (kthread+0x118/0x150) [ 153.249758] > > [<c0055c2c>] (kthread) from [<c00090b4>] (ret_from_fork+0x14/0x20) > > [ 153.257006] Exception stack(0xdde43fb0 to 0xdde43ff8) > > > > [ 153.262095] 3fa0: 00000000 > > 00000000 00000000 00000000 > > > > [ 153.270306] 3fc0: 00000000 00000000 00000000 00000000 > > 00000000 > > > > 00000000 00000000 00000000 [ 153.278520] 3fe0: 00000000 > > 00000000 > > > > 00000000 00000000 00000013 00000000 [ 153.285159] irq event > > stamp: > > > > 3323022 [ 153.288787] hardirqs last enabled at (3323021): > > > > [<c0861c4c>] _raw_spin_unlock_irq+0x24/0x2c [ 153.297261] hardirqs > > > > last disabled at (3323022): [<c040d7a0>] clk_enable_lock+0x10/0x124 > > > > [ 153.305392] softirqs last enabled at (3322092): [<c000a504>] > > > > __do_softirq+0x344/0x540 [ 153.313352] softirqs last disabled at > > > > (3322081): [<c00385c0>] irq_exit+0x10c/0x128 [ 153.320946] ---[ end > > > > trace a506731ccd9bd703 ]--- > > > > > > > > When the DMA driver is built as module then dma_request_chan() > > > > returns -EPROBE_DEFER, so let's treat this case in the same way as > > > > "there is no DMA support" and make i2c_imx_dma_request() return > > success. > > > > > > > > Reported-by: Russell King <linux@armlinux.org.uk> > > > > Fixes: e1ab9a468e3b ("i2c: imx: improve the error handling in > > > > i2c_imx_dma_request()") > > > > Signed-off-by: Fabio Estevam <festevam@gmail.com> > > > > > > Just learned that Andrey has already submitted a fix for this same problem: > > > > > > I think we can go with Andrey's solution. > > > > > > Maybe Andrey could resend it with the Fixes tag so that it can be > > > backported to stable. > > > > Well, the question is what behaviour do we want. The driver's original > > behaviour prior to it breaking was to ignore DMA if it wasn't found (e.g. when > > the DMA driver is a module.) > > > > Your patch restores that behaviour, which is the safest approach. > > > > Audrey's patch has the effect that none of the I2C buses will be registered if > > the DMA driver is a module, until the DMA driver is loaded. Since an I2C bus > > _may_ have the systems RTC on it, not probing the I2C bus will cause the RTC > > not to be read by the kernel at boot, leading to the system booting with a > > unixtime of 0. > > > > I haven't checked what the situation is wrt RTCs on the ZII boards, whether > > they do indeed have external I2C RTCs on them. > > > > I think the problem of this patch is that it breaks the defer probe of DMA enablement > which seems like a normal case in kernel. e.g. DMA driver probed later than I2C. > > I wonder if there's a real customer requirement that wanting the I2C slave devices (e.g. RTC) > to be probed early but using I2C with DMA and build DMA as module. > > If there is that requirement, maybe the correct way to fix this issue to explicitly disable I2C DMA > in DT rather than simply fallback to polling mode for all the cases including the reasonable defer probe. This sounds like a more in-depth fix than what we should be doing for stable trees. Can we revert the original patch and restore the old behaviour, then propose a proper feature enhancement that doesn't regress existing setups?
Hi Russell, On Thu, Aug 8, 2019 at 8:11 AM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > This sounds like a more in-depth fix than what we should be doing for > stable trees. Can we revert the original patch and restore the old > behaviour, then propose a proper feature enhancement that doesn't > regress existing setups? Yes, I will send a revert patch. Thanks
Hi Aisheng, On Thu, Aug 8, 2019 at 8:02 AM Aisheng Dong <aisheng.dong@nxp.com> wrote: > I think the problem of this patch is that it breaks the defer probe of DMA enablement > which seems like a normal case in kernel. e.g. DMA driver probed later than I2C. Please note that prior to commit e1ab9a468e3b ("i2c: imx: improve the error handling in i2c_imx_dma_request()") there was no DMA defer probe support at all, so the path I sent does not break defer probe. It just restores the original behaviour. > I wonder if there's a real customer requirement that wanting the I2C slave devices (e.g. RTC) > to be probed early but using I2C with DMA and build DMA as module. > > If there is that requirement, maybe the correct way to fix this issue to explicitly disable I2C DMA > in DT rather than simply fallback to polling mode for all the cases including the reasonable defer probe. We should try to avoid fixing things by changing existing devicetrees. I will submit a revert patch soon.
> This sounds like a more in-depth fix than what we should be doing for > stable trees. Can we revert the original patch and restore the old > behaviour, then propose a proper feature enhancement that doesn't > regress existing setups? Yes, that. And since some interested parties have gathered now, I start yet another call for a maintainer for this driver. It is really needed, I'd say.
On Thu, Aug 08, 2019 at 10:07:28PM +0200, Wolfram Sang wrote: > > > This sounds like a more in-depth fix than what we should be doing for > > stable trees. Can we revert the original patch and restore the old > > behaviour, then propose a proper feature enhancement that doesn't > > regress existing setups? > > Yes, that. > > And since some interested parties have gathered now, I start yet another > call for a maintainer for this driver. It is really needed, I'd say. > If no one against it, I will take this task.
> > And since some interested parties have gathered now, I start yet another > > call for a maintainer for this driver. It is really needed, I'd say. > > > > If no one against it, I will take this task. Awesome, thanks! Given I am looking for a maintainer for a while now, I'd be much surprised if there is someone against it. I suggest you send a patch adding you to MAINTAINERS and let people comment there.
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index b1b8b938d7f4..0a07daeafdf2 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -337,7 +337,7 @@ static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, fail_al: devm_kfree(dev, dma); /* return successfully if there is no dma support */ - return ret == -ENODEV ? 0 : ret; + return ((ret == -ENODEV || ret == -EPROBE_DEFER)) ? 0 : ret; } static void i2c_imx_dma_callback(void *arg)
When booting with the DMA driver as module (such as CONFIG_FSL_EDMA=m) the following endlessly clk warnings are seen: [ 153.077831] ------------[ cut here ]------------ [ 153.082528] WARNING: CPU: 0 PID: 15 at drivers/clk/clk.c:924 clk_core_disable_lock+0x18/0x24 [ 153.093077] i2c0 already disabled [ 153.096416] Modules linked in: [ 153.099521] CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G W 5.2.0+ #321 [ 153.107290] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree) [ 153.113772] Workqueue: events deferred_probe_work_func [ 153.118979] [<c0019560>] (unwind_backtrace) from [<c0014734>] (show_stack+0x10/0x14) [ 153.126778] [<c0014734>] (show_stack) from [<c083f8dc>] (dump_stack+0x9c/0xd4) [ 153.134051] [<c083f8dc>] (dump_stack) from [<c0031154>] (__warn+0xf8/0x124) [ 153.141056] [<c0031154>] (__warn) from [<c0031248>] (warn_slowpath_fmt+0x38/0x48) [ 153.148580] [<c0031248>] (warn_slowpath_fmt) from [<c040fde0>] (clk_core_disable_lock+0x18/0x24) [ 153.157413] [<c040fde0>] (clk_core_disable_lock) from [<c058f520>] (i2c_imx_probe+0x554/0x6ec) [ 153.166076] [<c058f520>] (i2c_imx_probe) from [<c04b9178>] (platform_drv_probe+0x48/0x98) [ 153.174297] [<c04b9178>] (platform_drv_probe) from [<c04b7298>] (really_probe+0x1d8/0x2c0) [ 153.182605] [<c04b7298>] (really_probe) from [<c04b7554>] (driver_probe_device+0x5c/0x174) [ 153.190909] [<c04b7554>] (driver_probe_device) from [<c04b58c8>] (bus_for_each_drv+0x44/0x8c) [ 153.199480] [<c04b58c8>] (bus_for_each_drv) from [<c04b746c>] (__device_attach+0xa0/0x108) [ 153.207782] [<c04b746c>] (__device_attach) from [<c04b65a4>] (bus_probe_device+0x88/0x90) [ 153.215999] [<c04b65a4>] (bus_probe_device) from [<c04b6a04>] (deferred_probe_work_func+0x60/0x90) [ 153.225003] [<c04b6a04>] (deferred_probe_work_func) from [<c004f190>] (process_one_work+0x204/0x634) [ 153.234178] [<c004f190>] (process_one_work) from [<c004f618>] (worker_thread+0x20/0x484) [ 153.242315] [<c004f618>] (worker_thread) from [<c0055c2c>] (kthread+0x118/0x150) [ 153.249758] [<c0055c2c>] (kthread) from [<c00090b4>] (ret_from_fork+0x14/0x20) [ 153.257006] Exception stack(0xdde43fb0 to 0xdde43ff8) [ 153.262095] 3fa0: 00000000 00000000 00000000 00000000 [ 153.270306] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 153.278520] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 153.285159] irq event stamp: 3323022 [ 153.288787] hardirqs last enabled at (3323021): [<c0861c4c>] _raw_spin_unlock_irq+0x24/0x2c [ 153.297261] hardirqs last disabled at (3323022): [<c040d7a0>] clk_enable_lock+0x10/0x124 [ 153.305392] softirqs last enabled at (3322092): [<c000a504>] __do_softirq+0x344/0x540 [ 153.313352] softirqs last disabled at (3322081): [<c00385c0>] irq_exit+0x10c/0x128 [ 153.320946] ---[ end trace a506731ccd9bd703 ]--- When the DMA driver is built as module then dma_request_chan() returns -EPROBE_DEFER, so let's treat this case in the same way as "there is no DMA support" and make i2c_imx_dma_request() return success. Reported-by: Russell King <linux@armlinux.org.uk> Fixes: e1ab9a468e3b ("i2c: imx: improve the error handling in i2c_imx_dma_request()") Signed-off-by: Fabio Estevam <festevam@gmail.com> --- drivers/i2c/busses/i2c-imx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)