amd74xx crashes when resuming from STR

PATCH In review Jason Lunz 5 participants 17 messages Jul 15, 2006 – Jul 28, 2006 1 message not in the archive libata

Share & tools

Jason Lunz

author

2006-07-15 21:02 UTC #0 lore raw

On my laptop, suspend-to-ram works for all drivers with the exception of the amd74xx ide driver. And even then, it only has problems when accessing a UDMA hard drive. I know this because the system can use STR reliably when booted from a livecd, so long as nothing accesses the hard disk.

I'm running amd64 2.6.17 untainted. The motherboard and ide chipset are nvidia:

# lspci
00:00.0 Host bridge: nVidia Corporation nForce3 Host Bridge (rev a4)
00:01.0 ISA bridge: nVidia Corporation nForce3 LPC Bridge (rev a6)
00:01.1 SMBus: nVidia Corporation nForce3 SMBus (rev a4)
00:02.0 USB Controller: nVidia Corporation nForce3 USB 1.1 (rev a5)
00:02.1 USB Controller: nVidia Corporation nForce3 USB 1.1 (rev a5)
00:02.2 USB Controller: nVidia Corporation nForce3 USB 2.0 (rev a2)
00:06.0 Multimedia audio controller: nVidia Corporation nForce3 Audio (rev a2)
00:06.1 Modem: nVidia Corporation nForce3 Audio (rev a2)
00:08.0 IDE interface: nVidia Corporation nForce3 IDE (rev a5)
00:0a.0 PCI bridge: nVidia Corporation nForce3 PCI Bridge (rev a2)
00:0b.0 PCI bridge: nVidia Corporation nForce3 AGP Bridge (rev a4)
00:18.0 Host bridge: Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] HyperTransport Technology Configuration
00:18.1 Host bridge: Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Address Map
00:18.2 Host bridge: Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] DRAM Controller
00:18.3 Host bridge: Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Miscellaneous Control
01:00.0 FireWire (IEEE 1394): Texas Instruments TSB43AB21 IEEE-1394a-2000 Controller (PHY/Link)
01:01.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8139/8139C/8139C+ (rev 10)
01:02.0 Network controller: Broadcom Corporation BCM4306 802.11b/g Wireless LAN Controller (rev 03)
01:04.0 CardBus bridge: Texas Instruments PCI1620 PC Card Controller (rev 01)
01:04.1 CardBus bridge: Texas Instruments PCI1620 PC Card Controller (rev 01)
01:04.2 System peripheral: Texas Instruments PCI1620 Firmware Loading Function (rev 01)
0a:00.0 VGA compatible controller: nVidia Corporation NV17 [GeForce4 420 Go 32M] (rev a3)

I know that the amd74xx driver is definitely the problem, because STR works reliably when using the ide-generic driver. But in that case there's no DMA and the drive is painfully slow. And the crash is not DMA-related, because the cdrom also uses DMA, yet it does trouble-free suspend/resume under amd74xx.

Here's info from driver load, and /proc:

Uniform Multi-Platform E-IDE driver Revision: 7.00alpha2
ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
NFORCE3-150: IDE controller at PCI slot 0000:00:08.0
NFORCE3-150: chipset revision 165
NFORCE3-150: not 100% native mode: will probe irqs later
NFORCE3-150: BIOS didn't set cable bits correctly. Enabling workaround.
NFORCE3-150: 0000:00:08.0 (rev a5) UDMA133 controller

    ide0: BM-DMA at 0x2080-0x2087, BIOS settings: hda:DMA, hdb:pio
    ide1: BM-DMA at 0x2088-0x208f, BIOS settings: hdc:DMA, hdd:pio

Probing IDE interface ide0...
hda: HITACHI_DK23DA-20, ATA DISK drive
isa bounce pool size: 16 pages
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
Probing IDE interface ide1...
hdc: HL-DT-STCD-RW/DVD DRIVE GCC-4241N, ATAPI CD/DVD-ROM drive ide1 at 0x170-0x177,0x376 on irq 15
hda: max request size: 128KiB
hda: 39070080 sectors (20003 MB) w/2048KiB Cache, CHS=38760/16/63, UDMA(100)
hda: cache flushes supported
hda: hda1 hda2 hda3

# cat /proc/ide/amd74xx
----------AMD BusMastering IDE Configuration----------------
Driver Version:                     2.13
South Bridge:                       0000:00:08.0
Revision:                           IDE 0xa5
Highest DMA rate:                   UDMA133
BM-DMA base:                        0x2080
PCI clock:                          33.3MHz
-----------------------Primary IDE-------Secondary IDE------
Prefetch Buffer:              yes                 yes
Post Write Buffer:            yes                 yes
Enabled:                      yes                 yes
Simplex only:                  no                  no
Cable Type:                   80w                 40w
-------------------drive0----drive1----drive2----drive3-----
Transfer Mode:       UDMA       PIO       DMA       PIO
Address Setup:       30ns      90ns      30ns      90ns
Cmd Active:          90ns      90ns      90ns      90ns
Cmd Recovery:        30ns      30ns      30ns      30ns
Data Active:         90ns     330ns      90ns     330ns
Data Recovery:       30ns     270ns      30ns     270ns
Cycle Time:          20ns     600ns     120ns     600ns
Transfer Rate:   99.9MB/s   3.3MB/s  16.6MB/s   3.3MB/s

Here's the crash that occurs post-resume, as captured by netconsole. I compiled drivers/ide/ide-io.c with DEBUG_PM #defined:

netconsole: network logging started
Stopping tasks: ============================================|
hdc: start_power_step(step: 0)
hdc: completing PM request, suspend
hda: start_power_step(step: 0)
hda: complete_power_step(step: 0, stat: 50, err: 0)
hda: start_power_step(step: 1)
hda: complete_power_step(step: 1, stat: 50, err: 0)
hda: completing PM request, suspend
pnp: Device 00:0b disabled.
ACPI: PCI interrupt for device 0000:01:04.1 disabled
ACPI: PCI interrupt for device 0000:01:04.0 disabled
ACPI: PCI interrupt for device 0000:01:02.0 disabled

PCI: Enabling device 0000:01:02.0 (0000 -> 0002)
ACPI: PCI Interrupt 0000:01:02.0[A] -> Link [LNK3] -> GSI 17 (level, low) -> IRQ 21
PM: Writing back config space on device 0000:01:02.0 at offset f (was 100, writing 10b)
PM: Writing back config space on device 0000:01:02.0 at offset 4 (was 0, writing e0104000)
PM: Writing back config space on device 0000:01:02.0 at offset 3 (was 0, writing 4000)
PM: Writing back config space on device 0000:01:02.0 at offset 1 (was 2, writing 106)
PM: Writing back config space on device 0000:01:04.0 at offset f (was 34001ff, writing 5c0010b)
PM: Writing back config space on device 0000:01:04.0 at offset e (was 0, writing 34fc)
PM: Writing back config space on device 0000:01:04.0 at offset d (was 0, writing 3400)
PM: Writing back config space on device 0000:01:04.0 at offset c (was 0, writing 30fc)
PM: Writing back config space on device 0000:01:04.0 at offset b (was 0, writing 3000)
PM: Writing back config space on device 0000:01:04.0 at offset a (was 0, writing e07ff000)
PM: Writing back config space on device 0000:01:04.0 at offset 8 (was 0, writing 31fff000)
PM: Writing back config space on device 0000:01:04.0 at offset 6 (was 40000000, writing b0050201)
PM: Writing back config space on device 0000:01:04.0 at offset 3 (was 824008, writing 82a810)
PM: Writing back config space on device 0000:01:04.0 at offset 1 (was 2100107, writing 2100007)
ACPI: PCI Interrupt 0000:01:04.0[A] -> Link [LNK1] -> GSI 19 (level, low) -> IRQ 16
PM: Writing back config space on device 0000:01:04.1 at offset f (was 34002ff, writing 5c0020a)
PM: Writing back config space on device 0000:01:04.1 at offset e (was 0, writing 3cfc)
PM: Writing back config space on device 0000:01:04.1 at offset d (was 0, writing 3c00)
PM: Writing back config space on device 0000:01:04.1 at offset c (was 0, writing 38fc)
PM: Writing back config space on device 0000:01:04.1 at offset b (was 0, writing 3800)
PM: Writing back config space on device 0000:01:04.1 at offset a (was 0, writing e0fff000)
PM: Writing back config space on device 0000:01:04.1 at offset 8 (was 0, writing 33fff000)
PM: Writing back config space on device 0000:01:04.1 at offset 7 (was e1000000, writing 32000000)
PM: Writing back config space on device 0000:01:04.1 at offset 6 (was 40000000, writing b0090601)
PM: Writing back config space on device 0000:01:04.1 at offset 3 (was 824008, writing 82a810)
PM: Writing back config space on device 0000:01:04.1 at offset 1 (was 2100103, writing 2100007)
ACPI: PCI Interrupt 0000:01:04.1[B] -> Link [LNK2] -> GSI 18 (level, low) -> IRQ 17
PM: Writing back config space on device 0000:01:04.2 at offset 4 (was 1, writing 7401)
PM: Writing back config space on device 0000:01:04.2 at offset 3 (was 0, writing 4010)
PM: Writing back config space on device 0000:01:04.2 at offset 1 (was 2100000, writing 2100107)
PM: Writing back config space on device 0000:0a:00.0 at offset f (was 1050100, writing 105010b)
PM: Writing back config space on device 0000:0a:00.0 at offset 6 (was 8, writing f8000008)
PM: Writing back config space on device 0000:0a:00.0 at offset 5 (was 8, writing f0000008)
PM: Writing back config space on device 0000:0a:00.0 at offset 4 (was 0, writing e2000000)
PM: Writing back config space on device 0000:0a:00.0 at offset 3 (was 0, writing 4000)
PM: Writing back config space on device 0000:0a:00.0 at offset 1 (was 2b00000, writing 2b00007)
pnp: Res cnt 3
pnp: res cnt 3
pnp: Encode io
pnp: Encode io
pnp: Encode irq
pnp: Failed to activate device 00:08.
pnp: Res cnt 1
pnp: res cnt 1
pnp: Encode irq
pnp: Failed to activate device 00:09.
pnp: Res cnt 4
pnp: res cnt 4
pnp: Encode io
pnp: Encode io
pnp: Encode irq
pnp: Encode dma
pnp: Device 00:0b activated.
hda: Wakeup request inited, waiting for !BSY...
hda: start_power_step(step: 1000)
hda: complete_power_step(step: 1000, stat: 50, err: 0)
hda: start_power_step(step: 1001)
hda: completing PM request, resume
hdc: Wakeup request inited, waiting for !BSY...
hdc: start_power_step(step: 1000)
hdc: completing PM request, resume
Restarting tasks...
done
hda: dma_timer_expiry: dma status == 0x21
hda: DMA timeout error

HARDWARE ERROR
CPU 0: Machine Check Exception: 4 Bank 4: b200000000070f0f
TSC 1370e9bdb9
This is not a software problem!
Run through mcelog --ascii to decode and contact your hardware vendor

Kernel panic - not syncing: Machine check

Does this driver need special handling? I notice one other driver in drivers/ide/pci, sc1200, implements its own pci_driver->suspend() and pci_driver->resume() hooks. Maybe similar methods are needed in this case?

thanks,

Jason

Pavel Machek

2006-07-18 04:34 UTC #1 lore raw

Hi!

Quoted 5 lines

On my laptop, suspend-to-ram works for all drivers with the exception of the amd74xx ide driver. And even then, it only has problems when accessing a UDMA hard drive. I know this because the system can use STR reliably when booted from a livecd, so long as nothing accesses the hard disk.

I guess you need to ask on linux-ide mainling lists?

Ahha, you are cross-posting there, already.

What happens if you rmmod amd74xx and insmod it after resume?
Pavel

Signature · 1 line

Thanks for all the (sleeping) penguins.

Pavel Machek

2006-07-18 04:35 UTC #2 lore raw

In reply to Jason Lunz

Hi!

Quoted 5 lines

On my laptop, suspend-to-ram works for all drivers with the exception of the amd74xx ide driver. And even then, it only has problems when accessing a UDMA hard drive. I know this because the system can use STR reliably when booted from a livecd, so long as nothing accesses the hard disk.

I guess you need to ask on linux-ide mainling lists?

Ahha, you are cross-posting there, already.

What happens if you rmmod amd74xx and insmod it after resume?
Pavel

Signature · 1 line

Thanks for all the (sleeping) penguins.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Vojtech Pavlik

2006-07-18 13:39 UTC #4 lore raw

Quoted 8 lines · Pavel Machek

Hi!
Quoted 5 lines
On my laptop, suspend-to-ram works for all drivers with the exception of the amd74xx ide driver. And even then, it only has problems when accessing a UDMA hard drive. I know this because the system can use STR reliably when booted from a livecd, so long as nothing accesses the hard disk.

Quoted 5 lines

I guess you need to ask on linux-ide mainling lists?
Ahha, you are cross-posting there, already.
What happens if you rmmod amd74xx and insmod it after resume?

amd74xx relies on the BIOS to have set up a significant part of the controller correctly. This likely isn't true after S3 resume. It might be necessary to save & restore the whole PCI config space of the device.

Signature · 2 lines

Vojtech Pavlik
Director SuSE Labs

Jason Lunz

author

2006-07-24 00:53 UTC #5 lore raw

PATCH 1/1 amd74xx: implement suspend-to-ram

The amd74xx driver needs to reprogram each drive's PIO timings as well as the DMA timings on resume from s2ram. Otherwise, my nforce3-150-based laptop hangs hard when ide_start_power_step() calls drive->hwif->ide_dma_check(drive).

Suspend/resume from ram now works with the disk and the cdrom under load, both with and without DMA enabled.

Signed-off-by Jason Lunz <[email protected]>

---

I'm hardcoding a maximum of 2 ide channels, but other aspects of this driver (like the amd_80w global) are already doing that.

DMA is re-enabled on resume even if it wasn't on at suspend, but that doesn't look unusual in drivers/ide/pci.

 drivers/ide/pci/amd74xx.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

drivers/ide/pci/amd74xx.c +53 -0

Index: linux-2.6.18-rc2-git1/drivers/ide/pci/amd74xx.c
===================================================================
@@ -83,6 +83,8 @@
 static ide_pci_device_t *amd_chipset;
 static unsigned int amd_80w;
 static unsigned int amd_clock;
+#define AMD_MAX_CHANNELS	(2)
+static ide_hwif_t *amd_hwifs[AMD_MAX_CHANNELS];
 
 static char *amd_dma[] = { "MWDMA16", "UDMA33", "UDMA66", "UDMA100", "UDMA133" };
 static unsigned char amd_cyc2udma[] = { 6, 6, 5, 4, 0, 1, 1, 2, 2, 3, 3, 3, 3, 3, 3, 7 };

@@ -416,6 +418,8 @@
 {
 	int i;
 
+	amd_hwifs[hwif->channel] = hwif;
+
 	if (hwif->irq == 0) /* 0 is bogus but will do for now */
 		hwif->irq = pci_get_legacy_ide_irq(hwif->pci_dev, hwif->channel);

@@ -494,6 +498,53 @@
 	/* 19 */ DECLARE_AMD_DEV("AMD5536"),
 };
 
+static int amd74xx_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	pci_save_state(dev);
+
+	// PM_EVENT_SUSPEND means s2ram. otherwise, the disk behind this device
+	// might hold the s2disk image, and we can't disable the disk
+	// controller until we've used it to write that out.
+	if(state.event == PM_EVENT_SUSPEND) {
+		pci_disable_device(dev);
+		pci_set_power_state(dev, pci_choose_state(dev, state));
+	}
+	return 0;
+}
+
+static int amd74xx_resume(struct pci_dev *dev)
+{
+	int retval = 0;
+	int i;
+
+	pci_set_power_state(dev, PCI_D0);
+	retval = pci_enable_device(dev);
+	pci_restore_state(dev);
+
+	for (i = 0; i < AMD_MAX_CHANNELS; i++) {
+		int d;
+
+		if (!amd_hwifs[i])
+			continue;
+
+		for (d = 0; d < MAX_DRIVES; ++d) {
+			ide_drive_t *drive = &amd_hwifs[i]->drives[d];
+			if (drive->present && !__ide_dma_bad_drive(drive)) {
+				/* this is the primary reason this driver needs
+				 * a suspend()/resume() implementation at all.
+				 * Calling amd74xx_ide_dma_check() without also
+				 * calling amd74xx_tune_drive() hangs my
+				 * nforce3-150 system.  ide-disk.c will do just
+				 * that later if we're resuming from s2ram. */
+				amd_hwifs[i]->tuneproc(drive, 255);
+				amd_hwifs[i]->ide_dma_check(drive);
+			}
+		}
+	}
+
+	return retval;
+}
+
 static int __devinit amd74xx_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	amd_chipset = amd74xx_chipsets + id->driver_data;

@@ -539,6 +590,8 @@
 	.name		= "AMD_IDE",
 	.id_table	= amd74xx_pci_tbl,
 	.probe		= amd74xx_probe,
+	.suspend	= amd74xx_suspend,
+	.resume		= amd74xx_resume,
 };
 
 static int amd74xx_ide_init(void)

Pavel Machek

2006-07-25 23:09 UTC #6 lore raw

Re: [patch, rft] amd74xx: implement suspend-to-ram

Hi!

I do not have the affected hardware, sorry.

Quoted 5 lines

Index: linux-2.6.18-rc2-git1/drivers/ide/pci/amd74xx.c
===================================================================

drivers/ide/pci/amd74xx.c line 507

Quoted 10 lines

drivers/ide/pci/amd74xx.c +7 -0

@@ -494,6 +498,53 @@
 	/* 19 */ DECLARE_AMD_DEV("AMD5536"),
 };
 
+static int amd74xx_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	pci_save_state(dev);
+
+	// PM_EVENT_SUSPEND means s2ram. otherwise, the disk behind this device
+	// might hold the s2disk image, and we can't disable the disk
+	// controller until we've used it to write that out.

C-style comments, please.

drivers/ide/pci/amd74xx.c line 508

Quoted 1 line

drivers/ide/pci/amd74xx.c +1 -0
+	if(state.event == PM_EVENT_SUSPEND) {

"if (". Ouch and please don't do this. Suspend-to-disk will call amd74xx_resume when it needs to talk to the disk.

drivers/ide/pci/amd74xx.c line 513

Quoted 5 lines

drivers/ide/pci/amd74xx.c +5 -0

+		pci_disable_device(dev);
+		pci_set_power_state(dev, pci_choose_state(dev, state));
+	}
+	return 0;
+}

Pavel

Signature · 2 lines

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

David Brownell

2006-07-26 00:27 UTC #7 lore raw

Re: [linux-pm] [patch, rft] amd74xx: implement suspend-to-ram

Quoted 4 lines · Pavel Machek

drivers/ide/pci/amd74xx.c line 508
Quoted 2 lines
drivers/ide/pci/amd74xx.c +1 -0
+	if(state.event == PM_EVENT_SUSPEND) {
"if (". Ouch

He already got that feedback, along with the fact that PM_EVENT_SUSPEND gets called in _all_ suspend sequences not just "standby" and STR. That comment just needs to be removed. :)

Quoted 2 lines

and please don't do this. Suspend-to-disk will call amd74xx_resume when it needs to talk to the disk.

I don't understand the rest of this comment. That code looks correct (other than calling the goofy/broken pci_choose_state thing); PM_EVENT_FREEZE and PM_EVENT_PRETHAW should not disable the device.

So what "this" are you suggesting he not do ... and why?

Quoted 7 lines

Quoted 5 lines

drivers/ide/pci/amd74xx.c +5 -0

+		pci_disable_device(dev);
+		pci_set_power_state(dev, pci_choose_state(dev, state));
+	}
+	return 0;
+}

Jason Lunz

author

2006-07-26 02:45 UTC #8 lore raw

PATCH v2 1/1 amd74xx: fix hang on resume from ram

Suspend/resume from ram now works with the disk and the cdrom under load, both with and without DMA enabled.

Signed-off-by Jason Lunz <[email protected]>

---

I've incorporated improvements helpfully suggested by David Brownell and Pavel Machek.

I'm hardcoding a maximum of 2 ide channels, but other aspects of this driver (like the amd_80w global) are already doing that.

DMA is re-enabled on resume even if it wasn't on at suspend, but that doesn't look unusual in drivers/ide/pci.

 drivers/ide/pci/amd74xx.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

drivers/ide/pci/amd74xx.c +58 -0

Index: linux-2.6.18-rc2-git1/drivers/ide/pci/amd74xx.c
===================================================================
@@ -83,6 +83,8 @@
 static ide_pci_device_t *amd_chipset;
 static unsigned int amd_80w;
 static unsigned int amd_clock;
+#define AMD_MAX_CHANNELS	(2)
+static ide_hwif_t *amd_hwifs[AMD_MAX_CHANNELS];
 
 static char *amd_dma[] = { "MWDMA16", "UDMA33", "UDMA66", "UDMA100", "UDMA133" };
 static unsigned char amd_cyc2udma[] = { 6, 6, 5, 4, 0, 1, 1, 2, 2, 3, 3, 3, 3, 3, 3, 7 };

@@ -416,6 +418,8 @@
 {
 	int i;
 
+	amd_hwifs[hwif->channel] = hwif;
+
 	if (hwif->irq == 0) /* 0 is bogus but will do for now */
 		hwif->irq = pci_get_legacy_ide_irq(hwif->pci_dev, hwif->channel);

@@ -494,6 +498,58 @@
 	/* 19 */ DECLARE_AMD_DEV("AMD5536"),
 };
 
+#ifdef	CONFIG_PM
+
+static int amd74xx_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	pci_save_state(dev);
+
+	if (state.event == PM_EVENT_SUSPEND) {
+		pci_disable_device(dev);
+		pci_set_power_state(dev, PCI_D3hot);
+	}
+	return 0;
+}
+
+static int amd74xx_resume(struct pci_dev *dev)
+{
+	int retval = 0;
+	int i;
+
+	pci_set_power_state(dev, PCI_D0);
+	retval = pci_enable_device(dev);
+	pci_restore_state(dev);
+
+	for (i = 0; i < AMD_MAX_CHANNELS; i++) {
+		int d;
+
+		if (!amd_hwifs[i])
+			continue;
+
+		for (d = 0; d < MAX_DRIVES; ++d) {
+			ide_drive_t *drive = &amd_hwifs[i]->drives[d];
+			if (drive->present && !__ide_dma_bad_drive(drive)) {
+				/* this is the primary reason this driver needs
+				 * a suspend()/resume() implementation at all.
+				 * Calling amd74xx_ide_dma_check() without also
+				 * calling amd74xx_tune_drive() hangs my
+				 * nforce3-150 system.  ide-io.c will do just
+				 * that later if we're resuming from s2ram.
+				 */
+				amd_hwifs[i]->tuneproc(drive, 255);
+				amd_hwifs[i]->ide_dma_check(drive);
+			}
+		}
+	}
+
+	return retval;
+}
+
+#else	/* !CONFIG_PM */
+#define amd74xx_suspend
+#define amd74xx_resume
+#endif	/* !CONFIG_PM */
+
 static int __devinit amd74xx_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	amd_chipset = amd74xx_chipsets + id->driver_data;

@@ -539,6 +595,8 @@
 	.name		= "AMD_IDE",
 	.id_table	= amd74xx_pci_tbl,
 	.probe		= amd74xx_probe,
+	.suspend	= amd74xx_suspend,
+	.resume		= amd74xx_resume,
 };
 
 static int amd74xx_ide_init(void)

Jason Lunz

author

2006-07-26 03:14 UTC #9 lore raw

PATCH v3 1/1 amd74xx: fix hang on resume from ram

In reply to David Brownell

Suspend/resume from ram now works with the disk and the cdrom under load, both with and without DMA enabled.

Signed-off-by Jason Lunz <[email protected]>

---

I'm hardcoding a maximum of 2 ide channels, but other aspects of this driver (like the amd_80w global) are already doing that.

DMA is re-enabled on resume even if it wasn't on at suspend, but that doesn't look unusual in drivers/ide/pci.

I've incorporated improvements helpfully suggested by David Brownell and Pavel Machek.

Now compiles with !CONFIG_PM.

 drivers/ide/pci/amd74xx.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

drivers/ide/pci/amd74xx.c +57 -0

Index: linux-2.6.18-rc2-git1/drivers/ide/pci/amd74xx.c
===================================================================
@@ -83,6 +83,8 @@
 static ide_pci_device_t *amd_chipset;
 static unsigned int amd_80w;
 static unsigned int amd_clock;
+#define AMD_MAX_CHANNELS	(2)
+static ide_hwif_t *amd_hwifs[AMD_MAX_CHANNELS];
 
 static char *amd_dma[] = { "MWDMA16", "UDMA33", "UDMA66", "UDMA100", "UDMA133" };
 static unsigned char amd_cyc2udma[] = { 6, 6, 5, 4, 0, 1, 1, 2, 2, 3, 3, 3, 3, 3, 3, 7 };

@@ -416,6 +418,8 @@
 {
 	int i;
 
+	amd_hwifs[hwif->channel] = hwif;
+
 	if (hwif->irq == 0) /* 0 is bogus but will do for now */
 		hwif->irq = pci_get_legacy_ide_irq(hwif->pci_dev, hwif->channel);

@@ -494,6 +498,55 @@
 	/* 19 */ DECLARE_AMD_DEV("AMD5536"),
 };
 
+#ifdef	CONFIG_PM
+
+static int amd74xx_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	pci_save_state(dev);
+
+	if (state.event == PM_EVENT_SUSPEND) {
+		pci_disable_device(dev);
+		pci_set_power_state(dev, PCI_D3hot);
+	}
+	return 0;
+}
+
+static int amd74xx_resume(struct pci_dev *dev)
+{
+	int retval = 0;
+	int i;
+
+	pci_set_power_state(dev, PCI_D0);
+	retval = pci_enable_device(dev);
+	pci_restore_state(dev);
+
+	for (i = 0; i < AMD_MAX_CHANNELS; i++) {
+		int d;
+
+		if (!amd_hwifs[i])
+			continue;
+
+		for (d = 0; d < MAX_DRIVES; ++d) {
+			ide_drive_t *drive = &amd_hwifs[i]->drives[d];
+			if (drive->present && !__ide_dma_bad_drive(drive)) {
+				/* this is the primary reason this driver needs
+				 * a suspend()/resume() implementation at all.
+				 * Calling amd74xx_ide_dma_check() without also
+				 * calling amd74xx_tune_drive() hangs my
+				 * nforce3-150 system.  ide-io.c will do just
+				 * that later if we're resuming from s2ram.
+				 */
+				amd_hwifs[i]->tuneproc(drive, 255);
+				amd_hwifs[i]->ide_dma_check(drive);
+			}
+		}
+	}
+
+	return retval;
+}
+
+#endif	/* !CONFIG_PM */
+
 static int __devinit amd74xx_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	amd_chipset = amd74xx_chipsets + id->driver_data;

@@ -539,6 +592,10 @@
 	.name		= "AMD_IDE",
 	.id_table	= amd74xx_pci_tbl,
 	.probe		= amd74xx_probe,
+#ifdef CONFIG_PM
+	.suspend	= amd74xx_suspend,
+	.resume		= amd74xx_resume,
+#endif
 };
 
 static int amd74xx_ide_init(void)

David Brownell

2006-07-27 20:35 UTC #10 lore raw

Re: [patch v3] amd74xx: fix hang on resume from ram

Your v3 patch worked for me on an NF3 system, modulo video issues I've yet to track down. It looks like that was otherwise the "last" bug blocking S3 from working on that box.

I can't say the same for an NF2 system, unfortunately. :(

- Dave

Pavel Machek

2006-07-26 09:02 UTC #11 lore raw

Re: [linux-pm] [patch, rft] amd74xx: implement suspend-to-ram

In reply to David Brownell

Hi!

Quoted 6 lines

Quoted 2 lines
and please don't do this. Suspend-to-disk will call amd74xx_resume when it needs to talk to the disk.
I don't understand the rest of this comment. That code looks correct (other than calling the goofy/broken pci_choose_state thing); PM_EVENT_FREEZE and PM_EVENT_PRETHAW should not disable the device.

Well, I'd prefer PM_EVENT_FREEZE and PM_EVENT_SUSPEND to be the same code (i.e. remove the if() and just do

Quoted 2 lines

Quoted 2 lines
drivers/ide/pci/amd74xx.c line 510
Quoted 2 lines
drivers/ide/pci/amd74xx.c +2 -0
+		pci_disable_device(dev);
+		pci_set_power_state(dev, pci_choose_state(dev, state));

unconditionaly). It should also work, and not really be slower.
Pavel

Signature · 2 lines

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

David Brownell

2006-07-27 00:29 UTC #12 lore raw

Re: [linux-pm] [patch, rft] amd74xx: implement suspend-to-ram

Quoted 9 lines · Pavel Machek

Hi!
Quoted 6 lines
Quoted 2 lines
and please don't do this. Suspend-to-disk will call amd74xx_resume when it needs to talk to the disk.
I don't understand the rest of this comment. That code looks correct (other than calling the goofy/broken pci_choose_state thing); PM_EVENT_FREEZE and PM_EVENT_PRETHAW should not disable the device.

Quoted 7 lines

Well, I'd prefer PM_EVENT_FREEZE and PM_EVENT_SUSPEND to be the same code (i.e. remove the if() and just do
Quoted 2 lines
Quoted 2 lines
drivers/ide/pci/amd74xx.c line 510
Quoted 2 lines
drivers/ide/pci/amd74xx.c +2 -0
+		pci_disable_device(dev);
+		pci_set_power_state(dev, pci_choose_state(dev, state));
unconditionaly). It should also work, and not really be slower.

Oh, I see. But wouldn't that cause FREEZE to spin down the drives?

Pavel Machek

2006-07-27 20:49 UTC #13 lore raw

Re: [linux-pm] [patch, rft] amd74xx: implement suspend-to-ram

Quoted 13 lines · David Brownell

Quoted 12 lines · Pavel Machek
Hi!
Quoted 6 lines
Quoted 2 lines
and please don't do this. Suspend-to-disk will call amd74xx_resume when it needs to talk to the disk.
I don't understand the rest of this comment. That code looks correct (other than calling the goofy/broken pci_choose_state thing); PM_EVENT_FREEZE and PM_EVENT_PRETHAW should not disable the device.
Well, I'd prefer PM_EVENT_FREEZE and PM_EVENT_SUSPEND to be the same code (i.e. remove the if() and just do

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
drivers/ide/pci/amd74xx.c line 510
Quoted 2 lines
drivers/ide/pci/amd74xx.c +2 -0
+		pci_disable_device(dev);
+		pci_set_power_state(dev, pci_choose_state(dev, state));
unconditionaly). It should also work, and not really be slower.
Oh, I see. But wouldn't that cause FREEZE to spin down the drives?

I do not think so. If it will, I'm obviously wrong and original version is okay.
Pavel

Signature · 2 lines

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

Jason Lunz

author

2006-07-27 22:43 UTC #14 lore raw

Re: [linux-pm] [patch, rft] amd74xx: implement suspend-to-ram

Quoted 4 lines · Pavel Machek

Quoted 1 line
Oh, I see. But wouldn't that cause FREEZE to spin down the drives?
I do not think so. If it will, I'm obviously wrong and original version is okay.

I think it's the ide bus shutdown methods that do power management on the drives. But here we're talking about the pci ide controller.

At any rate, not having the if() means my machine freezes during the suspend phase of STD.

Jason

Pavel Machek

2006-07-27 22:51 UTC #15 lore raw

Re: [linux-pm] [patch, rft] amd74xx: implement suspend-to-ram

Quoted 11 lines · Jason Lunz

Quoted 4 lines · Pavel Machek
Quoted 1 line
Oh, I see. But wouldn't that cause FREEZE to spin down the drives?
I do not think so. If it will, I'm obviously wrong and original version is okay.
I think it's the ide bus shutdown methods that do power management on the drives. But here we're talking about the pci ide controller.
At any rate, not having the if() means my machine freezes during the suspend phase of STD.

I'd still like to understand why it freezes (it should not), but I guess you have my ACK on that patch.
Pavel

Signature · 2 lines

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

Rafael J. Wysocki

2006-07-28 09:15 UTC #16 lore raw

Re: [linux-pm] [patch, rft] amd74xx: implement suspend-to-ram

Quoted 7 lines · Pavel Machek

Quoted 6 lines · Jason Lunz
Quoted 4 lines · Pavel Machek
Quoted 1 line
Oh, I see. But wouldn't that cause FREEZE to spin down the drives?
I do not think so. If it will, I'm obviously wrong and original version is okay.

Quoted 8 lines

Quoted 5 lines
I think it's the ide bus shutdown methods that do power management on the drives. But here we're talking about the pci ide controller.
At any rate, not having the if() means my machine freezes during the suspend phase of STD.
I'd still like to understand why it freezes (it should not), but I guess you have my ACK on that patch.

So I guess it needs to be forwarded to Andrew. Should I?

It helps to resume my box from RAM, btw. Now only the graphics doesn't wake up.

Rafael

Vojtech Pavlik

2006-07-28 13:23 UTC #17 lore raw

Re: [linux-pm] [patch, rft] amd74xx: implement suspend-to-ram

Quoted 11 lines · Rafael J. Wysocki

Quoted 10 lines · Pavel Machek
Quoted 9 lines · Jason Lunz
Quoted 4 lines · Pavel Machek
Quoted 1 line
Oh, I see. But wouldn't that cause FREEZE to spin down the drives?
I do not think so. If it will, I'm obviously wrong and original version is okay.
I think it's the ide bus shutdown methods that do power management on the drives. But here we're talking about the pci ide controller.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
At any rate, not having the if() means my machine freezes during the suspend phase of STD.
I'd still like to understand why it freezes (it should not), but I guess you have my ACK on that patch.
So I guess it needs to be forwarded to Andrew. Should I?

Yes, please do.

Quoted 7 lines

It helps to resume my box from RAM, btw. Now only the graphics doesn't wake up.
Rafael

Signature · 2 lines

Vojtech Pavlik
Director SuSE Labs