Hi Benjamin
The commit 1fb25cb8b83e85f5bf1a4adb3c9a254c4ce92405 "radeonfb: Fix resume
from D3Cold on some platforms" breaks resume from suspend to RAM on my
PowerBook (Model PowerBook5,8 with ATI Mobility Radeon 9600 M10).
On resume the pulsing led indicating sleep just changes to solid white
and nothing else happens.
Reverting this commit on top of the current Linus' tree fixes resume.
Gaudenz
--
Ever tried. Ever failed. No matter.
Try again. Fail again. Fail better.
~ Samuel Beckett ~
On Wed, 2009-03-04 at 09:38 +0100, Gaudenz Steinlin wrote:
> Hi Benjamin
>
> The commit 1fb25cb8b83e85f5bf1a4adb3c9a254c4ce92405 "radeonfb: Fix resume
> from D3Cold on some platforms" breaks resume from suspend to RAM on my
> PowerBook (Model PowerBook5,8 with ATI Mobility Radeon 9600 M10).
>
> On resume the pulsing led indicating sleep just changes to solid white
> and nothing else happens.
>
> Reverting this commit on top of the current Linus' tree fixes resume.
>
> Gaudenz
Does it help if you comment out the call to:
pmac_set_early_video_resume(radeonfb_early_resume, rinfo);
>From radeonfb_pm_init() ?
Cheers,
Ben.
On Wed, 2009-03-04 at 09:38 +0100, Gaudenz Steinlin wrote:
> Hi Benjamin
>
> The commit 1fb25cb8b83e85f5bf1a4adb3c9a254c4ce92405 "radeonfb: Fix resume
> from D3Cold on some platforms" breaks resume from suspend to RAM on my
> PowerBook (Model PowerBook5,8 with ATI Mobility Radeon 9600 M10).
>
> On resume the pulsing led indicating sleep just changes to solid white
> and nothing else happens.
>
> Reverting this commit on top of the current Linus' tree fixes resume.
Ok, so after some tests here on what appear to the the exact same
machine...
I did the commit specifically to -fix- a regression due to upstream
changes, ie, for me it doesn't work without that commit. That is very
weird.
In fact, I just tried also with AGP and DRM enabled with X wobbly
windows etc... and it's working just fine.
I know that the early resume hack I have in there for powerbooks might
be a bit fishy, so please try without it (according to my previous email
on that matter).
Please let me know if that makes a difference.
Cheers,
Ben.
On Thu, Mar 05, 2009 at 10:22:04AM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-03-04 at 09:38 +0100, Gaudenz Steinlin wrote:
> > Hi Benjamin
> >
> > The commit 1fb25cb8b83e85f5bf1a4adb3c9a254c4ce92405 "radeonfb: Fix resume
> > from D3Cold on some platforms" breaks resume from suspend to RAM on my
> > PowerBook (Model PowerBook5,8 with ATI Mobility Radeon 9600 M10).
> >
> > On resume the pulsing led indicating sleep just changes to solid white
> > and nothing else happens.
> >
> > Reverting this commit on top of the current Linus' tree fixes resume.
> >
> > Gaudenz
>
> Does it help if you comment out the call to:
>
> pmac_set_early_video_resume(radeonfb_early_resume, rinfo);
>
> >From radeonfb_pm_init() ?
No this doesn't help. the only change is, that the led goes off on
resume instead of on. The machine is still hanging and I have to turn it
if by pressing the power button for some seconds.
It might be that the kernel configuration matters. While orginally
bisecting the problem I initially tried with a reduced config to speed
up complilation and suddenly even previously working versions had the
bug. I then reverted to bisecting with my original kernel config.
I'm now trying to compile a kernel with pmac32_defconfig to see if that
works. I attached my orignial config. If you send me your config I can
also try with that.
Gaudenz
--
Ever tried. Ever failed. No matter.
Try again. Fail again. Fail better.
~ Samuel Beckett ~
On Thu, Mar 05, 2009 at 01:25:20PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-03-04 at 09:38 +0100, Gaudenz Steinlin wrote:
> > Hi Benjamin
> >
> > The commit 1fb25cb8b83e85f5bf1a4adb3c9a254c4ce92405 "radeonfb: Fix resume
> > from D3Cold on some platforms" breaks resume from suspend to RAM on my
> > PowerBook (Model PowerBook5,8 with ATI Mobility Radeon 9600 M10).
> >
> > On resume the pulsing led indicating sleep just changes to solid white
> > and nothing else happens.
> >
> > Reverting this commit on top of the current Linus' tree fixes resume.
>
> Ok, so after some tests here on what appear to the the exact same
> machine...
>
> I did the commit specifically to -fix- a regression due to upstream
> changes, ie, for me it doesn't work without that commit. That is very
> weird.
>
> In fact, I just tried also with AGP and DRM enabled with X wobbly
> windows etc... and it's working just fine.
>
> I know that the early resume hack I have in there for powerbooks might
> be a bit fishy, so please try without it (according to my previous email
> on that matter).
>
> Please let me know if that makes a difference.
As already said in an earlier mail, this does not make any difference. I
now ran some more tests:
-> with ppc32_defconfig: same results, resume with your commit is
broken, I did not yet test if reverting the commit fixes resume.
Please tell me if you are interested in this result.
-> with a minimal config (attached): resume does not work at all with or
without your commit. I suspect this is another bug with the same
symptoms.
If you have any ideas, I would be happy to do further tests. I could
also send you my compiles kernel (as a Debian package or a tar file) or
you could send me your working kernel image to see if it also works for
me.
Gaudenz
--
Ever tried. Ever failed. No matter.
Try again. Fail again. Fail better.
~ Samuel Beckett ~
> As already said in an earlier mail, this does not make any difference. I
> now ran some more tests:
>
> -> with ppc32_defconfig: same results, resume with your commit is
> broken, I did not yet test if reverting the commit fixes resume.
> Please tell me if you are interested in this result.
> -> with a minimal config (attached): resume does not work at all with or
> without your commit. I suspect this is another bug with the same
> symptoms.
>
> If you have any ideas, I would be happy to do further tests. I could
> also send you my compiles kernel (as a Debian package or a tar file) or
> you could send me your working kernel image to see if it also works for
> me.
I think that .config fails due to the lack of CPU_FREQ support. I think
I remember that specifically this powerbook model boots at low speed and
fails to resume if not suspended at high speed. The cpufreq driver for
powermacs knows about that and ramps up the frequency before suspend.
Can you give that a try ?
In fact I verified on mine that your minimum config fails the same way
you described but it works when I re-enable cpufreq support.
We might want to do something to warn people in this situation.. maybe
the sleep code can somewhat verify that cpufreq is there.
Cheers,
Ben.
On Fri, Mar 06, 2009 at 04:50:29PM +1100, Benjamin Herrenschmidt wrote:
>
> > As already said in an earlier mail, this does not make any difference. I
> > now ran some more tests:
> >
> > -> with ppc32_defconfig: same results, resume with your commit is
> > broken, I did not yet test if reverting the commit fixes resume.
> > Please tell me if you are interested in this result.
> > -> with a minimal config (attached): resume does not work at all with or
> > without your commit. I suspect this is another bug with the same
> > symptoms.
> >
> > If you have any ideas, I would be happy to do further tests. I could
> > also send you my compiles kernel (as a Debian package or a tar file) or
> > you could send me your working kernel image to see if it also works for
> > me.
>
> I think that .config fails due to the lack of CPU_FREQ support. I think
> I remember that specifically this powerbook model boots at low speed and
> fails to resume if not suspended at high speed. The cpufreq driver for
> powermacs knows about that and ramps up the frequency before suspend.
>
> Can you give that a try ?
OK, with CPU_FREQ enabled I have the same results as with
ppc32_defconfig or my full config. It fails to resume with your commit
and it works if I revert that commit.
>
> In fact I verified on mine that your minimum config fails the same way
> you described but it works when I re-enable cpufreq support.
So you are able to resume with my minimal config + CPU_FREQ? This would
be really strange if it is really the exact same model.
To verify if we really have the same model I included some data below. I
also included the gcc and binutils versions.
Please tell me if you have any further ideas on what to test.
Gaudenz
/proc/cpuinfo:
processor : 0
cpu : 7447A, altivec supported
clock : 1666.666000MHz
revision : 1.5 (pvr 8003 0105)
bogomips : 33.15
timebase : 8320000
platform : PowerMac
model : PowerBook5,8
machine : PowerBook5,8
motherboard : PowerBook5,8 MacRISC3 Power Macintosh
detected as : 287 (PowerBook G4 15")
pmac flags : 00000019
L2 cache : 512K unified
pmac-generation : NewWorld
Memory : 1536 MB
lspci -n:
0000:00:0b.0 0600: 106b:0066
0000:00:10.0 0300: 1002:4e50
0001:10:0b.0 0600: 106b:0067
0001:10:11.0 0280: 14e4:4318 (rev 02)
0001:10:14.0 0607: 104c:ac56
0001:10:15.0 0c03: 1033:0035 (rev 43)
0001:10:15.1 0c03: 1033:0035 (rev 43)
0001:10:15.2 0c03: 1033:00e0 (rev 04)
0001:10:17.0 ff00: 106b:003e
0002:24:0b.0 0600: 106b:0068
0002:24:0d.0 ff00: 106b:0069
0002:24:0e.0 0c00: 106b:006a
0002:24:0f.0 0200: 106b:006b
gcc --version:
gcc (Debian 4.3.3-5) 4.3.3
ld --version:
GNU ld (GNU Binutils for Debian) 2.19.1
--
Ever tried. Ever failed. No matter.
Try again. Fail again. Fail better.
~ Samuel Beckett ~
> So you are able to resume with my minimal config + CPU_FREQ? This would
> be really strange if it is really the exact same model.
Yes, it appears to be :-)
> To verify if we really have the same model I included some data below. I
> also included the gcc and binutils versions.
>
> Please tell me if you have any further ideas on what to test.
In the commmit your revert, I added a function that "tests" if the chip
appears to need to be POSTed: radeon_check_power_loss(). Try commenting
out the content and make it always return 1.
Another thing you can try in radeonfb_pci_resume():
if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
+ pci_restore_state(pdev);
+ pci_enable_device(pdev);
+ pci_set_master(pdev);
/* Wakeup chip */
And if that helps, then try to find out which of these 3 things helped.
Cheers,
Ben.
On Fri, Mar 06, 2009 at 08:57:20PM +1100, Benjamin Herrenschmidt wrote:
>
> > So you are able to resume with my minimal config + CPU_FREQ? This would
> > be really strange if it is really the exact same model.
>
> Yes, it appears to be :-)
>
> > To verify if we really have the same model I included some data below. I
> > also included the gcc and binutils versions.
> >
> > Please tell me if you have any further ideas on what to test.
>
> In the commmit your revert, I added a function that "tests" if the chip
> appears to need to be POSTed: radeon_check_power_loss(). Try commenting
> out the content and make it always return 1.
This does not help.
>
> Another thing you can try in radeonfb_pci_resume():
>
> if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
> + pci_restore_state(pdev);
Adding this fixes the bug. Apparently the PCI core does not fully
restore the state. Before your suggestions I also tried to find out
which part of your commit breaks resume and I found out that if I
reinsert the parts to save and restore the pci configuration the bug is
fixed. It seems that somehow the PCI coniguration is not fully restored [1].
> + pci_enable_device(pdev);
This does not help.
> + pci_set_master(pdev);
This does not help.
Gaudenz
[1] These are just my wild guesses. I don't really understand much of
the code involved.
--
Ever tried. Ever failed. No matter.
Try again. Fail again. Fail better.
~ Samuel Beckett ~
On Fri, 2009-03-06 at 12:41 +0100, Gaudenz Steinlin wrote:
>
> Adding this fixes the bug. Apparently the PCI core does not fully
> restore the state. Before your suggestions I also tried to find out
> which part of your commit breaks resume and I found out that if I
> reinsert the parts to save and restore the pci configuration the bug
> is
> fixed. It seems that somehow the PCI coniguration is not fully
> restored [1].
Ok, well, I though it would be due to some upstream changes but it's
possible that I either mis-read those changes or they got themselves
changed in some subtle way after I did that commit :-)
I still find it fascinating that it doesn't break for me here since it
is effectively the same machine model !
I'll do a patch fixing that but it will have to wait for monday.
Cheers,
Ben.
On Fri, 2009-03-06 at 12:41 +0100, Gaudenz Steinlin wrote:
> > Another thing you can try in radeonfb_pci_resume():
> >
> > if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
> > + pci_restore_state(pdev);
>
> Adding this fixes the bug. Apparently the PCI core does not fully
> restore the state. Before your suggestions I also tried to find out
> which part of your commit breaks resume and I found out that if I
> reinsert the parts to save and restore the pci configuration the bug is
> fixed. It seems that somehow the PCI coniguration is not fully restored [1].
Ok so this doesn't make sense to me at this stage... I see two
possibilities:
1- You haven't properly done the test disabling the early resume hack
(ie, you may have done it with also CPU_FREQ disabled for example) and
got a false negative there. The platform code calls into the early
resume stuff before the PCI core gets a chance to restore things so that
would be an explanation. I'll send a patch fixing that.
or
2- For some reason, the core call to pci_raw_set_power_state() from
pci_restore_standard_config() is returning an error. That would cause
the later not to restore the rest of the config.
So what I suggest is that while keeping your added pci_restore_state()
in there, you also add some printk's in pci_restore_standard_config() to
see anything weird happens in there or if it appears to properly call
pci_restore_state(). It would be useful for us to know.
Cheers,
Ben.
Please also test this patch...
Cheers,
Ben.
radeonfb/aty128fb: Disabled broken early resume hack for PowerBooks
radeonfb and aty128fb have a special hook called by the PowerMac platform
code very very early on resume from sleep to bring the screen back. This
is useful for debugging wakup problems, but unfortunately, this also became
a source of problems of its own.
The hook is called extremely early, with interrupts still off, and the code
path involved with that code nowadays rely on things like taking mutexes,
GFP_KERNEL allocations, etc...
In addition, the driver now relies on the PCI core to restore the standard
config space before calling resume which doesn't happen with this early
code path.
I'm keeping the code in but commented out along with a fixup call to
pci_restore_state(). The reason is that I still want to make it easy to
re-enable temporarily to track wake up problems, and it's possible that
I can revive it at some stage if we make sleeping things save to call
in early resume using a system state.
In the meantime, this should fix several reported regressions.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
Index: linux-work/drivers/video/aty/radeon_pm.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_pm.c 2009-03-10 09:38:25.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_pm.c 2009-03-10 09:40:51.000000000 +1100
@@ -2762,12 +2762,13 @@ int radeonfb_pci_resume(struct pci_dev *
return rc;
}
-#ifdef CONFIG_PPC_OF
+#ifdef CONFIG_PPC_OF__disabled
static void radeonfb_early_resume(void *data)
{
struct radeonfb_info *rinfo = data;
rinfo->no_schedule = 1;
+ pci_restore_state(rinfo->pdev);
radeonfb_pci_resume(rinfo->pdev);
rinfo->no_schedule = 0;
}
@@ -2834,7 +2835,14 @@ void radeonfb_pm_init(struct radeonfb_in
*/
if (rinfo->pm_mode != radeon_pm_none) {
pmac_call_feature(PMAC_FTR_DEVICE_CAN_WAKE, rinfo->of_node, 0, 1);
+#if 0 /* Disable the early video resume hack for now as it's causing problems, among
+ * others we now rely on the PCI core restoring the config space for us, which
+ * isn't the case with that hack, and that code path causes various things to
+ * be called with interrupts off while they shouldn't. I'm leaving the code in
+ * as it can be useful for debugging purposes
+ */
pmac_set_early_video_resume(radeonfb_early_resume, rinfo);
+#endif
}
#if 0
Index: linux-work/drivers/video/aty/aty128fb.c
===================================================================
--- linux-work.orig/drivers/video/aty/aty128fb.c 2009-03-10 09:41:21.000000000 +1100
+++ linux-work/drivers/video/aty/aty128fb.c 2009-03-10 09:41:44.000000000 +1100
@@ -1853,13 +1853,14 @@ static void aty128_bl_exit(struct backli
* Initialisation
*/
-#ifdef CONFIG_PPC_PMAC
+#ifdef CONFIG_PPC_PMAC__disabled
static void aty128_early_resume(void *data)
{
struct aty128fb_par *par = data;
if (try_acquire_console_sem())
return;
+ pci_restore_state(par->pdev);
aty128_do_resume(par->pdev);
release_console_sem();
}
@@ -1907,7 +1908,14 @@ static int __devinit aty128_init(struct
/* Indicate sleep capability */
if (par->chip_gen == rage_M3) {
pmac_call_feature(PMAC_FTR_DEVICE_CAN_WAKE, NULL, 0, 1);
+#if 0 /* Disable the early video resume hack for now as it's causing problems, among
+ * others we now rely on the PCI core restoring the config space for us, which
+ * isn't the case with that hack, and that code path causes various things to
+ * be called with interrupts off while they shouldn't. I'm leaving the code in
+ * as it can be useful for debugging purposes
+ */
pmac_set_early_video_resume(aty128_early_resume, par);
+#endif
}
/* Find default mode */
On Monday 09 March 2009, Benjamin Herrenschmidt wrote:
> On Fri, 2009-03-06 at 12:41 +0100, Gaudenz Steinlin wrote:
>
> > > Another thing you can try in radeonfb_pci_resume():
> > >
> > > if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
> > > + pci_restore_state(pdev);
> >
> > Adding this fixes the bug. Apparently the PCI core does not fully
> > restore the state. Before your suggestions I also tried to find out
> > which part of your commit breaks resume and I found out that if I
> > reinsert the parts to save and restore the pci configuration the bug is
> > fixed. It seems that somehow the PCI coniguration is not fully restored [1].
>
> Ok so this doesn't make sense to me at this stage... I see two
> possibilities:
>
> 1- You haven't properly done the test disabling the early resume hack
> (ie, you may have done it with also CPU_FREQ disabled for example) and
> got a false negative there. The platform code calls into the early
> resume stuff before the PCI core gets a chance to restore things so that
> would be an explanation. I'll send a patch fixing that.
>
> or
>
> 2- For some reason, the core call to pci_raw_set_power_state() from
> pci_restore_standard_config() is returning an error. That would cause
> the later not to restore the rest of the config.
>
> So what I suggest is that while keeping your added pci_restore_state()
> in there, you also add some printk's in pci_restore_standard_config() to
> see anything weird happens in there or if it appears to properly call
> pci_restore_state(). It would be useful for us to know.
Gaudenz, I'd also like to know if the appended patch (on top of vanilla Linus'
tree) makes any different. Unfortunately, I haven't had the time to test it
myself yet.
Thanks,
Rafael
---
drivers/pci/pci-driver.c | 42 ++++++++++++++++++++++++++++++------------
1 file changed, 30 insertions(+), 12 deletions(-)
Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -556,7 +556,7 @@ static int pci_pm_suspend_noirq(struct d
static int pci_pm_resume_noirq(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
- struct device_driver *drv = dev->driver;
+ struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
int error = 0;
pci_pm_default_resume_noirq(pci_dev);
@@ -564,8 +564,13 @@ static int pci_pm_resume_noirq(struct de
if (pci_has_legacy_pm_support(pci_dev))
return pci_legacy_resume_early(dev);
- if (drv && drv->pm && drv->pm->resume_noirq)
- error = drv->pm->resume_noirq(dev);
+ if (pm) {
+ if (pm->resume_noirq)
+ error = pm->resume_noirq(dev);
+ } else {
+ if (pci_is_bridge(pci_dev))
+ pci_pm_reenable_device(pci_dev);
+ }
return error;
}
@@ -592,7 +597,8 @@ static int pci_pm_resume(struct device *
if (pm->resume)
error = pm->resume(dev);
} else {
- pci_pm_reenable_device(pci_dev);
+ if (!pci_is_bridge(pci_dev))
+ pci_pm_reenable_device(pci_dev);
}
return 0;
@@ -662,7 +668,7 @@ static int pci_pm_freeze_noirq(struct de
static int pci_pm_thaw_noirq(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
- struct device_driver *drv = dev->driver;
+ struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
int error = 0;
if (pci_has_legacy_pm_support(pci_dev))
@@ -670,8 +676,13 @@ static int pci_pm_thaw_noirq(struct devi
pci_update_current_state(pci_dev, PCI_D0);
- if (drv && drv->pm && drv->pm->thaw_noirq)
- error = drv->pm->thaw_noirq(dev);
+ if (pm) {
+ if (pm->thaw_noirq)
+ error = pm->thaw_noirq(dev);
+ } else {
+ if (pci_is_bridge(pci_dev))
+ pci_pm_reenable_device(pci_dev);
+ }
return error;
}
@@ -689,7 +700,8 @@ static int pci_pm_thaw(struct device *de
if (pm->thaw)
error = pm->thaw(dev);
} else {
- pci_pm_reenable_device(pci_dev);
+ if (!pci_is_bridge(pci_dev))
+ pci_pm_reenable_device(pci_dev);
}
return error;
@@ -744,7 +756,7 @@ static int pci_pm_poweroff_noirq(struct
static int pci_pm_restore_noirq(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
- struct device_driver *drv = dev->driver;
+ struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
int error = 0;
pci_pm_default_resume_noirq(pci_dev);
@@ -752,8 +764,13 @@ static int pci_pm_restore_noirq(struct d
if (pci_has_legacy_pm_support(pci_dev))
return pci_legacy_resume_early(dev);
- if (drv && drv->pm && drv->pm->restore_noirq)
- error = drv->pm->restore_noirq(dev);
+ if (pm) {
+ if (pm->restore_noirq)
+ error = pm->restore_noirq(dev);
+ } else {
+ if (pci_is_bridge(pci_dev))
+ pci_pm_reenable_device(pci_dev);
+ }
return error;
}
@@ -780,7 +797,8 @@ static int pci_pm_restore(struct device
if (pm->restore)
error = pm->restore(dev);
} else {
- pci_pm_reenable_device(pci_dev);
+ if (!pci_is_bridge(pci_dev))
+ pci_pm_reenable_device(pci_dev);
}
return error;
Hi Rafael
On Tue, Mar 10, 2009 at 10:32:58AM +0100, Rafael J. Wysocki wrote:
>
> Gaudenz, I'd also like to know if the appended patch (on top of vanilla Linus'
> tree) makes any different. Unfortunately, I haven't had the time to test it
> myself yet.
No, this patch does not make any difference.
Gaudenz
>
> Thanks,
> Rafael
>
> ---
> drivers/pci/pci-driver.c | 42 ++++++++++++++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 12 deletions(-)
>
> Index: linux-2.6/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci-driver.c
> +++ linux-2.6/drivers/pci/pci-driver.c
> @@ -556,7 +556,7 @@ static int pci_pm_suspend_noirq(struct d
> static int pci_pm_resume_noirq(struct device *dev)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> - struct device_driver *drv = dev->driver;
> + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> int error = 0;
>
> pci_pm_default_resume_noirq(pci_dev);
> @@ -564,8 +564,13 @@ static int pci_pm_resume_noirq(struct de
> if (pci_has_legacy_pm_support(pci_dev))
> return pci_legacy_resume_early(dev);
>
> - if (drv && drv->pm && drv->pm->resume_noirq)
> - error = drv->pm->resume_noirq(dev);
> + if (pm) {
> + if (pm->resume_noirq)
> + error = pm->resume_noirq(dev);
> + } else {
> + if (pci_is_bridge(pci_dev))
> + pci_pm_reenable_device(pci_dev);
> + }
>
> return error;
> }
> @@ -592,7 +597,8 @@ static int pci_pm_resume(struct device *
> if (pm->resume)
> error = pm->resume(dev);
> } else {
> - pci_pm_reenable_device(pci_dev);
> + if (!pci_is_bridge(pci_dev))
> + pci_pm_reenable_device(pci_dev);
> }
>
> return 0;
> @@ -662,7 +668,7 @@ static int pci_pm_freeze_noirq(struct de
> static int pci_pm_thaw_noirq(struct device *dev)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> - struct device_driver *drv = dev->driver;
> + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> int error = 0;
>
> if (pci_has_legacy_pm_support(pci_dev))
> @@ -670,8 +676,13 @@ static int pci_pm_thaw_noirq(struct devi
>
> pci_update_current_state(pci_dev, PCI_D0);
>
> - if (drv && drv->pm && drv->pm->thaw_noirq)
> - error = drv->pm->thaw_noirq(dev);
> + if (pm) {
> + if (pm->thaw_noirq)
> + error = pm->thaw_noirq(dev);
> + } else {
> + if (pci_is_bridge(pci_dev))
> + pci_pm_reenable_device(pci_dev);
> + }
>
> return error;
> }
> @@ -689,7 +700,8 @@ static int pci_pm_thaw(struct device *de
> if (pm->thaw)
> error = pm->thaw(dev);
> } else {
> - pci_pm_reenable_device(pci_dev);
> + if (!pci_is_bridge(pci_dev))
> + pci_pm_reenable_device(pci_dev);
> }
>
> return error;
> @@ -744,7 +756,7 @@ static int pci_pm_poweroff_noirq(struct
> static int pci_pm_restore_noirq(struct device *dev)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> - struct device_driver *drv = dev->driver;
> + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> int error = 0;
>
> pci_pm_default_resume_noirq(pci_dev);
> @@ -752,8 +764,13 @@ static int pci_pm_restore_noirq(struct d
> if (pci_has_legacy_pm_support(pci_dev))
> return pci_legacy_resume_early(dev);
>
> - if (drv && drv->pm && drv->pm->restore_noirq)
> - error = drv->pm->restore_noirq(dev);
> + if (pm) {
> + if (pm->restore_noirq)
> + error = pm->restore_noirq(dev);
> + } else {
> + if (pci_is_bridge(pci_dev))
> + pci_pm_reenable_device(pci_dev);
> + }
>
> return error;
> }
> @@ -780,7 +797,8 @@ static int pci_pm_restore(struct device
> if (pm->restore)
> error = pm->restore(dev);
> } else {
> - pci_pm_reenable_device(pci_dev);
> + if (!pci_is_bridge(pci_dev))
> + pci_pm_reenable_device(pci_dev);
> }
>
> return error;
--
Ever tried. Ever failed. No matter.
Try again. Fail again. Fail better.
~ Samuel Beckett ~
On Tue, 2009-03-10 at 16:01 +0100, Gaudenz Steinlin wrote:
> Hi Rafael
>
> On Tue, Mar 10, 2009 at 10:32:58AM +0100, Rafael J. Wysocki wrote:
> >
> > Gaudenz, I'd also like to know if the appended patch (on top of vanilla Linus'
> > tree) makes any different. Unfortunately, I haven't had the time to test it
> > myself yet.
>
> No, this patch does not make any difference.
What about the one I sent you that removes the early wakeup ?
Cheers,
Ben.
> Gaudenz
>
> >
> > Thanks,
> > Rafael
> >
> > ---
> > drivers/pci/pci-driver.c | 42 ++++++++++++++++++++++++++++++------------
> > 1 file changed, 30 insertions(+), 12 deletions(-)
> >
> > Index: linux-2.6/drivers/pci/pci-driver.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/pci-driver.c
> > +++ linux-2.6/drivers/pci/pci-driver.c
> > @@ -556,7 +556,7 @@ static int pci_pm_suspend_noirq(struct d
> > static int pci_pm_resume_noirq(struct device *dev)
> > {
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > - struct device_driver *drv = dev->driver;
> > + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > int error = 0;
> >
> > pci_pm_default_resume_noirq(pci_dev);
> > @@ -564,8 +564,13 @@ static int pci_pm_resume_noirq(struct de
> > if (pci_has_legacy_pm_support(pci_dev))
> > return pci_legacy_resume_early(dev);
> >
> > - if (drv && drv->pm && drv->pm->resume_noirq)
> > - error = drv->pm->resume_noirq(dev);
> > + if (pm) {
> > + if (pm->resume_noirq)
> > + error = pm->resume_noirq(dev);
> > + } else {
> > + if (pci_is_bridge(pci_dev))
> > + pci_pm_reenable_device(pci_dev);
> > + }
> >
> > return error;
> > }
> > @@ -592,7 +597,8 @@ static int pci_pm_resume(struct device *
> > if (pm->resume)
> > error = pm->resume(dev);
> > } else {
> > - pci_pm_reenable_device(pci_dev);
> > + if (!pci_is_bridge(pci_dev))
> > + pci_pm_reenable_device(pci_dev);
> > }
> >
> > return 0;
> > @@ -662,7 +668,7 @@ static int pci_pm_freeze_noirq(struct de
> > static int pci_pm_thaw_noirq(struct device *dev)
> > {
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > - struct device_driver *drv = dev->driver;
> > + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > int error = 0;
> >
> > if (pci_has_legacy_pm_support(pci_dev))
> > @@ -670,8 +676,13 @@ static int pci_pm_thaw_noirq(struct devi
> >
> > pci_update_current_state(pci_dev, PCI_D0);
> >
> > - if (drv && drv->pm && drv->pm->thaw_noirq)
> > - error = drv->pm->thaw_noirq(dev);
> > + if (pm) {
> > + if (pm->thaw_noirq)
> > + error = pm->thaw_noirq(dev);
> > + } else {
> > + if (pci_is_bridge(pci_dev))
> > + pci_pm_reenable_device(pci_dev);
> > + }
> >
> > return error;
> > }
> > @@ -689,7 +700,8 @@ static int pci_pm_thaw(struct device *de
> > if (pm->thaw)
> > error = pm->thaw(dev);
> > } else {
> > - pci_pm_reenable_device(pci_dev);
> > + if (!pci_is_bridge(pci_dev))
> > + pci_pm_reenable_device(pci_dev);
> > }
> >
> > return error;
> > @@ -744,7 +756,7 @@ static int pci_pm_poweroff_noirq(struct
> > static int pci_pm_restore_noirq(struct device *dev)
> > {
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > - struct device_driver *drv = dev->driver;
> > + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > int error = 0;
> >
> > pci_pm_default_resume_noirq(pci_dev);
> > @@ -752,8 +764,13 @@ static int pci_pm_restore_noirq(struct d
> > if (pci_has_legacy_pm_support(pci_dev))
> > return pci_legacy_resume_early(dev);
> >
> > - if (drv && drv->pm && drv->pm->restore_noirq)
> > - error = drv->pm->restore_noirq(dev);
> > + if (pm) {
> > + if (pm->restore_noirq)
> > + error = pm->restore_noirq(dev);
> > + } else {
> > + if (pci_is_bridge(pci_dev))
> > + pci_pm_reenable_device(pci_dev);
> > + }
> >
> > return error;
> > }
> > @@ -780,7 +797,8 @@ static int pci_pm_restore(struct device
> > if (pm->restore)
> > error = pm->restore(dev);
> > } else {
> > - pci_pm_reenable_device(pci_dev);
> > + if (!pci_is_bridge(pci_dev))
> > + pci_pm_reenable_device(pci_dev);
> > }
> >
> > return error;
>
On Wed, Mar 11, 2009 at 07:58:46AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2009-03-10 at 16:01 +0100, Gaudenz Steinlin wrote:
> > Hi Rafael
> >
> > On Tue, Mar 10, 2009 at 10:32:58AM +0100, Rafael J. Wysocki wrote:
> > >
> > > Gaudenz, I'd also like to know if the appended patch (on top of vanilla Linus'
> > > tree) makes any different. Unfortunately, I haven't had the time to test it
> > > myself yet.
> >
> > No, this patch does not make any difference.
>
> What about the one I sent you that removes the early wakeup ?
Sorry, I first tested Rafaels patch. I now also found the time to test
your patch and it works. With this patch on top of todays Linus tree,
the resume problem is gone.
Gaudenz
--
Ever tried. Ever failed. No matter.
Try again. Fail again. Fail better.
~ Samuel Beckett ~
On Tue, Mar 10, 2009 at 09:37:44AM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2009-03-06 at 12:41 +0100, Gaudenz Steinlin wrote:
>
> > > Another thing you can try in radeonfb_pci_resume():
> > >
> > > if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
> > > + pci_restore_state(pdev);
> >
> > Adding this fixes the bug. Apparently the PCI core does not fully
> > restore the state. Before your suggestions I also tried to find out
> > which part of your commit breaks resume and I found out that if I
> > reinsert the parts to save and restore the pci configuration the bug is
> > fixed. It seems that somehow the PCI coniguration is not fully restored [1].
>
> Ok so this doesn't make sense to me at this stage... I see two
> possibilities:
>
> 1- You haven't properly done the test disabling the early resume hack
> (ie, you may have done it with also CPU_FREQ disabled for example) and
> got a false negative there. The platform code calls into the early
> resume stuff before the PCI core gets a chance to restore things so that
> would be an explanation. I'll send a patch fixing that.
>
> or
>
> 2- For some reason, the core call to pci_raw_set_power_state() from
> pci_restore_standard_config() is returning an error. That would cause
> the later not to restore the rest of the config.
>
> So what I suggest is that while keeping your added pci_restore_state()
> in there, you also add some printk's in pci_restore_standard_config() to
> see anything weird happens in there or if it appears to properly call
> pci_restore_state(). It would be useful for us to know.
Are you still interested in these tests after that disabling the early
resume hack fixed things? I can try my best to test and add printk's but
it will probably take some time because I can't constantly reboot my
computer and still do some usefull work...
Gaudenz
--
Ever tried. Ever failed. No matter.
Try again. Fail again. Fail better.
~ Samuel Beckett ~
On Tue, 2009-03-10 at 22:30 +0100, Gaudenz Steinlin wrote:
> Are you still interested in these tests after that disabling the early
> resume hack fixed things? I can try my best to test and add printk's
> but
> it will probably take some time because I can't constantly reboot my
> computer and still do some usefull work...
>
No thanks, I think the initial test with early resume was flawed due to
something like having cpufreq disabled.
Cheers,
Ben.
On Tuesday 10 March 2009, Benjamin Herrenschmidt wrote:
> On Tue, 2009-03-10 at 22:30 +0100, Gaudenz Steinlin wrote:
> > Are you still interested in these tests after that disabling the early
> > resume hack fixed things? I can try my best to test and add printk's
> > but
> > it will probably take some time because I can't constantly reboot my
> > computer and still do some usefull work...
> >
> No thanks, I think the initial test with early resume was flawed due to
> something like having cpufreq disabled.
Well, I've lost track a bit.
Which patch is going to be the final fix?
Rafael
On Wed, 2009-03-11 at 00:04 +0100, Rafael J. Wysocki wrote:
> On Tuesday 10 March 2009, Benjamin Herrenschmidt wrote:
> > On Tue, 2009-03-10 at 22:30 +0100, Gaudenz Steinlin wrote:
> > > Are you still interested in these tests after that disabling the early
> > > resume hack fixed things? I can try my best to test and add printk's
> > > but
> > > it will probably take some time because I can't constantly reboot my
> > > computer and still do some usefull work...
> > >
> > No thanks, I think the initial test with early resume was flawed due to
> > something like having cpufreq disabled.
>
> Well, I've lost track a bit.
>
> Which patch is going to be the final fix?
I sent it to Linus as part of a pull request, it removes my early resume
hooks for now, I'll look into fixing them after .29
Cheers,
Ben.