| Message ID | 20191212233428.14648-4-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Tegra I2C: Support atomic transfers and correct suspend/resume | expand |
13.12.2019 02:34, Dmitry Osipenko пишет: > I noticed that sometime I2C clock is kept enabled during suspend-resume. > This happens because runtime PM defers dynamic suspension and thus it may > happen that runtime PM is in active state when system enters into suspend. > In particular I2C controller that is used for CPU's DVFS is often kept ON > during suspend because CPU's voltage scaling happens quite often. > > Note: we marked runtime PM as IRQ-safe during the driver's probe in the > "Support atomic transfers" patch, thus it's okay to enforce runtime PM > suspend/resume in the NOIRQ phase which is used for the system-level > suspend/resume of the driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index b3ecdd87e91f..d309a314f4d6 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev) > static int __maybe_unused tegra_i2c_suspend(struct device *dev) > { > struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > + int err; > > i2c_mark_adapter_suspended(&i2c_dev->adapter); > > + err = pm_runtime_force_suspend(dev); > + if (err < 0) > + return err; > + > return 0; > } > > @@ -1813,6 +1818,10 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev) > if (err) > return err; > > + err = pm_runtime_force_resume(dev); > + if (err < 0) > + return err; > + > i2c_mark_adapter_resumed(&i2c_dev->adapter); > > return 0; > It just occurred to me that this patch needs to marked as fixes for the "i2c: tegra: Move suspend handling to NOIRQ phase" patch because it broke runtime PM enable-refcount by disabling clock/pinmux on resume from suspend. For now I'll wait for the review comments. Please review, thanks in advance.
On Fri, Dec 13, 2019 at 02:34:28AM +0300, Dmitry Osipenko wrote: > I noticed that sometime I2C clock is kept enabled during suspend-resume. > This happens because runtime PM defers dynamic suspension and thus it may > happen that runtime PM is in active state when system enters into suspend. > In particular I2C controller that is used for CPU's DVFS is often kept ON > during suspend because CPU's voltage scaling happens quite often. > > Note: we marked runtime PM as IRQ-safe during the driver's probe in the > "Support atomic transfers" patch, thus it's okay to enforce runtime PM > suspend/resume in the NOIRQ phase which is used for the system-level > suspend/resume of the driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ > 1 file changed, 9 insertions(+) I've recently discussed this with Rafael in the context of runtime PM support in the Tegra DRM driver and my understanding is that you're not supposed to force runtime PM suspension like this. I had meant to send out an alternative patch to fix this, which I've done now: http://patchwork.ozlabs.org/patch/1209148/ That's more in line with what Rafael and I had discussed in the other thread and should address the issue that you're seeing as well. Thierry > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index b3ecdd87e91f..d309a314f4d6 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev) > static int __maybe_unused tegra_i2c_suspend(struct device *dev) > { > struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > + int err; > > i2c_mark_adapter_suspended(&i2c_dev->adapter); > > + err = pm_runtime_force_suspend(dev); > + if (err < 0) > + return err; > + > return 0; > } > > @@ -1813,6 +1818,10 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev) > if (err) > return err; > > + err = pm_runtime_force_resume(dev); > + if (err < 0) > + return err; > + > i2c_mark_adapter_resumed(&i2c_dev->adapter); > > return 0; > -- > 2.24.0 >
13.12.2019 16:47, Thierry Reding пишет: > On Fri, Dec 13, 2019 at 02:34:28AM +0300, Dmitry Osipenko wrote: >> I noticed that sometime I2C clock is kept enabled during suspend-resume. >> This happens because runtime PM defers dynamic suspension and thus it may >> happen that runtime PM is in active state when system enters into suspend. >> In particular I2C controller that is used for CPU's DVFS is often kept ON >> during suspend because CPU's voltage scaling happens quite often. >> >> Note: we marked runtime PM as IRQ-safe during the driver's probe in the >> "Support atomic transfers" patch, thus it's okay to enforce runtime PM >> suspend/resume in the NOIRQ phase which is used for the system-level >> suspend/resume of the driver. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) > > I've recently discussed this with Rafael in the context of runtime PM > support in the Tegra DRM driver and my understanding is that you're not > supposed to force runtime PM suspension like this. > > I had meant to send out an alternative patch to fix this, which I've > done now: > > http://patchwork.ozlabs.org/patch/1209148/ > > That's more in line with what Rafael and I had discussed in the other > thread and should address the issue that you're seeing as well. Well, either me or you are still having some misunderstanding of the runtime PM :) To my knowledge there are a lot of drivers that enforce suspension of the runtime PM during system's suspend, it should be a right thing to do especially in a context of the Tegra I2C driver because we're using asynchronous pm_runtime_put() and thus at the time of system's suspending, the runtime PM could be ON (as I wrote in the commit message) and then Terga's I2C driver manually disables the clock on resume (woopsie). By invoking pm_runtime_force_suspend() on systems's suspend, the runtime PM executes tegra_i2c_runtime_suspend() if device is in active state. On system resume, pm_runtime_force_resume() either keeps device in a suspended state or resumes it, say if for userspace disabled the runtime PM for the I2C controller. Rafael, could you please clarify whether my patch is doing a wrong thing? >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index b3ecdd87e91f..d309a314f4d6 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev) >> static int __maybe_unused tegra_i2c_suspend(struct device *dev) >> { >> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); >> + int err; >> >> i2c_mark_adapter_suspended(&i2c_dev->adapter); >> >> + err = pm_runtime_force_suspend(dev); >> + if (err < 0) >> + return err; >> + >> return 0; >> } >> >> @@ -1813,6 +1818,10 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev) >> if (err) >> return err; >> >> + err = pm_runtime_force_resume(dev); >> + if (err < 0) >> + return err; >> + >> i2c_mark_adapter_resumed(&i2c_dev->adapter); >> >> return 0; >> -- >> 2.24.0 >>
13.12.2019 02:34, Dmitry Osipenko пишет: > I noticed that sometime I2C clock is kept enabled during suspend-resume. > This happens because runtime PM defers dynamic suspension and thus it may > happen that runtime PM is in active state when system enters into suspend. > In particular I2C controller that is used for CPU's DVFS is often kept ON > during suspend because CPU's voltage scaling happens quite often. > > Note: we marked runtime PM as IRQ-safe during the driver's probe in the > "Support atomic transfers" patch, thus it's okay to enforce runtime PM > suspend/resume in the NOIRQ phase which is used for the system-level > suspend/resume of the driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index b3ecdd87e91f..d309a314f4d6 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev) > static int __maybe_unused tegra_i2c_suspend(struct device *dev) > { > struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > + int err; > > i2c_mark_adapter_suspended(&i2c_dev->adapter); I'm now in a doubt that it is correct to use NOIRQ level at all for the suspend because i2c_mark_adapter_suspended() uses mutex, thus I'm wondering what will happen if there is an asynchronous transfer happening during suspend.. The i2c_mark_adapter_suspended() will try to block and will never return? > + err = pm_runtime_force_suspend(dev); > + if (err < 0) > + return err; > + > return 0; > } > > @@ -1813,6 +1818,10 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev) > if (err) > return err; > > + err = pm_runtime_force_resume(dev); > + if (err < 0) > + return err; > + > i2c_mark_adapter_resumed(&i2c_dev->adapter); > > return 0; >
13.12.2019 17:29, Dmitry Osipenko пишет: > 13.12.2019 02:34, Dmitry Osipenko пишет: >> I noticed that sometime I2C clock is kept enabled during suspend-resume. >> This happens because runtime PM defers dynamic suspension and thus it may >> happen that runtime PM is in active state when system enters into suspend. >> In particular I2C controller that is used for CPU's DVFS is often kept ON >> during suspend because CPU's voltage scaling happens quite often. >> >> Note: we marked runtime PM as IRQ-safe during the driver's probe in the >> "Support atomic transfers" patch, thus it's okay to enforce runtime PM >> suspend/resume in the NOIRQ phase which is used for the system-level >> suspend/resume of the driver. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index b3ecdd87e91f..d309a314f4d6 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev) >> static int __maybe_unused tegra_i2c_suspend(struct device *dev) >> { >> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); >> + int err; >> >> i2c_mark_adapter_suspended(&i2c_dev->adapter); > > I'm now in a doubt that it is correct to use NOIRQ level at all for the > suspend because i2c_mark_adapter_suspended() uses mutex, thus I'm > wondering what will happen if there is an asynchronous transfer > happening during suspend.. > > The i2c_mark_adapter_suspended() will try to block and will never return? Moreover, the I2C interrupt should be disabled during the NOIRQ phase. So, yes.. looks like making use of NOIRQ level wasn't a correct decision. On the other hand, I don't think that any I2C client driver used by Tegra SoCs in the upstream kernel could cause the problem at the moment, so it shouldn't be critical. BTW: Jon, please CC me next time ;) [I'll try to find a better solution for the PCIE problem] >> + err = pm_runtime_force_suspend(dev); >> + if (err < 0) >> + return err; >> + >> return 0; >> } >> >> @@ -1813,6 +1818,10 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev) >> if (err) >> return err; >> >> + err = pm_runtime_force_resume(dev); >> + if (err < 0) >> + return err; >> + >> i2c_mark_adapter_resumed(&i2c_dev->adapter); >> >> return 0; >> >
13.12.2019 17:04, Dmitry Osipenko пишет: > 13.12.2019 16:47, Thierry Reding пишет: >> On Fri, Dec 13, 2019 at 02:34:28AM +0300, Dmitry Osipenko wrote: >>> I noticed that sometime I2C clock is kept enabled during suspend-resume. >>> This happens because runtime PM defers dynamic suspension and thus it may >>> happen that runtime PM is in active state when system enters into suspend. >>> In particular I2C controller that is used for CPU's DVFS is often kept ON >>> during suspend because CPU's voltage scaling happens quite often. >>> >>> Note: we marked runtime PM as IRQ-safe during the driver's probe in the >>> "Support atomic transfers" patch, thus it's okay to enforce runtime PM >>> suspend/resume in the NOIRQ phase which is used for the system-level >>> suspend/resume of the driver. >>> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >> >> I've recently discussed this with Rafael in the context of runtime PM >> support in the Tegra DRM driver and my understanding is that you're not >> supposed to force runtime PM suspension like this. >> >> I had meant to send out an alternative patch to fix this, which I've >> done now: >> >> http://patchwork.ozlabs.org/patch/1209148/ >> >> That's more in line with what Rafael and I had discussed in the other >> thread and should address the issue that you're seeing as well. > > Well, either me or you are still having some misunderstanding of the > runtime PM :) To my knowledge there are a lot of drivers that enforce > suspension of the runtime PM during system's suspend, it should be a > right thing to do especially in a context of the Tegra I2C driver > because we're using asynchronous pm_runtime_put() and thus at the time > of system's suspending, the runtime PM could be ON (as I wrote in the > commit message) and then Terga's I2C driver manually disables the clock > on resume (woopsie). Actually, looks like it's not the asynchronous pm_runtime_put() is the cause of suspending in active state. I see that only one of three I2C controllers is suspended in the enabled state, maybe some child (I2C client) device keeps it awake, will try to find out. > By invoking pm_runtime_force_suspend() on systems's suspend, the runtime > PM executes tegra_i2c_runtime_suspend() if device is in active state. On > system resume, pm_runtime_force_resume() either keeps device in a > suspended state or resumes it, say if for userspace disabled the runtime > PM for the I2C controller. > > Rafael, could you please clarify whether my patch is doing a wrong thing? > >>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >>> index b3ecdd87e91f..d309a314f4d6 100644 >>> --- a/drivers/i2c/busses/i2c-tegra.c >>> +++ b/drivers/i2c/busses/i2c-tegra.c >>> @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev) >>> static int __maybe_unused tegra_i2c_suspend(struct device *dev) >>> { >>> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); >>> + int err; >>> >>> i2c_mark_adapter_suspended(&i2c_dev->adapter); >>> >>> + err = pm_runtime_force_suspend(dev); >>> + if (err < 0) >>> + return err; >>> + >>> return 0; >>> } >>> >>> @@ -1813,6 +1818,10 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev) >>> if (err) >>> return err; >>> >>> + err = pm_runtime_force_resume(dev); >>> + if (err < 0) >>> + return err; >>> + >>> i2c_mark_adapter_resumed(&i2c_dev->adapter); >>> >>> return 0; >>> -- >>> 2.24.0 >>> >
13.12.2019 21:01, Dmitry Osipenko пишет: > 13.12.2019 17:04, Dmitry Osipenko пишет: >> 13.12.2019 16:47, Thierry Reding пишет: >>> On Fri, Dec 13, 2019 at 02:34:28AM +0300, Dmitry Osipenko wrote: >>>> I noticed that sometime I2C clock is kept enabled during suspend-resume. >>>> This happens because runtime PM defers dynamic suspension and thus it may >>>> happen that runtime PM is in active state when system enters into suspend. >>>> In particular I2C controller that is used for CPU's DVFS is often kept ON >>>> during suspend because CPU's voltage scaling happens quite often. >>>> >>>> Note: we marked runtime PM as IRQ-safe during the driver's probe in the >>>> "Support atomic transfers" patch, thus it's okay to enforce runtime PM >>>> suspend/resume in the NOIRQ phase which is used for the system-level >>>> suspend/resume of the driver. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>> >>> I've recently discussed this with Rafael in the context of runtime PM >>> support in the Tegra DRM driver and my understanding is that you're not >>> supposed to force runtime PM suspension like this. >>> >>> I had meant to send out an alternative patch to fix this, which I've >>> done now: >>> >>> http://patchwork.ozlabs.org/patch/1209148/ >>> >>> That's more in line with what Rafael and I had discussed in the other >>> thread and should address the issue that you're seeing as well. >> >> Well, either me or you are still having some misunderstanding of the >> runtime PM :) To my knowledge there are a lot of drivers that enforce >> suspension of the runtime PM during system's suspend, it should be a >> right thing to do especially in a context of the Tegra I2C driver >> because we're using asynchronous pm_runtime_put() and thus at the time >> of system's suspending, the runtime PM could be ON (as I wrote in the >> commit message) and then Terga's I2C driver manually disables the clock >> on resume (woopsie). > > Actually, looks like it's not the asynchronous pm_runtime_put() is the > cause of suspending in active state. I see that only one of three I2C > controllers is suspended in the enabled state, maybe some child (I2C > client) device keeps it awake, will try to find out. > >> By invoking pm_runtime_force_suspend() on systems's suspend, the runtime >> PM executes tegra_i2c_runtime_suspend() if device is in active state. On >> system resume, pm_runtime_force_resume() either keeps device in a >> suspended state or resumes it, say if for userspace disabled the runtime >> PM for the I2C controller. >> >> Rafael, could you please clarify whether my patch is doing a wrong thing? [snip] I'm now thinking that it will be not very worthwhile to spend time on trying to understand why runtime PM isn't working as expected here. It will be better to simply remove runtime PM from the I2C driver because it is used only for clock-gating/pinmuxing and it is a very light operation in comparison to I2C transfer performance. Thus it should be better to avoid the runtime PM overhead by enabling/disabling the I2C clocks before/after the transfer, I think that's what driver did before the runtime PM addition. Thierry / Jon / Mikko, any objections?
13.12.2019 17:55, Dmitry Osipenko пишет: > 13.12.2019 17:29, Dmitry Osipenko пишет: >> 13.12.2019 02:34, Dmitry Osipenko пишет: >>> I noticed that sometime I2C clock is kept enabled during suspend-resume. >>> This happens because runtime PM defers dynamic suspension and thus it may >>> happen that runtime PM is in active state when system enters into suspend. >>> In particular I2C controller that is used for CPU's DVFS is often kept ON >>> during suspend because CPU's voltage scaling happens quite often. >>> >>> Note: we marked runtime PM as IRQ-safe during the driver's probe in the >>> "Support atomic transfers" patch, thus it's okay to enforce runtime PM >>> suspend/resume in the NOIRQ phase which is used for the system-level >>> suspend/resume of the driver. >>> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >>> index b3ecdd87e91f..d309a314f4d6 100644 >>> --- a/drivers/i2c/busses/i2c-tegra.c >>> +++ b/drivers/i2c/busses/i2c-tegra.c >>> @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev) >>> static int __maybe_unused tegra_i2c_suspend(struct device *dev) >>> { >>> struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); >>> + int err; >>> >>> i2c_mark_adapter_suspended(&i2c_dev->adapter); >> >> I'm now in a doubt that it is correct to use NOIRQ level at all for the >> suspend because i2c_mark_adapter_suspended() uses mutex, thus I'm >> wondering what will happen if there is an asynchronous transfer >> happening during suspend.. >> >> The i2c_mark_adapter_suspended() will try to block and will never return? > > Moreover, the I2C interrupt should be disabled during the NOIRQ phase. > So, yes.. looks like making use of NOIRQ level wasn't a correct > decision. On the other hand, I don't think that any I2C client driver > used by Tegra SoCs in the upstream kernel could cause the problem at the > moment, so it shouldn't be critical. > > BTW: Jon, please CC me next time ;) [I'll try to find a better solution > for the PCIE problem] On a second thought, the NOIRQ shouldn't really cause any big problems because if something executes I2C asynchronously, then the transfer should simply timeout.
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index b3ecdd87e91f..d309a314f4d6 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev) static int __maybe_unused tegra_i2c_suspend(struct device *dev) { struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); + int err; i2c_mark_adapter_suspended(&i2c_dev->adapter); + err = pm_runtime_force_suspend(dev); + if (err < 0) + return err; + return 0; } @@ -1813,6 +1818,10 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev) if (err) return err; + err = pm_runtime_force_resume(dev); + if (err < 0) + return err; + i2c_mark_adapter_resumed(&i2c_dev->adapter); return 0;
I noticed that sometime I2C clock is kept enabled during suspend-resume. This happens because runtime PM defers dynamic suspension and thus it may happen that runtime PM is in active state when system enters into suspend. In particular I2C controller that is used for CPU's DVFS is often kept ON during suspend because CPU's voltage scaling happens quite often. Note: we marked runtime PM as IRQ-safe during the driver's probe in the "Support atomic transfers" patch, thus it's okay to enforce runtime PM suspend/resume in the NOIRQ phase which is used for the system-level suspend/resume of the driver. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ 1 file changed, 9 insertions(+)