* [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback @ 2014-01-28 5:48 xinhui.pan 2014-01-28 18:30 ` Wolfram Sang 0 siblings, 1 reply; 7+ messages in thread From: xinhui.pan @ 2014-01-28 5:48 UTC (permalink / raw) To: linux-kernel Cc: linux-i2c, wsa, mika.westerberg, andriy.shevchenko, yanmin_zhang, bo.he From: "xinhui.pan" <xinhuix.pan@intel.com> i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. Signed-off-by: bo.he <bo.he@intel.com> Signed-off-by: xinhui.pan <xinhuix.pan@intel.com> --- drivers/i2c/busses/i2c-designware-pcidrv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index f6ed06c..96e81f6 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) int err = pm_schedule_suspend(dev, 500); dev_dbg(dev, "runtime_idle called\n"); - if (err != 0) - return 0; + if (err) + return err; return -EBUSY; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback 2014-01-28 5:48 [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback xinhui.pan @ 2014-01-28 18:30 ` Wolfram Sang 2014-01-29 2:03 ` xinhui.pan 2014-01-29 8:35 ` Mika Westerberg 0 siblings, 2 replies; 7+ messages in thread From: Wolfram Sang @ 2014-01-28 18:30 UTC (permalink / raw) To: xinhui.pan Cc: linux-kernel, linux-i2c, mika.westerberg, andriy.shevchenko, yanmin_zhang, bo.he [-- Attachment #1: Type: text/plain, Size: 1256 bytes --] On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: > From: "xinhui.pan" <xinhuix.pan@intel.com> > > i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. I don't understand... > Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. > > Signed-off-by: bo.he <bo.he@intel.com> > Signed-off-by: xinhui.pan <xinhuix.pan@intel.com> > --- > drivers/i2c/busses/i2c-designware-pcidrv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c > index f6ed06c..96e81f6 100644 > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c > @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) > int err = pm_schedule_suspend(dev, 500); > dev_dbg(dev, "runtime_idle called\n"); > > - if (err != 0) > - return 0; > + if (err) > + return err; > return -EBUSY; ... it does return EBUSY when pm_schedule_suspend() succeeds? It only returns 0 if it does not succeed (for which I don't know if this is an apropriate behaviour). Mika? > } > > -- > 1.7.9.5 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback 2014-01-28 18:30 ` Wolfram Sang @ 2014-01-29 2:03 ` xinhui.pan 2014-01-29 7:35 ` xinhui.pan 2014-01-29 8:35 ` Mika Westerberg 1 sibling, 1 reply; 7+ messages in thread From: xinhui.pan @ 2014-01-29 2:03 UTC (permalink / raw) To: Wolfram Sang Cc: linux-kernel, linux-i2c, mika.westerberg, andriy.shevchenko, yanmin_zhang, bo.he > On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: >> From: "xinhui.pan" <xinhuix.pan@intel.com> >> >> i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. > > I don't understand... > Sorry for my poor English. Even if i2c_dw_pci_runtime_idle succeed ,it should return -EBUSY. >> Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. >> >> Signed-off-by: bo.he <bo.he@intel.com> >> Signed-off-by: xinhui.pan <xinhuix.pan@intel.com> >> --- >> drivers/i2c/busses/i2c-designware-pcidrv.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c >> index f6ed06c..96e81f6 100644 >> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >> @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) >> int err = pm_schedule_suspend(dev, 500); >> dev_dbg(dev, "runtime_idle called\n"); >> >> - if (err != 0) >> - return 0; >> + if (err) >> + return err; >> return -EBUSY; > > ... it does return EBUSY when pm_schedule_suspend() succeeds? It only > returns 0 if it does not succeed (for which I don't know if this is an > apropriate behaviour). Mika? > >> } >> >> -- >> 1.7.9.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback 2014-01-29 2:03 ` xinhui.pan @ 2014-01-29 7:35 ` xinhui.pan 0 siblings, 0 replies; 7+ messages in thread From: xinhui.pan @ 2014-01-29 7:35 UTC (permalink / raw) To: Wolfram Sang Cc: linux-kernel, linux-i2c, mika.westerberg, andriy.shevchenko, yanmin_zhang, bo.he 于 2014年01月29日 10:03, xinhui.pan 写道: > >> On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: >>> From: "xinhui.pan" <xinhuix.pan@intel.com> >>> >>> i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. >> >> I don't understand... >> > Sorry for my poor English. > Even if i2c_dw_pci_runtime_idle succeed ,it should return -EBUSY. >>> Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. >>> >>> Signed-off-by: bo.he <bo.he@intel.com> >>> Signed-off-by: xinhui.pan <xinhuix.pan@intel.com> >>> --- >>> drivers/i2c/busses/i2c-designware-pcidrv.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c >>> index f6ed06c..96e81f6 100644 >>> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >>> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >>> @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) >>> int err = pm_schedule_suspend(dev, 500); >>> dev_dbg(dev, "runtime_idle called\n"); >>> >>> - if (err != 0) >>> - return 0; >>> + if (err) >>> + return err; >>> return -EBUSY; >> >> ... it does return EBUSY when pm_schedule_suspend() succeeds? It only >> returns 0 if it does not succeed (for which I don't know if this is an >> apropriate behaviour). Mika? >> hi , I found one sentence in /Documentation/power/runtime_pm.txt "If there is no idle callback, or if the callback returns 0, then the PM core will attempt to carry out a runtime suspend of the device, also respecting devices configured for autosuspend." so is this a right way to prevent this? Br. xinhui >>> } >>> >>> -- >>> 1.7.9.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback 2014-01-28 18:30 ` Wolfram Sang 2014-01-29 2:03 ` xinhui.pan @ 2014-01-29 8:35 ` Mika Westerberg 2014-01-29 10:00 ` xinhui.pan 1 sibling, 1 reply; 7+ messages in thread From: Mika Westerberg @ 2014-01-29 8:35 UTC (permalink / raw) To: Wolfram Sang Cc: xinhui.pan, linux-kernel, linux-i2c, andriy.shevchenko, yanmin_zhang, bo.he On Tue, Jan 28, 2014 at 07:30:35PM +0100, Wolfram Sang wrote: > On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: > > From: "xinhui.pan" <xinhuix.pan@intel.com> > > > > i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. > > I don't understand... > > > Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. > > > > Signed-off-by: bo.he <bo.he@intel.com> > > Signed-off-by: xinhui.pan <xinhuix.pan@intel.com> > > --- > > drivers/i2c/busses/i2c-designware-pcidrv.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c > > index f6ed06c..96e81f6 100644 > > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c > > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c > > @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) > > int err = pm_schedule_suspend(dev, 500); > > dev_dbg(dev, "runtime_idle called\n"); > > > > - if (err != 0) > > - return 0; > > + if (err) > > + return err; > > return -EBUSY; > > ... it does return EBUSY when pm_schedule_suspend() succeeds? It only > returns 0 if it does not succeed (for which I don't know if this is an > apropriate behaviour). Mika? If I understand correctly, pm_schedule_suspend(dev, 500) is there because we want to runtime suspend in 500 ms. It then returns -EBUSY to prevent PM runtime from carrying on suspend on it's own. However, I have no idea where this magical 500 ms requirement comes from. If we fail to schedule suspend we let the PM core to do whatever it thinks suitable (in this case I suppose it suspends the device). I think the whole idle dance could be replaced with a use of runtime PM autosuspend, just like we do in the platform version of the driver. Xinghui, Is this a real problem that you are trying to solve? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback 2014-01-29 8:35 ` Mika Westerberg @ 2014-01-29 10:00 ` xinhui.pan 2014-01-29 10:38 ` Mika Westerberg 0 siblings, 1 reply; 7+ messages in thread From: xinhui.pan @ 2014-01-29 10:00 UTC (permalink / raw) To: Mika Westerberg, Wolfram Sang Cc: linux-kernel, linux-i2c, andriy.shevchenko, yanmin_zhang, bo.he 于 2014年01月29日 16:35, Mika Westerberg 写道: > On Tue, Jan 28, 2014 at 07:30:35PM +0100, Wolfram Sang wrote: >> On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: >>> From: "xinhui.pan" <xinhuix.pan@intel.com> >>> >>> i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. >> >> I don't understand... >> >>> Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. >>> >>> Signed-off-by: bo.he <bo.he@intel.com> >>> Signed-off-by: xinhui.pan <xinhuix.pan@intel.com> >>> --- >>> drivers/i2c/busses/i2c-designware-pcidrv.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c >>> index f6ed06c..96e81f6 100644 >>> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >>> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >>> @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) >>> int err = pm_schedule_suspend(dev, 500); >>> dev_dbg(dev, "runtime_idle called\n"); >>> >>> - if (err != 0) >>> - return 0; >>> + if (err) >>> + return err; >>> return -EBUSY; >> >> ... it does return EBUSY when pm_schedule_suspend() succeeds? It only >> returns 0 if it does not succeed (for which I don't know if this is an >> apropriate behaviour). Mika? > > If I understand correctly, pm_schedule_suspend(dev, 500) is there because > we want to runtime suspend in 500 ms. It then returns -EBUSY to prevent PM > runtime from carrying on suspend on it's own. However, I have no idea where > this magical 500 ms requirement comes from. > > If we fail to schedule suspend we let the PM core to do whatever it thinks > suitable (in this case I suppose it suspends the device). > Hi ,Mika If the callback returns 0,it means pm_schedule_suspend fails, also means rpm_check_suspend_allowed(pm_schedule_suspend calls it) returns nonzero value.As a result,rpm_suspend will be called by rpm_idle. However in rpm_idle, rpm_check_suspend_allowed is called at first,too. and the return value is treated as it is.But rpm_idle just returns without doing anything(rpm_suspend is not called). in both case above,why goes in different ways? I am confused. > I think the whole idle dance could be replaced with a use of runtime PM > autosuspend, just like we do in the platform version of the driver. > > Xinghui, > > Is this a real problem that you are trying to solve? > To be honest,we got many panic when testing. But is not caused by this driver I think. while checking problems,we found these confusing codes by accident. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback 2014-01-29 10:00 ` xinhui.pan @ 2014-01-29 10:38 ` Mika Westerberg 0 siblings, 0 replies; 7+ messages in thread From: Mika Westerberg @ 2014-01-29 10:38 UTC (permalink / raw) To: xinhui.pan Cc: Wolfram Sang, linux-kernel, linux-i2c, andriy.shevchenko, yanmin_zhang, bo.he On Wed, Jan 29, 2014 at 06:00:26PM +0800, xinhui.pan wrote: > > > 于 2014年01月29日 16:35, Mika Westerberg 写道: > > On Tue, Jan 28, 2014 at 07:30:35PM +0100, Wolfram Sang wrote: > >> On Tue, Jan 28, 2014 at 01:48:28PM +0800, xinhui.pan wrote: > >>> From: "xinhui.pan" <xinhuix.pan@intel.com> > >>> > >>> i2c_dw_pci_runtime_idle should return -EBUSY rather than zero if it do success. > >> > >> I don't understand... > >> > >>> Otherwise rpm_idle will call pm_suspend again and that may cause pm_schedule_suspend delay invalidate. > >>> > >>> Signed-off-by: bo.he <bo.he@intel.com> > >>> Signed-off-by: xinhui.pan <xinhuix.pan@intel.com> > >>> --- > >>> drivers/i2c/busses/i2c-designware-pcidrv.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c > >>> index f6ed06c..96e81f6 100644 > >>> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c > >>> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c > >>> @@ -190,8 +190,8 @@ static int i2c_dw_pci_runtime_idle(struct device *dev) > >>> int err = pm_schedule_suspend(dev, 500); > >>> dev_dbg(dev, "runtime_idle called\n"); > >>> > >>> - if (err != 0) > >>> - return 0; > >>> + if (err) > >>> + return err; > >>> return -EBUSY; > >> > >> ... it does return EBUSY when pm_schedule_suspend() succeeds? It only > >> returns 0 if it does not succeed (for which I don't know if this is an > >> apropriate behaviour). Mika? > > > > If I understand correctly, pm_schedule_suspend(dev, 500) is there because > > we want to runtime suspend in 500 ms. It then returns -EBUSY to prevent PM > > runtime from carrying on suspend on it's own. However, I have no idea where > > this magical 500 ms requirement comes from. > > > > If we fail to schedule suspend we let the PM core to do whatever it thinks > > suitable (in this case I suppose it suspends the device). > > > > Hi ,Mika > If the callback returns 0,it means pm_schedule_suspend fails, > also means rpm_check_suspend_allowed(pm_schedule_suspend calls it) > returns nonzero value.As a result,rpm_suspend will be called by rpm_idle. > However in rpm_idle, rpm_check_suspend_allowed is called at first,too. > and the return value is treated as it is.But rpm_idle just returns > without doing anything(rpm_suspend is not called). > in both case above,why goes in different ways? I am confused. To be honest, I don't know ;-) > > I think the whole idle dance could be replaced with a use of runtime PM > > autosuspend, just like we do in the platform version of the driver. > > > > Xinghui, > > > > Is this a real problem that you are trying to solve? > > > > To be honest,we got many panic when testing. > But is not caused by this driver I think. > while checking problems,we found these confusing codes by accident. OK, so if this is not a problem, I would suggest keeping the code as is for now as someone who introduced the pm_schedule_suspend() thing probably tested it and it worked then. In fact, I think that the PCI part of that driver needs an audit since it does some PCI power management magic itself which drivers aren't supposed to do anymore. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-01-29 10:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-28 5:48 [PATCH] i2c-designware-pcidrv: fix the incorrect return of idle callback xinhui.pan 2014-01-28 18:30 ` Wolfram Sang 2014-01-29 2:03 ` xinhui.pan 2014-01-29 7:35 ` xinhui.pan 2014-01-29 8:35 ` Mika Westerberg 2014-01-29 10:00 ` xinhui.pan 2014-01-29 10:38 ` Mika Westerberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox