| Message ID | 20231107141201.623482-1-alexander.stein@ew.tq-group.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7,1/1] i2c: lpi2c: use clk notifier for rate changes | expand |
> If you see your part in my reply down here you see that some > parts are missing. Perhaps there is a bug in lei that has missed > the top part of your patch because in lore I see that, indeed, > there is nothing wrong with your mail. Seems to be a lei bug, indeed. I can apply the patch from my mutt inbox without a problem.
Hi Wolfram, > > If you see your part in my reply down here you see that some > > parts are missing. Perhaps there is a bug in lei that has missed > > the top part of your patch because in lore I see that, indeed, > > there is nothing wrong with your mail. > > Seems to be a lei bug, indeed. I can apply the patch from my mutt inbox > without a problem. thanks for confirming, I will download it manually from lore and review it, then. Thanks, Andi
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? > 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? > > @@ -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. > + /* > + * 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); > -- > 2.34.1 >
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);
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
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"); + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_get_noresume(&pdev->dev);