Hi Andi,
Am Freitag, 10. November 2023, 13:27:20 CET schrieb 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...
I've improved the commit message about an actual deadlock.
> [...]
>
> > > > > @@ -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.
Thinking about this again, I think EINVAL is an appropriate error code.
The parent clock frequency is also an input for the i2c transfer. So if, for
whatever reason, that clock frequency is 0, it is an invalid value (argument).
I've checked other drivers what they do if that clock is 0. Unfortunately most
don't consider this case at all. But some do, so e.g. i2c_lpc2k_probe() or
dc_i2c_init_hw() both return EINVAL if the clk or a calculated divider is 0.
> If the deadlock you mentioned is a warning from the lockdep, then
> please remove the "Fixes:" tag.
It's not just a lockdep warning, the deadlock actually happened.
Best regards,
Alexander
> Andi
--
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/
