| Message ID | 20240928014409.633542-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: stm32f7: Do not prepare/unprepare clock during runtime suspend/resume | expand |
Hi Marek, thanks a lot for the patch. On Sat, Sep 28, 2024 at 03:43:46AM +0200, Marek Vasut wrote: > In case there is any sort of clock controller attached to this I2C bus > controller, for example Versaclock or even an AIC32x4 I2C codec, then > an I2C transfer triggered from the clock controller clk_ops .prepare > callback may trigger a deadlock on drivers/clk/clk.c prepare_lock mutex. > > This is because the clock controller first grabs the prepare_lock mutex > and then performs the prepare operation, including its I2C access. The > I2C access resumes this I2C bus controller via .runtime_resume callback, > which calls clk_prepare_enable(), which attempts to grab the prepare_lock > mutex again and deadlocks. > > Since the clock are already prepared since probe() and unprepared in > remove(), use simple clk_enable()/clk_disable() calls to enable and > disable the clock on runtime suspend and resume, to avoid hitting the > prepare_lock mutex. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alain Volmat <alain.volmat@foss.st.com> > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Andi Shyti <andi.shyti@kernel.org> > Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Pierre-Yves MORDRET <pierre-yves.mordret@foss.st.com> > Cc: kernel@dh-electronics.com > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-i2c@vger.kernel.org > Cc: linux-stm32@st-md-mailman.stormreply.com > --- > drivers/i2c/busses/i2c-stm32f7.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c > index cfee2d9c09de3..65c035728a4fa 100644 > --- a/drivers/i2c/busses/i2c-stm32f7.c > +++ b/drivers/i2c/busses/i2c-stm32f7.c > @@ -2395,7 +2395,7 @@ static int __maybe_unused stm32f7_i2c_runtime_suspend(struct device *dev) > struct stm32f7_i2c_dev *i2c_dev = dev_get_drvdata(dev); > > if (!stm32f7_i2c_is_slave_registered(i2c_dev)) > - clk_disable_unprepare(i2c_dev->clk); > + clk_disable(i2c_dev->clk); > > return 0; > } > @@ -2406,7 +2406,7 @@ static int __maybe_unused stm32f7_i2c_runtime_resume(struct device *dev) > int ret; > > if (!stm32f7_i2c_is_slave_registered(i2c_dev)) { > - ret = clk_prepare_enable(i2c_dev->clk); > + ret = clk_enable(i2c_dev->clk); > if (ret) { > dev_err(dev, "failed to prepare_enable clock\n"); The call now being clk_enable, could you also change the error message from prepare_enable to enable ? With that done, Acked-by: Alain Volmat <alain.volmat@foss.st.com> Regards, Alain > return ret; > -- > 2.45.2 >
On 9/30/24 8:38 PM, Alain Volmat wrote: > Hi Marek, Hello Alain, > thanks a lot for the patch. > > On Sat, Sep 28, 2024 at 03:43:46AM +0200, Marek Vasut wrote: >> In case there is any sort of clock controller attached to this I2C bus >> controller, for example Versaclock or even an AIC32x4 I2C codec, then >> an I2C transfer triggered from the clock controller clk_ops .prepare >> callback may trigger a deadlock on drivers/clk/clk.c prepare_lock mutex. >> >> This is because the clock controller first grabs the prepare_lock mutex >> and then performs the prepare operation, including its I2C access. The >> I2C access resumes this I2C bus controller via .runtime_resume callback, >> which calls clk_prepare_enable(), which attempts to grab the prepare_lock >> mutex again and deadlocks. >> >> Since the clock are already prepared since probe() and unprepared in >> remove(), use simple clk_enable()/clk_disable() calls to enable and >> disable the clock on runtime suspend and resume, to avoid hitting the >> prepare_lock mutex. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Alain Volmat <alain.volmat@foss.st.com> >> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >> Cc: Andi Shyti <andi.shyti@kernel.org> >> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com> >> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Cc: Pierre-Yves MORDRET <pierre-yves.mordret@foss.st.com> >> Cc: kernel@dh-electronics.com >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-i2c@vger.kernel.org >> Cc: linux-stm32@st-md-mailman.stormreply.com >> --- >> drivers/i2c/busses/i2c-stm32f7.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c >> index cfee2d9c09de3..65c035728a4fa 100644 >> --- a/drivers/i2c/busses/i2c-stm32f7.c >> +++ b/drivers/i2c/busses/i2c-stm32f7.c >> @@ -2395,7 +2395,7 @@ static int __maybe_unused stm32f7_i2c_runtime_suspend(struct device *dev) >> struct stm32f7_i2c_dev *i2c_dev = dev_get_drvdata(dev); >> >> if (!stm32f7_i2c_is_slave_registered(i2c_dev)) >> - clk_disable_unprepare(i2c_dev->clk); >> + clk_disable(i2c_dev->clk); >> >> return 0; >> } >> @@ -2406,7 +2406,7 @@ static int __maybe_unused stm32f7_i2c_runtime_resume(struct device *dev) >> int ret; >> >> if (!stm32f7_i2c_is_slave_registered(i2c_dev)) { >> - ret = clk_prepare_enable(i2c_dev->clk); >> + ret = clk_enable(i2c_dev->clk); >> if (ret) { >> dev_err(dev, "failed to prepare_enable clock\n"); > > The call now being clk_enable, could you also change the error message > from prepare_enable to enable ? > > With that done, > Acked-by: Alain Volmat <alain.volmat@foss.st.com> Fixed in V2, thanks !
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c index cfee2d9c09de3..65c035728a4fa 100644 --- a/drivers/i2c/busses/i2c-stm32f7.c +++ b/drivers/i2c/busses/i2c-stm32f7.c @@ -2395,7 +2395,7 @@ static int __maybe_unused stm32f7_i2c_runtime_suspend(struct device *dev) struct stm32f7_i2c_dev *i2c_dev = dev_get_drvdata(dev); if (!stm32f7_i2c_is_slave_registered(i2c_dev)) - clk_disable_unprepare(i2c_dev->clk); + clk_disable(i2c_dev->clk); return 0; } @@ -2406,7 +2406,7 @@ static int __maybe_unused stm32f7_i2c_runtime_resume(struct device *dev) int ret; if (!stm32f7_i2c_is_slave_registered(i2c_dev)) { - ret = clk_prepare_enable(i2c_dev->clk); + ret = clk_enable(i2c_dev->clk); if (ret) { dev_err(dev, "failed to prepare_enable clock\n"); return ret;
In case there is any sort of clock controller attached to this I2C bus controller, for example Versaclock or even an AIC32x4 I2C codec, then an I2C transfer triggered from the clock controller clk_ops .prepare callback may trigger a deadlock on drivers/clk/clk.c prepare_lock mutex. This is because the clock controller first grabs the prepare_lock mutex and then performs the prepare operation, including its I2C access. The I2C access resumes this I2C bus controller via .runtime_resume callback, which calls clk_prepare_enable(), which attempts to grab the prepare_lock mutex again and deadlocks. Since the clock are already prepared since probe() and unprepared in remove(), use simple clk_enable()/clk_disable() calls to enable and disable the clock on runtime suspend and resume, to avoid hitting the prepare_lock mutex. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Alain Volmat <alain.volmat@foss.st.com> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Andi Shyti <andi.shyti@kernel.org> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Pierre-Yves MORDRET <pierre-yves.mordret@foss.st.com> Cc: kernel@dh-electronics.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-i2c@vger.kernel.org Cc: linux-stm32@st-md-mailman.stormreply.com --- drivers/i2c/busses/i2c-stm32f7.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)