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

[Andi Shyti]

Alexander,

if you...

On Thu, Nov 09, 2023 at 10:10:46AM +0100, Andi Shyti wrote:
> On Thu, Nov 09, 2023 at 08:54:51AM +0100, Alexander Stein wrote:
> > Am Donnerstag, 9. November 2023, 00:38:09 CET schrieb Andi Shyti:
> > > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote:
> > > > 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.
> 
> yes, if the clock is using the same controller then it's likely
> that you might end up in a deadlock. This is why I like this
> patch and I believe this shouild go in the library at some point.
> 
> But the deadlock is not mentioned in the commit log and lockdep
> doesn't always mean deadlock.

... improve the commit message, reporting the real deadlock case
instead of a lockdep warning and...

[...]

> > > > @@ -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.
> 
> It's definitely not EINVAL as we are failing not because of
> invalid arguments. I thought of EIO because at some point the
> clock rate has been retrieved (or set, thus i/o) and "rate_per"
> updated accordingly. But I agree that's not the perfect value
> either.
> 
> I couldn't think of a better error value, though.

... find a more appropriate error number, I will ack this patch.

If the deadlock you mentioned is a warning from the lockdep, then
please remove the "Fixes:" tag.

Andi