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
