| Message ID | 20231107141201.623482-1-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,1/1] i2c: lpi2c: use clk notifier for rate changes | expand |
Hi Alexander, is it my mail client not working or is is your patch that has gone through something terribly bad? Andi On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote: > * 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); > -- > 2.34.1 >
Hi Andi, Am Dienstag, 7. November 2023, 22:20:49 CET schrieb Andi Shyti: > Hi Alexander, > > is it my mail client not working or is is your patch that has > gone through something terribly bad? I can't see anything obviously wrong. Can you elaborate? Thanks Alexander > Andi > > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote: > > * 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);
Hi Alexander, On Wed, Nov 08, 2023 at 07:46:12AM +0100, Alexander Stein wrote: > Am Dienstag, 7. November 2023, 22:20:49 CET schrieb Andi Shyti: > > is it my mail client not working or is is your patch that has > > gone through something terribly bad? > > I can't see anything obviously wrong. Can you elaborate? 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. Andi > Thanks > Alexander > > > Andi > > > > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote: > > > * 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); > > > -- > 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/ > >
> 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);
Hi Alexander, 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. > [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. 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. > > > @@ -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. why do you care? If ENOMEM has failed, then let the driver fail on its own, don't be the one pulling the trigger. But I'm not strong here: it's not completely wrong to bail out, either. Andi > 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/ > >
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
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
Hi, > > ... 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. That means a v8 is in your queue? > > ... 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. IMHO, the return value doesn't matter that much as we have a descriptive text accompanying it anyhow. Happy hacking, Wolfram
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);