Re: [PATCH v8 1/1] i2c: lpi2c: use clk notifier for rate changes

Re: [PATCH v8 1/1] i2c: lpi2c: use clk notifier for rate changes

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4613 bytes --]

Hello,

On Wed, Jan 10, 2024 at 01:05:56PM +0100, Alexander Stein wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> Instead of repeatedly calling clk_get_rate for each transfer, register
> a clock notifier to update the cached divider value each time the clock
> rate actually changes.
> A deadlock has been observed while adding tlv320aic32x4 audio codec to
> the system. When this clock provider adds its clock, the clk mutex is
> locked already, it needs to access i2c, which in return needs the mutex
> for clk_get_rate as well.
> 
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v8:
> * Improved commit message describing an actual observed deadlock
> 
> Changes in v7:
> * Use dev_err_probe
> * Reworked/Shortened the commit message
>  It is not about saving CPU cycles, but to avoid locking the clk subsystem
>  upon each transfer.
> 
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 40 +++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 678b30e90492a..3070e605fd8ff 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -5,6 +5,7 @@
>   * Copyright 2016 Freescale Semiconductor, Inc.
>   */
>  
> +#include <linux/atomic.h>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
> @@ -99,6 +100,8 @@ struct lpi2c_imx_struct {
>  	__u8			*rx_buf;
>  	__u8			*tx_buf;
>  	struct completion	complete;
> +	struct notifier_block	clk_change_nb;
> +	atomic_t		rate_per;
>  	unsigned int		msglen;
>  	unsigned int		delivered;
>  	unsigned int		block_data;
> @@ -197,6 +200,20 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx)
>  	} while (1);
>  }
>  
> +static int lpi2c_imx_clk_change_cb(struct notifier_block *nb,
> +				   unsigned long action, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct lpi2c_imx_struct *lpi2c_imx = container_of(nb,
> +							  struct lpi2c_imx_struct,
> +							  clk_change_nb);
> +
> +	if (action & POST_RATE_CHANGE)
> +		atomic_set(&lpi2c_imx->rate_per, ndata->new_rate);
> +
> +	return NOTIFY_OK;
> +}
> +
>  /* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
>  static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
>  {
> @@ -207,7 +224,7 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
>  
>  	lpi2c_imx_set_mode(lpi2c_imx);
>  
> -	clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
> +	clk_rate = atomic_read(&lpi2c_imx->rate_per);
>  	if (!clk_rate)
>  		return -EINVAL;
>  
> @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> +					 &lpi2c_imx->clk_change_nb);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "can't register peripheral clock notifier\n");
> +	/*
> +	 * Lock the clock rate to avoid rate change between clk_get_rate() and
> +	 * atomic_set()
> +	 */
> +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "can't lock I2C peripheral clock rate\n");
> +
> +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx->clks[0].clk));
> +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> +	if (!atomic_read(&lpi2c_imx->rate_per))
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "can't get I2C peripheral clock rate\n");
> +

If the clkrate isn't expected to actually change, you can just delay the
call to clk_rate_exclusive_put() until driver unbind time and not
register a notifier at all. The result would be more lightweight, you
wouldn't even need an atomic variable for .rate_per.

https://lore.kernel.org/all/20240104225512.1124519-2-u.kleine-koenig@pengutronix.de/
might be beneficial for that.

Having said that, improving the locking in the clk framework to not
trigger this deadlock would be nice.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v8 1/1] i2c: lpi2c: use clk notifier for rate changes

[Alexander Stein]

Hello Uwe,

Am Donnerstag, 11. Januar 2024, 09:51:30 CET schrieb Uwe Kleine-König:
> Hello,
> 
> On Wed, Jan 10, 2024 at 01:05:56PM +0100, Alexander Stein wrote:
> > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > Instead of repeatedly calling clk_get_rate for each transfer, register
> > a clock notifier to update the cached divider value each time the clock
> > rate actually changes.
> > A deadlock has been observed while adding tlv320aic32x4 audio codec to
> > the system. When this clock provider adds its clock, the clk mutex is
> > locked already, it needs to access i2c, which in return needs the mutex
> > for clk_get_rate as well.
> > 
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v8:
> > * Improved commit message describing an actual observed deadlock
> > 
> > Changes in v7:
> > * Use dev_err_probe
> > * Reworked/Shortened the commit message
> > 
> >  It is not about saving CPU cycles, but to avoid locking the clk subsystem
> >  upon each transfer.
> >  
> >  drivers/i2c/busses/i2c-imx-lpi2c.c | 40 +++++++++++++++++++++++++++++-
> >  1 file changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c index 678b30e90492a..3070e605fd8ff
> > 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -5,6 +5,7 @@
> > 
> >   * Copyright 2016 Freescale Semiconductor, Inc.
> >   */
> > 
> > +#include <linux/atomic.h>
> > 
> >  #include <linux/clk.h>
> >  #include <linux/completion.h>
> >  #include <linux/delay.h>
> > 
> > @@ -99,6 +100,8 @@ struct lpi2c_imx_struct {
> > 
> >  	__u8			*rx_buf;
> >  	__u8			*tx_buf;
> >  	struct completion	complete;
> > 
> > +	struct notifier_block	clk_change_nb;
> > +	atomic_t		rate_per;
> > 
> >  	unsigned int		msglen;
> >  	unsigned int		delivered;
> >  	unsigned int		block_data;
> > 
> > @@ -197,6 +200,20 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct
> > *lpi2c_imx)> 
> >  	} while (1);
> >  
> >  }
> > 
> > +static int lpi2c_imx_clk_change_cb(struct notifier_block *nb,
> > +				   unsigned long action, void *data)
> > +{
> > +	struct clk_notifier_data *ndata = data;
> > +	struct lpi2c_imx_struct *lpi2c_imx = container_of(nb,
> > +							  struct 
lpi2c_imx_struct,
> > +							  
clk_change_nb);
> > +
> > +	if (action & POST_RATE_CHANGE)
> > +		atomic_set(&lpi2c_imx->rate_per, ndata->new_rate);
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> > 
> >  /* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
> >  static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
> >  {
> > 
> > @@ -207,7 +224,7 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct
> > *lpi2c_imx)> 
> >  	lpi2c_imx_set_mode(lpi2c_imx);
> > 
> > -	clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
> > +	clk_rate = atomic_read(&lpi2c_imx->rate_per);
> > 
> >  	if (!clk_rate)
> >  	
> >  		return -EINVAL;
> > 
> > @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device
> > *pdev)> 
> >  	if (ret)
> >  	
> >  		return ret;
> > 
> > +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> > +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> > +					 &lpi2c_imx->clk_change_nb);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "can't register peripheral clock 
notifier\n");
> > +	/*
> > +	 * Lock the clock rate to avoid rate change between clk_get_rate() 
and
> > +	 * atomic_set()
> > +	 */
> > +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "can't lock I2C peripheral clock 
rate\n");
> > +
> > +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx-
>clks[0].clk));
> > +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> > +	if (!atomic_read(&lpi2c_imx->rate_per))
> > +		return dev_err_probe(&pdev->dev, -EINVAL,
> > +				     "can't get I2C peripheral clock 
rate\n");
> > +
> 
> If the clkrate isn't expected to actually change, you can just delay the
> call to clk_rate_exclusive_put() until driver unbind time and not
> register a notifier at all. The result would be more lightweight, you
> wouldn't even need an atomic variable for .rate_per.

On imx93 I don't expect the parent clock rate to change, as each lpi2c 
peripheral has its own dedicated root clock.
On imx8qxp and imx8qm lpi2c has it's own "clock tree", but these clocks are 
managed by the system controller.
Now idea about imx95 as this one apparently uses SCMI based clock driver.
No idea about imx7ulp, imx8ulp and imx8dxl.

Best regards,
Alexander

> https://lore.kernel.org/all/20240104225512.1124519-2-u.kleine-koenig@pengutr
> onix.de/ might be beneficial for that.
> 
> Having said that, improving the locking in the clk framework to not
> trigger this deadlock would be nice.
> 
> Best regards
> Uwe


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v8 1/1] i2c: lpi2c: use clk notifier for rate changes

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

Hello Alexander,

On Wed, Jan 17, 2024 at 08:02:19AM +0100, Alexander Stein wrote:
> Am Donnerstag, 11. Januar 2024, 09:51:30 CET schrieb Uwe Kleine-König:
> > On Wed, Jan 10, 2024 at 01:05:56PM +0100, Alexander Stein wrote:
> > > +	lpi2c_imx->clk_change_nb.notifier_call = lpi2c_imx_clk_change_cb;
> > > +	ret = devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk,
> > > +					 &lpi2c_imx->clk_change_nb);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "can't register peripheral clock notifier\n");
> > > +	/*
> > > +	 * Lock the clock rate to avoid rate change between clk_get_rate() and
> > > +	 * atomic_set()
> > > +	 */
> > > +	ret = clk_rate_exclusive_get(lpi2c_imx->clks[0].clk);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "can't lock I2C peripheral clock rate\n");
> > > +
> > > +	atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx-clks[0].clk));
> > > +	clk_rate_exclusive_put(lpi2c_imx->clks[0].clk);
> > > +	if (!atomic_read(&lpi2c_imx->rate_per))
> > > +		return dev_err_probe(&pdev->dev, -EINVAL,
> > > +				     "can't get I2C peripheral clock rate\n");
> > > +
> > 
> > If the clkrate isn't expected to actually change, you can just delay the
> > call to clk_rate_exclusive_put() until driver unbind time and not
> > register a notifier at all. The result would be more lightweight, you
> > wouldn't even need an atomic variable for .rate_per.
> 
> On imx93 I don't expect the parent clock rate to change, as each lpi2c 
> peripheral has its own dedicated root clock.
> On imx8qxp and imx8qm lpi2c has it's own "clock tree", but these clocks are 
> managed by the system controller.
> Now idea about imx95 as this one apparently uses SCMI based clock driver.
> No idea about imx7ulp, imx8ulp and imx8dxl.

Then maybe pick the easier approach and only start with a notifier when
the need arises?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]