AHCI on Intel 82801GBM/GHM using pci quirk

trumpet_tom · 08-17-2009, 09:15 PM

I recently came across a patch to force my eee 1000h to use ahci without it being available as a bios option. Since I applied the patch I can't resume from suspend. I found the patch here: http://mjg59.livejournal.com/85504.html. I'm using it on kernel 2.6.31-rc6, although the patch is clearly from a fair while ago.

I am wondering if this patch has since been looked at elsewhere. One of the comments is about fixing this problem and mentions this:

Quote:

I will try adding 27c5 to the list of "DECLARE_PCI_FIXUP_RESUME_EARLY" lines

I am guessing this would help me too as I have this:

Code:

root@eeepc:/home/tom# lspci -nn
00:1f.2 SATA controller [0106]: Intel Corporation 82801GBM/GHM (ICH7 Family) SATA AHCI Controller [8086:27c5] (rev 02)

I read and noticed myself that changing the driver from the other one (PIIX) changes the number 27c4 to 27c5. Sadly this is as far as my knowledge of the matter extends.

Anyone have any ideas how I can get my laptop to resume from suspend again?

Thanks,
Tom

trumpet_tom · 08-23-2009, 08:12 AM

Well I'm guessing no-one has any ideas about this then. I've gone back to the ata_piix driver for now. Should this problem be pointed out anywhere else to get more attention?

lambchops468 · 09-26-2009, 04:20 PM

Check that livejournal page, I just posted

I am the original OP for that thread.

trumpet_tom · 09-26-2009, 09:25 PM

nice one for getting back about this, had all but given up the ghost! i just tried putting in the line you said but it still didn't work..maybe i put it in the wrong place? i put it after these lines in drivers/pci/quirks.c:

DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2652, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2653, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2680, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x27c4, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2828, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2928, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x292d, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x292e, quirk_ahci_sata);

#if 0
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2651, quirk_ahci_sata);
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x27c0, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2820, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2825, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2920, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2921, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2926, quirk_ahci_sata);
DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, 0x27c5, quirk_ahci_sata);
#endif

is that where you meant?
thanks a lot anyway,
tom

lambchops468 · 09-27-2009, 11:09 AM

Wrong spot, this is the right way

DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2652, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2653, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2680, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x27c4, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2828, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2928, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x292d, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x292e, quirk_ahci_sata);
DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, 0x27c5, quirk_ahci_sata);

#if 0
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2651, quirk_ahci_sata);
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x27c0, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2820, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2825, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2920, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2921, quirk_ahci_sata);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2926, quirk_ahci_sata);

#endif

just a little C background:
the stuff betwee "#if 0" and "#endif" doesn't get compiled in because "#if 0" evaluates to false, so the preprocessor (the thing that reads all the stuff with # in front before the compiler compiles the code) deletes those lines before the compiler reads them, so putting it there won't help.

trumpet_tom · 09-27-2009, 04:11 PM

yeah i also tried putting the line there. thanks for the background anyway! even with the line in the correct place the laptop won't resume. is there something else i might be missing?

trumpet_tom · 10-19-2009, 11:43 AM

This still isn't working on 2.6.31.4, clearly something else going on..

lambchops468 · 11-14-2009, 04:46 PM

so I finally upgraded to 2.6.31 and found the problem.

you can skip the following stuff from my notes and just download the attachment (patch)

Quote:

Kernel 2.6.31 brought with it:

author Alek Du <alek.du@intel.com>
Sat, 8 Aug 2009 00:46:19 +0000 (08:46 +0800)
committer Jesse Barnes <jbarnes@virtuousgeek.org>
Thu, 20 Aug 2009 16:08:45 +0000 (09:08 -0700)
commit c82f63e411f1b58427c103bd95af2863b1c96dd1
tree 4a18447facd22c384c871e312cb3183c01a44b2c tree | snapshot
parent 6c30c53fd5ae6a99a23ad78e90c428d2c8ffb07f commit | diff
PCI: check saved state before restore

Without the check, the config space may be filled with zeros. Though
the driver should try to avoid call restoring before saving, but the
pci layer also should check this.

Also removes the existing check in pci_restore_standard_config, since
it's superfluous with the new check in restore_state.

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Alek Du <alek.du@intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

However, this is a problem. During resume from suspend to ram, the kernel pci layer restores the registers for the SATA controller once, then says okay, and sets dev->state_saved = false. However, since the restore goes from highest address (the BARs [base address registers]) to lowest register, some of the higher registers are set as RO because according to the lower registers controller is in PIIX mode. Then the quirk ICH to AHCI mode quirk runs, changing some bits in the register configuration space and making the higher registers RW. Prior to 2.6.31, when the driver resumed the device, it would also call pci_restore_state() (in drivers/ata/libata-core.c) and it would restore the registers that were not written the first round (during the kernel pci layer restore). But since this commit added a check to see if saved_state is true, it won't restore the registers into the register space, which is now RW. This patch deletes this check. heh

2.6.30.6-debug-dmesg-resume -> generated using PCI debugging (kernel config: CONFIG_PCI_DEBUG=y)
the 2.6.31.5 dmesg would say: instead of:

[ 90.901283] ahci 0000:00:1f.2: resume
[ 90.901314] ahci 0000:00:1f.2: restoring config space at offset 0x9 (was 0x0, writing 0x58544000)
[ 90.901340] ahci 0000:00:1f.2: restoring config space at offset 0x1 (was 0x2b00405, writing 0x2b00407)
[ 90.901394] ahci 0000:00:1f.2: setting latency timer to 64

it would say:

ahci 0000:00:1f.2: resume
ahci 0000:00:1f.2: enabling device (0405 -> 0407)
ahci 0000:00:1f.2: setting latency timer to 64
ahci 0000:00:1f.2: controller reset failed (0xffffffff)

(this dmesg obtained by leaving the computer alone for >180 seconds (the time ahci takes to give up on reset) and resuming to a computer with no disk access but memory cache access for commands already run previously)

possible future problems (not merged into 2.6.31):
http://git.kernel.org/?p=linux/kerne...48d8319dda36ff

patch in plaintext for those who don't want to download (i intend to create a site with this patch...eventually)

Quote:

diff -uNr a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c 2009-09-09 18:13:59.000000000 -0400
+++ b/drivers/pci/pci-driver.c 2009-11-05 23:32:59.738049942 -0500
@@ -508,7 +508,7 @@
return error;
}

- return pci_restore_state(pci_dev);
+ return pci_dev->state_saved ? pci_restore_state(pci_dev) : 0;
}

static void pci_pm_default_resume_noirq(struct pci_dev *pci_dev)
diff -uNr a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c 2009-09-09 18:13:59.000000000 -0400
+++ b/drivers/pci/pci.c 2009-11-05 23:33:25.744719785 -0500
@@ -846,8 +846,6 @@
int i;
u32 val;

- if (!dev->state_saved)
- return 0;
/* PCI Express register must be restored first */
pci_restore_pcie_state(dev);

trumpet_tom · 11-16-2009, 05:26 PM

So, to clarify, I need the ahci quirk patch, the extra line in quirks.c and the pci-state patch?

For other reasons I'm using 2.6.32-rc7 at the moment and the combination of the three changes doesn't compile on that kernel. My fault I know! I'll try 2.6.31 when I get the chance. I can post the error if you're at all interested, it is complaining about previous redefinitions of all the variables. Maybe I just have messed up the patching.

Look forward to seeing it all come together!

UPDATE: Just put in the two patches and added the line. It works! Thanks!

lambchops468 · 01-31-2010, 02:17 PM

I updated (well, changed) the patch so that it doesn't affect the whole PCI system.

my notes:

Quote:

ahci-pci-restore-1.diff overrides the check in the ahci driver. We can keep the above commit and not affect all the PCI drivers. In the patch, ahci_pci_device_resume(pdev) (file drivers/ata/ahci.c) calls ata_pci_device_do_resume(pdev) (file drivers/ata/libata-core.c), which then calls pci_restore_state(pdev) (file drivers/pci/pci.c), where the check is located. Since the same device descriptor (pdev pointer) is passed down this call stack, we can just change the device descriptor's state_saved to = true in ahci_pci_device_resume (file drivers/ata/ahci.c), and the check in pci_restore_state(pdev) (file drivers/pci/pci.c) will pass.

I thought of this after seeing this commit:
author Kleber Sacilotto de Souza <klebers@linux.vnet.ibm.com>
Wed, 25 Nov 2009 22:13:43 +0000 (20:13 -0200)
committer Greg Kroah-Hartman <gregkh@suse.de>
Wed, 6 Jan 2010 23:03:11 +0000 (15:03 -0800)
commit c1d17da3cf8a961254fd18f4ddc8bbb743a33ab7
tree 60c3aa53504ac0496f12cacc548407f6a3c3ff69 tree | snapshot
parent a1092bf3809c7ec7b730d0cca3bdba964a813147 commit | diff
SCSI: ipr: fix EEH recovery

commit 99c965dd9ee1a004efc083c3d760ba982bb76adf upstream.

After commits c82f63e411f1b58427c103bd95af2863b1c96dd1 (PCI: check saved
state before restore) and 4b77b0a2ba27d64f58f16d8d4d48d8319dda36ff (PCI:
Clear saved_state after the state has been restored) PCI drivers are
prevented from restoring the device standard configuration registers
twice in a row. These changes introduced a regression on ipr EEH
recovery.

The ipr device driver saves the PCI state only during the device probe
and restores it on ipr_reset_restore_cfg_space() during IOA resets. This
behavior is causing the EEH recovery to fail after the second error
detected, since the registers are not being restored.

One possible solution would be saving the registers after restoring
them. The problem with this approach is that while recovering from an
EEH error if pci_save_state() results in an EEH error, the adapter/slot
will be reset, and end up back in ipr_reset_restore_cfg_space(), but it
won't have a valid saved state to restore, so pci_restore_state() will
fail.

The following patch introduces a workaround for this problem, hacking
around the PCI API by setting pdev->state_saved = true before we do the
restore. It fixes the EEH regression and prevents that we hit another
EEH error during EEH recovery.

[jejb: fix is a hack ... Jesse and Rafael will fix properly]
Signed-off-by: Kleber Sacilotto de Souza <klebers@linux.vnet.ibm.com>
Acked-by: Brian King <brking@linux.vnet.ibm.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: James Bottomley <James.Bottomley@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

patch in plaintext for those who don't want to download (i intend to create a site with this patch...eventually)

Quote:

diff -upNr a/drivers/ata/ahci.c b/drivers/ata/ahci.c
--- a/drivers/ata/ahci.c 2009-12-02 22:51:21.000000000 -0500
+++ b/drivers/ata/ahci.c 2010-01-30 22:40:14.600471423 -0500
@@ -2456,6 +2456,8 @@ static int ahci_pci_device_resume(struct
struct ata_host *host = dev_get_drvdata(&pdev->dev);
int rc;

+ //override check to see if PCI configuration space is already restored in pci_restore_state
+ pdev->state_saved = true;
rc = ata_pci_device_do_resume(pdev);
if (rc)
return rc;

trumpet_tom · 02-17-2010, 07:56 AM

So what's the state of this patch now? What needs to be applied, is it the small patch in your last post + the older one?

lambchops468 · 03-05-2010, 04:33 PM

sorry for the late reply :P

only the small, latest one is needed.