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

[Uwe Kleine-König]

Hello,

On Wed, Jan 10, 2024 at 01:05:56PM +0100, Alexander Stein wrote:
> From: Alexander Sverdlin <alexander.sverdlin at 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 at siemens.com>
> Reviewed-by: Alexander Stein <alexander.stein at ew.tq-group.com>
> Signed-off-by: Alexander Stein <alexander.stein at 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/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20240111/717625a7/attachment.sig>