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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Andi,

thanks for the feedback.

Am Donnerstag, 9. November 2023, 00:38:09 CET schrieb Andi Shyti:
> Hi Alexander,
> 
> On Tue, Nov 07, 2023 at 03:12:01PM +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.
> > There is also a lockdep splat if a I2C based clock provider needs to
> > access the i2c bus while in recalc_rate(). "prepare_lock" is about to
> > be locked recursively.
> > 
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> 
> What's the exact fix here? Is it the lockdep warning? Is it
> actually causing a real deadlock?

We've seen actual deadlocks on our imx8qxp based hardware using a downstream 
kernel, so we have implemented as similar fix [1]. This happened using 
tlv320aic32x4 audio codec.
The backtrace is similar, a i2c based clock provider is trying t issue an i2c 
transfer while adding the clock, thus 'prepare_lock' is already locked.
Lockdep raises an error both for downstream kernel as well as mainline, both 
for tlv320aic32x4 or pcf85063.

[1] https://github.com/tq-systems/linux-tqmaxx/commit/
b0339ff83a979f2ea066012b9209ea7c54f2b4e7

> > 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 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;
> 
> Doesn't seem like EINVAL, defined as "Invalid argument", is the
> correct number here. As we are failing to read the clock rate, do
> you think EIO is better?

Well, this is already the current error code. In both the old and new code I 
would consider this error case as 'no clock rate provided' rather than failing 
to read. I would reject EIO as there is no IO transfer for retrieving the 
clock rate.

> > @@ -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");
> 
> can't we fall back to how things were instead of failing the
> probe? But I'm not sure it would remove the lockdep warning,
> though. I can live with it.

I don't see a reason why you would want to continue if 
devm_clk_notifier_register() failed. It's either ENOMEM, EINVAL (if you pass 
NULL for clk or notifier block) or EEXIST if registered twice. So in reality 
only ENOMEM is possible, but then things went south already.

Best regards,
Alexander

> > +	/*
> > +	 * 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");
> > +
> 
> Not a bad patch, would be nice if all the above was provided by
> the library so that other drivers can use it.
> 
> Andi
> 
> >  	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
> >  	pm_runtime_use_autosuspend(&pdev->dev);
> >  	pm_runtime_get_noresume(&pdev->dev);


-- 
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/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel