This is unsafe, as the runtime PM callbacks are called from the PM
workqueue, so this may deadlock when handling an i2c attached clock,
which may already hold the clk_prepare mutex from another context.
Fixes: 1fc2fe204cb9 ("i2c: designware: Add runtime PM hooks")
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
drivers/i2c/busses/i2c-designware-common.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 5b1e8f74c4ac..b4e38bc0f876 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -776,7 +776,8 @@ static int i2c_dw_runtime_suspend(struct device *device)
return 0;
i2c_dw_disable(dev);
- i2c_dw_prepare_clk(dev, false);
+ clk_disable(dev->clk);
+ clk_disable(dev->pclk);
return 0;
}
@@ -794,8 +795,10 @@ static int i2c_dw_runtime_resume(struct device *device)
{
struct dw_i2c_dev *dev = dev_get_drvdata(device);
- if (!dev->shared_with_punit)
- i2c_dw_prepare_clk(dev, true);
+ if (!dev->shared_with_punit) {
+ clk_enable(dev->pclk);
+ clk_enable(dev->clk);
+ }
dev->init(dev);
--
2.50.1On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote: > This is unsafe, as the runtime PM callbacks are called from the PM > workqueue, so this may deadlock when handling an i2c attached clock, > which may already hold the clk_prepare mutex from another context. Can you be more specific? What is the actual issue in practice? Do you have traces and lockdep warnings? AFAICS it seems related to the bus recovery mechanism. Is this what you have in mind? -- With Best Regards, Andy Shevchenko
On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote: > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote: > > This is unsafe, as the runtime PM callbacks are called from the PM > > workqueue, so this may deadlock when handling an i2c attached clock, > > which may already hold the clk_prepare mutex from another context. > > Can you be more specific? What is the actual issue in practice? > Do you have traces and lockdep warnings? Assume we use i2c designware to control any i2c based clks, e.g the clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock mutex, then we call i2c adapter to operate the regs, to runtime resume the i2c adapter, we call clk_prepare_enable() which will try to get the prepare_lock mutex again. Regards. > > AFAICS it seems related to the bus recovery mechanism. Is this what you have in > mind? > > -- > With Best Regards, > Andy Shevchenko > >
On 8/20/25 7:33 PM, Jisheng Zhang wrote: > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote: >> On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote: >>> This is unsafe, as the runtime PM callbacks are called from the PM >>> workqueue, so this may deadlock when handling an i2c attached clock, >>> which may already hold the clk_prepare mutex from another context. >> >> Can you be more specific? What is the actual issue in practice? >> Do you have traces and lockdep warnings? > > Assume we use i2c designware to control any i2c based clks, e.g the > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock > mutex, then we call i2c adapter to operate the regs, to runtime resume > the i2c adapter, we call clk_prepare_enable() which will try to get > the prepare_lock mutex again. > I'd also like to see the issue here. I'm blind to see what's the relation between the clocks managed by the clk-si5351.c and clocks to the i2c-designware IP.
On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote: > On 8/20/25 7:33 PM, Jisheng Zhang wrote: > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote: > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote: > > > > This is unsafe, as the runtime PM callbacks are called from the PM > > > > workqueue, so this may deadlock when handling an i2c attached clock, > > > > which may already hold the clk_prepare mutex from another context. > > > > > > Can you be more specific? What is the actual issue in practice? > > > Do you have traces and lockdep warnings? > > > > Assume we use i2c designware to control any i2c based clks, e.g the > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock > > mutex, then we call i2c adapter to operate the regs, to runtime resume > > the i2c adapter, we call clk_prepare_enable() which will try to get > > the prepare_lock mutex again. > > > I'd also like to see the issue here. I'm blind to see what's the relation > between the clocks managed by the clk-si5351.c and clocks to the > i2c-designware IP. I believe they try to make an example when clk-si5351 is the provider of the clock to I²C host controller (DesignWare). But I'm still not sure about the issues here... Without (even simulated with specific delay injections) lockdep warnings it would be rather theoretical. -- With Best Regards, Andy Shevchenko
On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > which may already hold the clk_prepare mutex from another context.
> > > >
> > > > Can you be more specific? What is the actual issue in practice?
> > > > Do you have traces and lockdep warnings?
> > >
> > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > the prepare_lock mutex again.
> > >
> > I'd also like to see the issue here. I'm blind to see what's the relation
> > between the clocks managed by the clk-si5351.c and clocks to the
> > i2c-designware IP.
The key here is: all clks in the system share the same prepare_lock
mutex, so the global prepare_lock mutex is locked by clk-si5351
.prepare(), then in this exact .prepare(), the i2c-designware's runtime
resume will try to lock the same prepare_lock again due to
clk_prepare_enable()
can you plz check clk_prepare_lock() in drivers/clk/clk.c?
And if we take a look at other i2c adapters' drivers, we'll see
some of them have ever met this issue and already fixed it, such
as
i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
deadlock by keeping I2C clock prepared")
i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
mutex in PM callbacks")
>
> I believe they try to make an example when clk-si5351 is the provider of
> the clock to I²C host controller (DesignWare).
Nope, the example case is using i2c host controller to operate the clk-si5351
>
> But I'm still not sure about the issues here... Without (even simulated with
> specific delay injections) lockdep warnings it would be rather theoretical.
No, it happened in real world.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Fri, Aug 22, 2025 at 12:32:57AM +0800, Jisheng Zhang wrote:
> On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> > On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > > which may already hold the clk_prepare mutex from another context.
> > > > >
> > > > > Can you be more specific? What is the actual issue in practice?
> > > > > Do you have traces and lockdep warnings?
> > > >
> > > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > > the prepare_lock mutex again.
> > > >
> > > I'd also like to see the issue here. I'm blind to see what's the relation
> > > between the clocks managed by the clk-si5351.c and clocks to the
> > > i2c-designware IP.
>
> The key here is: all clks in the system share the same prepare_lock
> mutex, so the global prepare_lock mutex is locked by clk-si5351
> .prepare(), then in this exact .prepare(), the i2c-designware's runtime
> resume will try to lock the same prepare_lock again due to
> clk_prepare_enable()
> can you plz check clk_prepare_lock() in drivers/clk/clk.c?
>
> And if we take a look at other i2c adapters' drivers, we'll see
> some of them have ever met this issue and already fixed it, such
> as
>
> i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
> deadlock by keeping I2C clock prepared")
>
> i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
> mutex in PM callbacks")
Why is this an I²C driver problem?
> > I believe they try to make an example when clk-si5351 is the provider of
> > the clock to I²C host controller (DesignWare).
>
> Nope, the example case is using i2c host controller to operate the clk-si5351
Okay, so that chip is controlled over I²C, but how their clocks even related to
the I²C host controller clock?! I am sorry, I am lost here.
> > But I'm still not sure about the issues here... Without (even simulated with
> > specific delay injections) lockdep warnings it would be rather theoretical.
>
> No, it happened in real world.
Can you provide the asked traces and lockdep warnigns and/or other stuff to see
what's going on there?
--
With Best Regards,
Andy Shevchenko
On Fri, Aug 22, 2025 at 12:18:43PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 22, 2025 at 12:32:57AM +0800, Jisheng Zhang wrote:
> > On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > > > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > > > which may already hold the clk_prepare mutex from another context.
> > > > > >
> > > > > > Can you be more specific? What is the actual issue in practice?
> > > > > > Do you have traces and lockdep warnings?
> > > > >
> > > > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > > > the prepare_lock mutex again.
> > > > >
> > > > I'd also like to see the issue here. I'm blind to see what's the relation
> > > > between the clocks managed by the clk-si5351.c and clocks to the
> > > > i2c-designware IP.
> >
> > The key here is: all clks in the system share the same prepare_lock
> > mutex, so the global prepare_lock mutex is locked by clk-si5351
> > .prepare(), then in this exact .prepare(), the i2c-designware's runtime
> > resume will try to lock the same prepare_lock again due to
> > clk_prepare_enable()
> > can you plz check clk_prepare_lock() in drivers/clk/clk.c?
> >
> > And if we take a look at other i2c adapters' drivers, we'll see
> > some of them have ever met this issue and already fixed it, such
> > as
> >
> > i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
> > deadlock by keeping I2C clock prepared")
> >
> > i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
> > mutex in PM callbacks")
> Why is this an I²C driver problem?
I just read these two and one more referenced from one of the changes.
I do not think this is a correct fix. Seems to me like papering over a special
(corner case). I would agree on this change if and only if the CLK maintainers
tell us that there is no other way.
My understanding is that the I²C clock and client's clocks (when it's a clock
provider) are independent. There should not be such a clash to begin with. The
clock framework should operate on a clock subtrees and not having yet another
Global Kernel Lock.
That said, I think this is a design issue in CLK framework, we should not go and
"fix" all the drivers. Today it's I²C, tomorrow SPI and I³C and so on...
This is not a scalable solution.
Here is formal NAK until it will be worked with CLK maintainers to provide an
agreed roadmap for this(ese) issue(s).
> > > I believe they try to make an example when clk-si5351 is the provider of
> > > the clock to I²C host controller (DesignWare).
> >
> > Nope, the example case is using i2c host controller to operate the clk-si5351
>
> Okay, so that chip is controlled over I²C, but how their clocks even related to
> the I²C host controller clock?! I am sorry, I am lost here.
>
> > > But I'm still not sure about the issues here... Without (even simulated with
> > > specific delay injections) lockdep warnings it would be rather theoretical.
> >
> > No, it happened in real world.
>
> Can you provide the asked traces and lockdep warnigns and/or other stuff to see
> what's going on there?
--
With Best Regards,
Andy Shevchenko
On Fri, Aug 22, 2025 at 12:34:07PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 22, 2025 at 12:18:43PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 22, 2025 at 12:32:57AM +0800, Jisheng Zhang wrote:
> > > On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > > > > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > > > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > > > > which may already hold the clk_prepare mutex from another context.
> > > > > > >
> > > > > > > Can you be more specific? What is the actual issue in practice?
> > > > > > > Do you have traces and lockdep warnings?
> > > > > >
> > > > > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > > > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > > > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > > > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > > > > the prepare_lock mutex again.
> > > > > >
> > > > > I'd also like to see the issue here. I'm blind to see what's the relation
> > > > > between the clocks managed by the clk-si5351.c and clocks to the
> > > > > i2c-designware IP.
> > >
> > > The key here is: all clks in the system share the same prepare_lock
> > > mutex, so the global prepare_lock mutex is locked by clk-si5351
> > > .prepare(), then in this exact .prepare(), the i2c-designware's runtime
> > > resume will try to lock the same prepare_lock again due to
> > > clk_prepare_enable()
> > > can you plz check clk_prepare_lock() in drivers/clk/clk.c?
> > >
> > > And if we take a look at other i2c adapters' drivers, we'll see
> > > some of them have ever met this issue and already fixed it, such
> > > as
> > >
> > > i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
> > > deadlock by keeping I2C clock prepared")
> > >
> > > i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
> > > mutex in PM callbacks")
>
> > Why is this an I²C driver problem?
>
> I just read these two and one more referenced from one of the changes.
>
> I do not think this is a correct fix. Seems to me like papering over a special
> (corner case). I would agree on this change if and only if the CLK maintainers
> tell us that there is no other way.
>
> My understanding is that the I²C clock and client's clocks (when it's a clock
> provider) are independent. There should not be such a clash to begin with. The
> clock framework should operate on a clock subtrees and not having yet another
> Global Kernel Lock.
>
> That said, I think this is a design issue in CLK framework, we should not go and
After some thoughts, let me show you another case where this patch is
needed and not related with CLK framework at all: As can be seen in
patch2, atomic transfer support is added, but if IIRC, the
clk_prepare_enable() can't be used in atomic context. Then how to
support atomic transfer if patch1 is NAKed? Any comment is appreciated.
Thanks
> "fix" all the drivers. Today it's I²C, tomorrow SPI and I³C and so on...
> This is not a scalable solution.
>
> Here is formal NAK until it will be worked with CLK maintainers to provide an
> agreed roadmap for this(ese) issue(s).
>
> > > > I believe they try to make an example when clk-si5351 is the provider of
> > > > the clock to I²C host controller (DesignWare).
> > >
> > > Nope, the example case is using i2c host controller to operate the clk-si5351
> >
> > Okay, so that chip is controlled over I²C, but how their clocks even related to
> > the I²C host controller clock?! I am sorry, I am lost here.
> >
> > > > But I'm still not sure about the issues here... Without (even simulated with
> > > > specific delay injections) lockdep warnings it would be rather theoretical.
> > >
> > > No, it happened in real world.
> >
> > Can you provide the asked traces and lockdep warnigns and/or other stuff to see
> > what's going on there?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Fri, Aug 22, 2025 at 12:34:07PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 22, 2025 at 12:18:43PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 22, 2025 at 12:32:57AM +0800, Jisheng Zhang wrote:
> > > On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > > > > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > > > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > > > > which may already hold the clk_prepare mutex from another context.
> > > > > > >
> > > > > > > Can you be more specific? What is the actual issue in practice?
> > > > > > > Do you have traces and lockdep warnings?
> > > > > >
> > > > > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > > > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > > > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > > > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > > > > the prepare_lock mutex again.
> > > > > >
> > > > > I'd also like to see the issue here. I'm blind to see what's the relation
> > > > > between the clocks managed by the clk-si5351.c and clocks to the
> > > > > i2c-designware IP.
> > >
> > > The key here is: all clks in the system share the same prepare_lock
> > > mutex, so the global prepare_lock mutex is locked by clk-si5351
> > > .prepare(), then in this exact .prepare(), the i2c-designware's runtime
> > > resume will try to lock the same prepare_lock again due to
> > > clk_prepare_enable()
> > > can you plz check clk_prepare_lock() in drivers/clk/clk.c?
> > >
> > > And if we take a look at other i2c adapters' drivers, we'll see
> > > some of them have ever met this issue and already fixed it, such
> > > as
> > >
> > > i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
> > > deadlock by keeping I2C clock prepared")
> > >
> > > i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
> > > mutex in PM callbacks")
>
> > Why is this an I²C driver problem?
>
> I just read these two and one more referenced from one of the changes.
>
> I do not think this is a correct fix. Seems to me like papering over a special
> (corner case). I would agree on this change if and only if the CLK maintainers
> tell us that there is no other way.
>
> My understanding is that the I²C clock and client's clocks (when it's a clock
> provider) are independent. There should not be such a clash to begin with. The
> clock framework should operate on a clock subtrees and not having yet another
> Global Kernel Lock.
>
> That said, I think this is a design issue in CLK framework, we should not go and
> "fix" all the drivers. Today it's I²C, tomorrow SPI and I³C and so on...
> This is not a scalable solution.
The fact is there's no SPI attached clks. Current all clks are either
mmio based or i2c attached clks. And yep you're right, I3C has the same
problem if we use i3c controller to operate those i2c attached clks.
>
> Here is formal NAK until it will be worked with CLK maintainers to provide an
> agreed roadmap for this(ese) issue(s).
+ clk maintainers
Hi Michael and Stephen,
If we use i2c designware adapter to operate the i2c attached clks such
as clk-si5351, then there's a deadlock issue -- This issue is well
explained by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
mutex in PM callbacks")
I propsed a similar fix for the i2c-designware adapter, but Andy
disagree with this fix. Instead, he thinks the issue is from the clk
framework. Could you please comment?
Thanks
>
> > > > I believe they try to make an example when clk-si5351 is the provider of
> > > > the clock to I²C host controller (DesignWare).
> > >
> > > Nope, the example case is using i2c host controller to operate the clk-si5351
> >
> > Okay, so that chip is controlled over I²C, but how their clocks even related to
> > the I²C host controller clock?! I am sorry, I am lost here.
> >
> > > > But I'm still not sure about the issues here... Without (even simulated with
> > > > specific delay injections) lockdep warnings it would be rather theoretical.
> > >
> > > No, it happened in real world.
> >
> > Can you provide the asked traces and lockdep warnigns and/or other stuff to see
> > what's going on there?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
© 2016 - 2025 Red Hat, Inc.