iommu breaks usb after resume

iommu breaks usb after resume

[Pavel Machek]

Hi!

On 64-bit machine with 4GB ram (8core from amd), iommu breaks USB
after swsusp. iommu=soft fixes that issue.

I guess iommu handling in arch/x86/kernel/ should have some
suspend/resume support, as it manipulates hardware registers...?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: iommu breaks usb after resume

[Ingo Molnar]

* Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> On 64-bit machine with 4GB ram (8core from amd), iommu breaks USB
> after swsusp. iommu=soft fixes that issue.
> 
> I guess iommu handling in arch/x86/kernel/ should have some
> suspend/resume support, as it manipulates hardware registers...?

agreed ...

	Ingo

Re: iommu breaks usb after resume

[Andi Kleen]

On Thu, Mar 20, 2008 at 12:14:29PM +0100, Pavel Machek wrote:
> Hi!
> 
> On 64-bit machine with 4GB ram (8core from amd), iommu breaks USB
> after swsusp. iommu=soft fixes that issue.
> 
> I guess iommu handling in arch/x86/kernel/ should have some
> suspend/resume support, as it manipulates hardware registers...?

AGP suspend/resume should already handle restoring of the aperture
and the IOMMU code uses the same.  It has not other state. It might not work 
when the AGP driver is not compiled in/loaded though (I suspect
that is what happened to you) One simple fix would be to just enforce
that in Kconfig when suspend/resume is enabled.

-Andi

Re: iommu breaks usb after resume

[Pavel Machek]

Hi!

> > On 64-bit machine with 4GB ram (8core from amd), iommu breaks USB
> > after swsusp. iommu=soft fixes that issue.
> > 
> > I guess iommu handling in arch/x86/kernel/ should have some
> > suspend/resume support, as it manipulates hardware registers...?
> 
> AGP suspend/resume should already handle restoring of the aperture
> and the IOMMU code uses the same.  It has not other state. It might not work 
> when the AGP driver is not compiled in/loaded though (I suspect
> that is what happened to you) One simple fix would be to just enforce
> that in Kconfig when suspend/resume is enabled.

I had:

CONFIG_AGP=y
CONFIG_AGP_AMD64=y
CONFIG_AGP_INTEL=y
# CONFIG_AGP_SIS is not set
CONFIG_AGP_VIA=y
CONFIG_DRM=y
# CONFIG_DRM_TDFX is not set

...do I need to enable something more?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: iommu breaks usb after resume

[Andi Kleen]

> CONFIG_AGP=y
> CONFIG_AGP_AMD64=y
> CONFIG_AGP_INTEL=y
> # CONFIG_AGP_SIS is not set
> CONFIG_AGP_VIA=y
> CONFIG_DRM=y
> # CONFIG_DRM_TDFX is not set
> 
> ...do I need to enable something more?

Should have worked then. Ok modulo bugs. Maybe the ordering
is wrong now (AGP resume would need to run before anything
using the IOMMU)

-Andi

Re: iommu breaks usb after resume

[Pavel Machek]

Hi!

> > CONFIG_AGP=y
> > CONFIG_AGP_AMD64=y
> > CONFIG_AGP_INTEL=y
> > # CONFIG_AGP_SIS is not set
> > CONFIG_AGP_VIA=y
> > CONFIG_DRM=y
> > # CONFIG_DRM_TDFX is not set
> > 
> > ...do I need to enable something more?
> 
> Should have worked then. Ok modulo bugs. Maybe the ordering
> is wrong now (AGP resume would need to run before anything
> using the IOMMU)

So agp_amd64_resume() is responsible for reiniting iommu on new amd64
boxes?

It is registered as normal pci driver:

static struct pci_driver agp_amd64_pci_driver = {
	.name		= "agpgart-amd64",
	.id_table	= agp_amd64_pci_table,
	.probe		= agp_amd64_probe,
	.remove		= agp_amd64_remove,
#ifdef CONFIG_PM
	.suspend	= agp_amd64_suspend,
	.resume		= agp_amd64_resume,
#endif
};

...should it be modified to run early, as other pci devices (USB
controllers) may rely on this?

I did this... I'll verify it in 10 hours or so. If someone has amd64
system with >=4G ram, there should be hibernation problems. This
should fix it:

diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index d8200ac..4e85178 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -594,12 +594,14 @@ static int agp_amd64_suspend(struct pci_
 
 static int agp_amd64_resume(struct pci_dev *pdev)
 {
+	printk("agp_amd64: resume\n");
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
 
 	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA)
 		nforce3_agp_init(pdev);
 
+	printk("agp_amd64: 8151 configure\n");
 	return amd_8151_configure();
 }
 
@@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
 	.probe		= agp_amd64_probe,
 	.remove		= agp_amd64_remove,
 #ifdef CONFIG_PM
-	.suspend	= agp_amd64_suspend,
-	.resume		= agp_amd64_resume,
+	.suspend_late	= agp_amd64_suspend,
+	.resume_early	= agp_amd64_resume,
 #endif
 };
 


								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: iommu breaks usb after resume

[Ingo Molnar]

* Pavel Machek <pavel@ucw.cz> wrote:

> @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
>  	.probe		= agp_amd64_probe,
>  	.remove		= agp_amd64_remove,
>  #ifdef CONFIG_PM
> -	.suspend	= agp_amd64_suspend,
> -	.resume		= agp_amd64_resume,
> +	.suspend_late	= agp_amd64_suspend,
> +	.resume_early	= agp_amd64_resume,
>  #endif

ah, makes sense. I've queued up your fix in x86.git (sans the debug 
printks), please send your signoff once you can verify that it solves 
the hibernation hang on your box and we'll push it into 2.6.25.

	Ingo

Re: iommu breaks usb after resume

[Pavel Machek]

On Thu 2008-03-27 00:00:17, Ingo Molnar wrote:
> 
> * Pavel Machek <pavel@ucw.cz> wrote:
> 
> > @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
> >  	.probe		= agp_amd64_probe,
> >  	.remove		= agp_amd64_remove,
> >  #ifdef CONFIG_PM
> > -	.suspend	= agp_amd64_suspend,
> > -	.resume		= agp_amd64_resume,
> > +	.suspend_late	= agp_amd64_suspend,
> > +	.resume_early	= agp_amd64_resume,
> >  #endif
> 
> ah, makes sense. I've queued up your fix in x86.git (sans the debug 
> printks), please send your signoff once you can verify that it solves 
> the hibernation hang on your box and we'll push it into 2.6.25.

I've not even compiled it. It is obviously 

Signed-off-by: Pavel Machek <pavel@suse.cz>

(it is GPLed) ...but it is totally untested, and probably same fix
needs to go to all the agp drivers...?

I'll test it tommorow.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: iommu breaks usb after resume

[Andi Kleen]

> Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> (it is GPLed) ...but it is totally untested, and probably same fix
> needs to go to all the agp drivers...?

No the other AGP drivers don't have IOMMU functionality.

Also there still need to be Kconfig changes or alternatively 
extracting the suspend/resume code to another file that is selected
by both AGP and IOMMU.

-Andi

Re: iommu breaks usb after resume

[Pavel Machek]

Hi!

> > (it is GPLed) ...but it is totally untested, and probably same fix
> > needs to go to all the agp drivers...?
> 
> No the other AGP drivers don't have IOMMU functionality.

Ok, good.

> Also there still need to be Kconfig changes or alternatively 
> extracting the suspend/resume code to another file that is selected
> by both AGP and IOMMU.

Well, it gets weirder. Boot messages are:

Checking aperture...
Node 0: aperture @ 0 size 32 MB
No AGP bridge found
Your BIOS doesn't leave a aperture memory hole
Please enable the IOMMU option in the BIOS setup
This costs you 64 MB of RAM

...who is responsible for mapping aperture back during resume? It
seems like it is not amd64-agp: it does not find its hardware...?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: iommu breaks usb after resume

[Pavel Machek]

On Thu 2008-03-27 00:00:17, Ingo Molnar wrote:
> 
> * Pavel Machek <pavel@ucw.cz> wrote:
> 
> > @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
> >  	.probe		= agp_amd64_probe,
> >  	.remove		= agp_amd64_remove,
> >  #ifdef CONFIG_PM
> > -	.suspend	= agp_amd64_suspend,
> > -	.resume		= agp_amd64_resume,
> > +	.suspend_late	= agp_amd64_suspend,
> > +	.resume_early	= agp_amd64_resume,
> >  #endif
> 
> ah, makes sense. I've queued up your fix in x86.git (sans the debug 
> printks), please send your signoff once you can verify that it solves 
> the hibernation hang on your box and we'll push it into 2.6.25.

You have my signed-off from previous email, but...

it does not solve a problem here -- see the mail to andi. It may still
be good idea to take it (but I do not think it should go to 2.6.25),
but may machine still does not work.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

(eats disks) Re: iommu breaks usb after resume

[Pavel Machek]

On Thu 2008-03-27 09:42:31, Pavel Machek wrote:
> On Thu 2008-03-27 00:00:17, Ingo Molnar wrote:
> > 
> > * Pavel Machek <pavel@ucw.cz> wrote:
> > 
> > > @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
> > >  	.probe		= agp_amd64_probe,
> > >  	.remove		= agp_amd64_remove,
> > >  #ifdef CONFIG_PM
> > > -	.suspend	= agp_amd64_suspend,
> > > -	.resume		= agp_amd64_resume,
> > > +	.suspend_late	= agp_amd64_suspend,
> > > +	.resume_early	= agp_amd64_resume,
> > >  #endif
> > 
> > ah, makes sense. I've queued up your fix in x86.git (sans the debug 
> > printks), please send your signoff once you can verify that it solves 
> > the hibernation hang on your box and we'll push it into 2.6.25.
> 
> You have my signed-off from previous email, but...
> 
> it does not solve a problem here -- see the mail to andi. It may still
> be good idea to take it (but I do not think it should go to 2.6.25),
> but may machine still does not work.

Hmm, seems like this is pretty dangerous area. On resume, system is in
half-alive state: it responds to commands but IOMMU is just not
configure.

Unfortunately, it decided to write crap over my boot sector :-(.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: iommu breaks usb after resume

[Pavel Machek]

Hi!

> > * Pavel Machek <pavel@ucw.cz> wrote:
> > 
> > > @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
> > >  	.probe		= agp_amd64_probe,
> > >  	.remove		= agp_amd64_remove,
> > >  #ifdef CONFIG_PM
> > > -	.suspend	= agp_amd64_suspend,
> > > -	.resume		= agp_amd64_resume,
> > > +	.suspend_late	= agp_amd64_suspend,
> > > +	.resume_early	= agp_amd64_resume,
> > >  #endif
> > 
> > ah, makes sense. I've queued up your fix in x86.git (sans the debug 
> > printks), please send your signoff once you can verify that it solves 
> > the hibernation hang on your box and we'll push it into 2.6.25.
> 
> You have my signed-off from previous email, but...
> 
> it does not solve a problem here -- see the mail to andi. It may still
> be good idea to take it (but I do not think it should go to 2.6.25),
> but may machine still does not work.

No, I do not think problems are in drivers/char/agp...

end of aperture_64.c:

	/* Fix up the north bridges */
	for (num = 24; num < 32; num++) {
		if (!early_is_k8_nb(read_pci_config(0, num, 3, 0x00)))
			continue;

		/*
		 * Don't enable translation yet. That is done later.
		 * Assume this BIOS didn't initialise the GART so
		 * just overwrite all previous bits
		 */
		write_pci_config(0, num, 3, 0x90, aper_order<<1);
		write_pci_config(0, num, 3, 0x94, aper_alloc>>25);
	}
}

...this sounds like it needs to be done during resume, too, no?

Where's the other part? "Don't enable translation yet"? What enables
the translation?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: iommu breaks usb after resume

[Andi Kleen]

> ...this sounds like it needs to be done during resume, too, no?

This is only a workaround for BIOS that don't set up an aperture,
but yes if they don't do that it is likely needed on resume too.

You would need to add a resume handler to aperture.c

-Andi

Re: iommu breaks usb after resume

[Pavel Machek]

Hi!

> > ...this sounds like it needs to be done during resume, too, no?
> 
> This is only a workaround for BIOS that don't set up an aperture,
> but yes if they don't do that it is likely needed on resume too.
> 
> You would need to add a resume handler to aperture.c

Probably this needs to be redone during resume, right?

	/* Fix up the north bridges */
	for (num = 24; num < 32; num++) {
		if (!early_is_k8_nb(read_pci_config(0, num, 3, 0x00)))
			continue;

		/*
		 * Don't enable translation yet. That is done later.
		 * Assume this BIOS didn't initialise the GART so
		 * just overwrite all previous bits
		 */
		write_pci_config(0, num, 3, 0x90, aper_order<<1);
		write_pci_config(0, num, 3, 0x94, aper_alloc>>25);
	}

....plus the "enable translation" part... but where is that hiding?

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: iommu breaks usb after resume

[Andi Kleen]

On Thu, Mar 27, 2008 at 02:29:23PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > ...this sounds like it needs to be done during resume, too, no?
> > 
> > This is only a workaround for BIOS that don't set up an aperture,
> > but yes if they don't do that it is likely needed on resume too.
> > 
> > You would need to add a resume handler to aperture.c
> 
> Probably this needs to be redone during resume, right?

Yes, but using the standard pci access functions to avoid races which
the early boot code can ignore.

> 
> 	/* Fix up the north bridges */
> 	for (num = 24; num < 32; num++) {
> 		if (!early_is_k8_nb(read_pci_config(0, num, 3, 0x00)))
> 			continue;
> 
> 		/*
> 		 * Don't enable translation yet. That is done later.
> 		 * Assume this BIOS didn't initialise the GART so
> 		 * just overwrite all previous bits
> 		 */
> 		write_pci_config(0, num, 3, 0x90, aper_order<<1);
> 		write_pci_config(0, num, 3, 0x94, aper_alloc>>25);
> 	}
> 
> ....plus the "enable translation" part... but where is that hiding?

Both pci-gart and the AGP driver have code for this.

-Andi

Re: iommu breaks usb after resume

[Rafael J. Wysocki]

On Wednesday, 26 of March 2008, Pavel Machek wrote:
> Hi!
> 
> > > CONFIG_AGP=y
> > > CONFIG_AGP_AMD64=y
> > > CONFIG_AGP_INTEL=y
> > > # CONFIG_AGP_SIS is not set
> > > CONFIG_AGP_VIA=y
> > > CONFIG_DRM=y
> > > # CONFIG_DRM_TDFX is not set
> > > 
> > > ...do I need to enable something more?
> > 
> > Should have worked then. Ok modulo bugs. Maybe the ordering
> > is wrong now (AGP resume would need to run before anything
> > using the IOMMU)
> 
> So agp_amd64_resume() is responsible for reiniting iommu on new amd64
> boxes?
> 
> It is registered as normal pci driver:
> 
> static struct pci_driver agp_amd64_pci_driver = {
> 	.name		= "agpgart-amd64",
> 	.id_table	= agp_amd64_pci_table,
> 	.probe		= agp_amd64_probe,
> 	.remove		= agp_amd64_remove,
> #ifdef CONFIG_PM
> 	.suspend	= agp_amd64_suspend,
> 	.resume		= agp_amd64_resume,
> #endif
> };
> 
> ...should it be modified to run early, as other pci devices (USB
> controllers) may rely on this?
> 
> I did this... I'll verify it in 10 hours or so. If someone has amd64
> system with >=4G ram, there should be hibernation problems. This
> should fix it:
> 
> diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
> index d8200ac..4e85178 100644
> --- a/drivers/char/agp/amd64-agp.c
> +++ b/drivers/char/agp/amd64-agp.c
> @@ -594,12 +594,14 @@ static int agp_amd64_suspend(struct pci_
>  
>  static int agp_amd64_resume(struct pci_dev *pdev)
>  {
> +	printk("agp_amd64: resume\n");
>  	pci_set_power_state(pdev, PCI_D0);
>  	pci_restore_state(pdev);
>  
>  	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA)
>  		nforce3_agp_init(pdev);
>  
> +	printk("agp_amd64: 8151 configure\n");
>  	return amd_8151_configure();
>  }
>  
> @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
>  	.probe		= agp_amd64_probe,
>  	.remove		= agp_amd64_remove,
>  #ifdef CONFIG_PM
> -	.suspend	= agp_amd64_suspend,
> -	.resume		= agp_amd64_resume,
> +	.suspend_late	= agp_amd64_suspend,
> +	.resume_early	= agp_amd64_resume,
>  #endif
>  };

Okay, a couple of questions:

(1) Are you sure that the .suspend() and .resume() callbacks will just work
    with interrupts disabled?

(2) Even if they work, is it sufficient to just move them to the "late" and
    "early" parts?  That is, isn't there anything using the iommu in the
    "early" and "late" callbacks of the other devices?

Rafael

Re: iommu breaks usb after resume

[Pavel Machek]

On Thu 2008-03-27 21:30:37, Rafael J. Wysocki wrote:
> On Wednesday, 26 of March 2008, Pavel Machek wrote:
> > Hi!
> > 
> > > > CONFIG_AGP=y
> > > > CONFIG_AGP_AMD64=y
> > > > CONFIG_AGP_INTEL=y
> > > > # CONFIG_AGP_SIS is not set
> > > > CONFIG_AGP_VIA=y
> > > > CONFIG_DRM=y
> > > > # CONFIG_DRM_TDFX is not set
> > > > 
> > > > ...do I need to enable something more?
> > > 
> > > Should have worked then. Ok modulo bugs. Maybe the ordering
> > > is wrong now (AGP resume would need to run before anything
> > > using the IOMMU)
> > 
> > So agp_amd64_resume() is responsible for reiniting iommu on new amd64
> > boxes?
> > 
> > It is registered as normal pci driver:
> > 
> > static struct pci_driver agp_amd64_pci_driver = {
> > 	.name		= "agpgart-amd64",
> > 	.id_table	= agp_amd64_pci_table,
> > 	.probe		= agp_amd64_probe,
> > 	.remove		= agp_amd64_remove,
> > #ifdef CONFIG_PM
> > 	.suspend	= agp_amd64_suspend,
> > 	.resume		= agp_amd64_resume,
> > #endif
> > };
> > 
> > ...should it be modified to run early, as other pci devices (USB
> > controllers) may rely on this?
> > 
> > I did this... I'll verify it in 10 hours or so. If someone has amd64
> > system with >=4G ram, there should be hibernation problems. This
> > should fix it:
> > 
> > diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
> > index d8200ac..4e85178 100644
> > --- a/drivers/char/agp/amd64-agp.c
> > +++ b/drivers/char/agp/amd64-agp.c
> > @@ -594,12 +594,14 @@ static int agp_amd64_suspend(struct pci_
> >  
> >  static int agp_amd64_resume(struct pci_dev *pdev)
> >  {
> > +	printk("agp_amd64: resume\n");
> >  	pci_set_power_state(pdev, PCI_D0);
> >  	pci_restore_state(pdev);
> >  
> >  	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA)
> >  		nforce3_agp_init(pdev);
> >  
> > +	printk("agp_amd64: 8151 configure\n");
> >  	return amd_8151_configure();
> >  }
> >  
> > @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
> >  	.probe		= agp_amd64_probe,
> >  	.remove		= agp_amd64_remove,
> >  #ifdef CONFIG_PM
> > -	.suspend	= agp_amd64_suspend,
> > -	.resume		= agp_amd64_resume,
> > +	.suspend_late	= agp_amd64_suspend,
> > +	.resume_early	= agp_amd64_resume,
> >  #endif
> >  };
> 
> Okay, a couple of questions:
> 
> (1) Are you sure that the .suspend() and .resume() callbacks will just work
>     with interrupts disabled?

I have not really checked -- it turned out this is not "my" problem
after all. My machine (called leet) uses different setup. If they will
not, I guess lockdep will tell us ;-).

> (2) Even if they work, is it sufficient to just move them to the "late" and
>     "early" parts?  That is, isn't there anything using the iommu in the
>     "early" and "late" callbacks of the other devices?

We definitely have iommu users in "normal" callbacks: USB. I do not
think we have iommu users in early/late callbacks...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: iommu breaks usb after resume

[Rafael J. Wysocki]

On Friday, 28 of March 2008, Pavel Machek wrote:
> On Thu 2008-03-27 21:30:37, Rafael J. Wysocki wrote:
> > On Wednesday, 26 of March 2008, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > CONFIG_AGP=y
> > > > > CONFIG_AGP_AMD64=y
> > > > > CONFIG_AGP_INTEL=y
> > > > > # CONFIG_AGP_SIS is not set
> > > > > CONFIG_AGP_VIA=y
> > > > > CONFIG_DRM=y
> > > > > # CONFIG_DRM_TDFX is not set
> > > > > 
> > > > > ...do I need to enable something more?
> > > > 
> > > > Should have worked then. Ok modulo bugs. Maybe the ordering
> > > > is wrong now (AGP resume would need to run before anything
> > > > using the IOMMU)
> > > 
> > > So agp_amd64_resume() is responsible for reiniting iommu on new amd64
> > > boxes?
> > > 
> > > It is registered as normal pci driver:
> > > 
> > > static struct pci_driver agp_amd64_pci_driver = {
> > > 	.name		= "agpgart-amd64",
> > > 	.id_table	= agp_amd64_pci_table,
> > > 	.probe		= agp_amd64_probe,
> > > 	.remove		= agp_amd64_remove,
> > > #ifdef CONFIG_PM
> > > 	.suspend	= agp_amd64_suspend,
> > > 	.resume		= agp_amd64_resume,
> > > #endif
> > > };
> > > 
> > > ...should it be modified to run early, as other pci devices (USB
> > > controllers) may rely on this?
> > > 
> > > I did this... I'll verify it in 10 hours or so. If someone has amd64
> > > system with >=4G ram, there should be hibernation problems. This
> > > should fix it:
> > > 
> > > diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
> > > index d8200ac..4e85178 100644
> > > --- a/drivers/char/agp/amd64-agp.c
> > > +++ b/drivers/char/agp/amd64-agp.c
> > > @@ -594,12 +594,14 @@ static int agp_amd64_suspend(struct pci_
> > >  
> > >  static int agp_amd64_resume(struct pci_dev *pdev)
> > >  {
> > > +	printk("agp_amd64: resume\n");
> > >  	pci_set_power_state(pdev, PCI_D0);
> > >  	pci_restore_state(pdev);
> > >  
> > >  	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA)
> > >  		nforce3_agp_init(pdev);
> > >  
> > > +	printk("agp_amd64: 8151 configure\n");
> > >  	return amd_8151_configure();
> > >  }
> > >  
> > > @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
> > >  	.probe		= agp_amd64_probe,
> > >  	.remove		= agp_amd64_remove,
> > >  #ifdef CONFIG_PM
> > > -	.suspend	= agp_amd64_suspend,
> > > -	.resume		= agp_amd64_resume,
> > > +	.suspend_late	= agp_amd64_suspend,
> > > +	.resume_early	= agp_amd64_resume,
> > >  #endif
> > >  };
> > 
> > Okay, a couple of questions:
> > 
> > (1) Are you sure that the .suspend() and .resume() callbacks will just work
> >     with interrupts disabled?
> 
> I have not really checked -- it turned out this is not "my" problem
> after all. My machine (called leet) uses different setup. If they will
> not, I guess lockdep will tell us ;-).

Lockdep need not tell us about it, I think.  OTOH, I don't really see a
scenario in which having interrupts disabled while executing ->suspend() and
->resume() might hurt.

> > (2) Even if they work, is it sufficient to just move them to the "late" and
> >     "early" parts?  That is, isn't there anything using the iommu in the
> >     "early" and "late" callbacks of the other devices?
> 
> We definitely have iommu users in "normal" callbacks: USB.

Oh, there may be more.  You never know what appears in future systems. :-)

> I do not think we have iommu users in early/late callbacks...

Perhaps not at the moment, but what's the guarantee there won't be any in the
future?

Rafael

Re: iommu breaks usb after resume

[Pavel Machek]

Hi!

> > > > diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
> > > > index d8200ac..4e85178 100644
> > > > --- a/drivers/char/agp/amd64-agp.c
> > > > +++ b/drivers/char/agp/amd64-agp.c
> > > > @@ -594,12 +594,14 @@ static int agp_amd64_suspend(struct pci_
> > > >  
> > > >  static int agp_amd64_resume(struct pci_dev *pdev)
> > > >  {
> > > > +	printk("agp_amd64: resume\n");
> > > >  	pci_set_power_state(pdev, PCI_D0);
> > > >  	pci_restore_state(pdev);
> > > >  
> > > >  	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA)
> > > >  		nforce3_agp_init(pdev);
> > > >  
> > > > +	printk("agp_amd64: 8151 configure\n");
> > > >  	return amd_8151_configure();
> > > >  }
> > > >  
> > > > @@ -733,8 +735,8 @@ static struct pci_driver agp_amd64_pci_d
> > > >  	.probe		= agp_amd64_probe,
> > > >  	.remove		= agp_amd64_remove,
> > > >  #ifdef CONFIG_PM
> > > > -	.suspend	= agp_amd64_suspend,
> > > > -	.resume		= agp_amd64_resume,
> > > > +	.suspend_late	= agp_amd64_suspend,
> > > > +	.resume_early	= agp_amd64_resume,
> > > >  #endif
> > > >  };
> > > 
> > > Okay, a couple of questions:
> > > 
> > > (1) Are you sure that the .suspend() and .resume() callbacks will just work
> > >     with interrupts disabled?
> > 
> > I have not really checked -- it turned out this is not "my" problem
> > after all. My machine (called leet) uses different setup. If they will
> > not, I guess lockdep will tell us ;-).
> 
> Lockdep need not tell us about it, I think.  OTOH, I don't really see a
> scenario in which having interrupts disabled while executing ->suspend() and
> ->resume() might hurt.

Ok.

> > > (2) Even if they work, is it sufficient to just move them to the "late" and
> > >     "early" parts?  That is, isn't there anything using the iommu in the
> > >     "early" and "late" callbacks of the other devices?
> > 
> > We definitely have iommu users in "normal" callbacks: USB.
> 
> Oh, there may be more.  You never know what appears in future systems. :-)

Well, we'll catch them during code review, right?

> > I do not think we have iommu users in early/late callbacks...
> 
> Perhaps not at the moment, but what's the guarantee there won't be any in the
> future?

That's why we review changes... ok, maybe documenting it somewhere
would be nice...?

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html