Patchwork [git,patches] libata updates for 3.3

login
register
mail settings
Submitter Lin Ming
Date Jan. 16, 2012, 5:23 a.m.
Message ID <1326691403.13517.21.camel@minggr>
Download mbox | patch
Permalink /patch/136229/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Lin Ming - Jan. 16, 2012, 5:23 a.m.
On Mon, 2012-01-16 at 09:15 +0800, Lin Ming wrote:
> On Sun, 2012-01-15 at 09:41 -0500, Jeff Garzik wrote:
> > On 01/14/2012 12:21 AM, Linus Torvalds wrote:
> > > On Sun, Jan 8, 2012 at 4:32 PM, Jeff Garzik<jeff@garzik.org>  wrote:
> > >>
> > >> Summary (very little excitement at all this time):
> > >>
> > >> 0) Will play around with git signed tags with the next update.
> > >>
> > >> 1) PM improvements, including runtime suspend/resume work
> > >
> > > Hmm. I don't know if this comes from the PM improvements or even this
> > > particular pull, but links that aren't connected are *really* slow.
> > >
> > > Annoyingly so.
> > >
> > > My Macbook Air that I finally can resume reliably again used to come
> > > back almost immediately from resume. No longer. And the reason seems
> > > to be this:
> > >
> > >   [  243.306149] ata_piix 0000:00:1f.2: setting latency timer to 64
> > >   [  243.306180] bcma: Found rev 6 PMU (capabilities 0x108C2606)
> > >   [  246.579648] ata1.01: failed to resume link (SControl 0)
> > >   [  246.735472] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
> > >   [  246.735485] ata1.01: SATA link down (SStatus 0 SControl 0)
> > >   [  246.743632] ata1.00: ACPI cmd ef/03:46:00:00:00:a0 (SET FEATURES)
> > > filtered out
> > >   [  246.744353] ata1.00: configured for UDMA/100
> > >   [  246.744537] sd 0:0:0:0: [sda] Starting disk
> > >   [  247.769806] ata2.00: failed to resume link (SControl 0)
> > >   [  248.796207] ata2.01: failed to resume link (SControl 0)
> > >   [  248.807665] ata2.00: SATA link down (SStatus 4 SControl 0)
> > >   [  248.807681] ata2.01: SATA link down (SStatus 0 SControl 0)
> > >   [  248.808338] PM: resume of devices complete after 5511.027 msecs
> > >   [  248.882074] PM: Finishing wakeup.
> > >
> > > Notice the basically five-second timeout all basically for "failed to
> > > resume link: for things that didn't have anything connected to them
> > > anyway.
> > >
> > > This is a bog-standard Intel controller, there's nothing odd there.
> > >
> > > I'm pretty sure this used to be much faster, but I haven't bisected
> > > any of it (and with all the problems I had with resume both due to
> > > wireless and MCE, I really wouldn't want to even try).
> > >
> > > Taking 5.5 seconds to come back from suspend-to-ram really is too
> > > long. Not *all* of it is the SATA part, but a lot of it is.
> > >
> > > For ATA suspend/resume, could we perhaps only resume the ports that
> > > *used* to have something on them? And then, if somebody has plugged
> > > something into the others, not consider that a resume thing at all,
> > > but a hotplug thing that happens *after* the resume?
> > >
> > > If it takes five seconds to notice new hardware after a resume, nobody
> > > cares. But the disk we had before obviously needs to get resumed.. But
> > > it does seem like it's the "no link" part that takes long.
> > 
> > We definitely notice new hardware after a resume, but you're right -- it 
> > should not take that long to work through ports that are empty.
> > 
> > Will take a look tomorrow (kid->doctor+relatives today, uff) at the most 
> > recent PM push; my quick testing did not show any problems, but 
> > suspend/resume varies widely across hardware platforms.  I think I might 
> > even have a MacBook I can test.  Apple platforms test to be weird too...  ;)
> 
> I just did a quick test with latest git head(122804e) and didn't find
> the problem either.
> 
> I'll test other machines.

Set SATA mode to IDE on my machine can reproduce this problem.
The cause is that ata port async suspend was not enabled yet.

Linus,

Could you please try below patch?

From ba9cba4df9e9389c4f11202e335bee6383b2868c Mon Sep 17 00:00:00 2001
From: Lin Ming <ming.m.lin@intel.com>
Date: Mon, 16 Jan 2012 13:09:02 +0800
Subject: [PATCH] ata: enable ata port async suspend

This saves devices suspend/resume time.

Tested system suspend/resume with SATA IDE/AHCI mode 3 times.
Below is the time took for devices suspend/resume.

SATA mode    vanilla-kernel           patched-kernel
---------    ---------------------    ---------------------
IDE          suspend: 0.744           suspend: 0.432
             (0.716, 0.768, 0.748)    (0.440, 0.428, 0.428)

             resume: 5.084            resume: 2.209
             (5.100, 5.064, 5.088)    (2.168, 2.232, 2.228)

AHCI:        suspend: 0.725           suspend: 0.449
             (0.740, 0.708, 0.728)    (0.456, 0.448, 0.444)

             resume: 2.556            resume: 1.896
             (2.604, 2.492, 2.572)    (1.932, 1.872, 1.884)

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/ata/libata-transport.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Linus Torvalds - Jan. 16, 2012, 7:25 p.m.
On Sun, Jan 15, 2012 at 9:23 PM, Lin Ming <ming.m.lin@intel.com> wrote:
>
> Set SATA mode to IDE on my machine can reproduce this problem.
> The cause is that ata port async suspend was not enabled yet.

Yup. Your patch helps.

But why don't we fix the silly legacy mode thing? Yeah, Apple's
firmware is crap and doesn't tend to initialize things, but still - we
should have at least an option to not use the legacy PIIX mode just
because the chipset wasn't programmed for AHCI by the bootup.

That would also help with people who have a BIOS that has the option,
but where the person hadn't noticed, and left it in legacy mode.

The IO ports are all there, the *only* reason we don't use AHCI seems
to be that the firmware didn't turn off the legacy bit, so it doesn't
show as an AHCI interface. That's kind of sad.

                Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - Jan. 16, 2012, 7:29 p.m.
On Mon, Jan 16, 2012 at 11:25 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But why don't we fix the silly legacy mode thing? Yeah, Apple's
> firmware is crap and doesn't tend to initialize things, but still - we
> should have at least an option to not use the legacy PIIX mode just
> because the chipset wasn't programmed for AHCI by the bootup.

Here's the lspci output for that device, in case anybody cares:


  00:1f.2 IDE interface [0101]: Intel Corporation 6 Series/C200 Series
Chipset Family 4 port SATA IDE Controller [8086:1c01] (rev 05)
(prog-if 8f [Master SecP SecO PriP PriO])
        Subsystem: Intel Corporation Device [8086:7270]
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
ParErr- Stepping- SERR- FastB2B- DisINTx-
        Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium
>TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
        Latency: 0
        Interrupt: pin B routed to IRQ 19
        Region 0: I/O ports at 2168 [size=8]
        Region 1: I/O ports at 217c [size=4]
        Region 2: I/O ports at 2160 [size=8]
        Region 3: I/O ports at 2178 [size=4]
        Region 4: I/O ports at 2060 [size=16]
        Region 5: I/O ports at ffe0 [size=16]
        Capabilities: <access denied>
        Kernel driver in use: ata_piix
  00: 86 80 01 1c 07 00 b0 02 05 8f 01 01 00 00 00 00
  10: 69 21 00 00 7d 21 00 00 61 21 00 00 79 21 00 00
  20: 61 20 00 00 e1 ff 00 00 00 00 00 00 86 80 70 72
  30: 00 00 00 00 70 00 00 00 00 00 00 00 0b 02 00 00

and I do think that resume used to be faster in v3.2 than it is even
with your patch. But maybe that's some rose-colored glasses.

I'll recompile an old kernel to check.

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Jan. 16, 2012, 7:42 p.m.
> The IO ports are all there, the *only* reason we don't use AHCI seems
> to be that the firmware didn't turn off the legacy bit, so it doesn't
> show as an AHCI interface. That's kind of sad.

There are two sets of problems with that

The first is that quite a few machines crap themselves if you do this
because nobody has ever tested things like the IRQ routing or the
firmware on suspend/resume when they find the hardwared controller has
gone for a walk.

Putting it back on suspend might help in some cases but then you get to
pick your way through the documentation minefield, deal with device
changes while in non AHCI mode, figure out how to set up registers the
BIOS didn't etc.

Would be a good project for someone with a lot of patience anyway as the
AHCI mode is also 64bit capable and *much* faster overall as it doesn't
keep stalling the CPU poking imaginary registers and turning them into
messages on the SATA bus.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - Jan. 16, 2012, 7:47 p.m.
On Mon, Jan 16, 2012 at 11:42 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> There are two sets of problems with that
>
> The first is that quite a few machines crap themselves if you do this
> because nobody has ever tested things like the IRQ routing or the
> firmware on suspend/resume when they find the hardwared controller has
> gone for a walk.

I think this could trivially be solved at least on these kinds of
Apple machines by just making it a config option, and just not doing
it if it doesn't work. Some kernel command line to say "ahci=force" or
whatever.

That doesn't solve the "user never even realized" problem, but at
least it gives the user the possibility of solving the "crap firmware
doesn't even allow this".

> Putting it back on suspend might help in some cases but then you get to
> pick your way through the documentation minefield, deal with device
> changes while in non AHCI mode, figure out how to set up registers the
> BIOS didn't etc.

Yeah, no, that would just be horrible. I doubt it happens in practice,
though.  I bet the normal PCI/AHCI resume will just do the right
thing. But I haven't tried..

                        Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Jan. 16, 2012, 7:54 p.m.
On Mon, 16 Jan 2012 11:47:12 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, Jan 16, 2012 at 11:42 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >
> > There are two sets of problems with that
> >
> > The first is that quite a few machines crap themselves if you do this
> > because nobody has ever tested things like the IRQ routing or the
> > firmware on suspend/resume when they find the hardwared controller has
> > gone for a walk.
> 
> I think this could trivially be solved at least on these kinds of
> Apple machines by just making it a config option, and just not doing
> it if it doesn't work. Some kernel command line to say "ahci=force" or
> whatever.

There are patches for this from way back including a white list of
devices that were tested

Just ping Matthew about

http://www.codon.org.uk/~mjg59/tmp/ahci_quirk_cleanup.diff

> That doesn't solve the "user never even realized" problem, but at
> least it gives the user the possibility of solving the "crap firmware
> doesn't even allow this".

For the "memory found over 4GB" case it may even be worth printing a
warning.

> > Putting it back on suspend might help in some cases but then you get to
> > pick your way through the documentation minefield, deal with device
> > changes while in non AHCI mode, figure out how to set up registers the
> > BIOS didn't etc.
> 
> Yeah, no, that would just be horrible. I doubt it happens in practice,
> though.  I bet the normal PCI/AHCI resume will just do the right
> thing. But I haven't tried..

Some BIOSes go off and try and re-unlock the drive with the bios password
etc. They get most unhappy when the controller isn't in the mode they
expected. Matthew used a white list for some he tested which didn't have
problems. Most netbooks seem ok with it.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - Jan. 16, 2012, 7:55 p.m.
On Mon, Jan 16, 2012 at 11:29 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> and I do think that resume used to be faster in v3.2 than it is even
> with your patch. But maybe that's some rose-colored glasses.
>
> I'll recompile an old kernel to check.

Confirmed.

Plain v3.2 really *is* faster than current git, even current git with
your patch.

In v3.2 I get this:

 [   92.035600] ata_piix 0000:00:1f.2: PCI INT B -> GSI 19 (level,
low) -> IRQ 19
 [   92.035610] ata_piix 0000:00:1f.2: setting latency timer to 64
 [   92.036213] sd 0:0:0:0: [sda] Starting disk
 [   93.060471] ata2.00: failed to resume link (SControl 0)
 [   93.379963] ata1.01: failed to resume link (SControl 0)
 [   93.535802] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
 [   93.535815] ata1.01: SATA link down (SStatus 0 SControl 0)
 [   93.543968] ata1.00: ACPI cmd ef/03:46:00:00:00:a0 (SET FEATURES)
filtered out
 [   93.544747] ata1.00: configured for UDMA/100
 [   93.916453] PM: resume of devices complete after 1883.974 msecs
 [   93.916677] PM: Finishing wakeup.
 [   94.086800] ata2.01: failed to resume link (SControl 0)
 [   94.098408] ata2.00: SATA link down (SStatus 4 SControl 0)
 [   94.098429] ata2.01: SATA link down (SStatus 0 SControl 0)

while current git (with your patch) gives me

 [  108.115373] ata_piix 0000:00:1f.2: setting latency timer to 64
 [  109.142010] ata2.00: failed to resume link (SControl 0)
 [  109.462004] ata1.01: failed to resume link (SControl 0)
 [  109.618065] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
 [  109.618078] ata1.01: SATA link down (SStatus 0 SControl 0)
 [  109.626242] ata1.00: ACPI cmd ef/03:46:00:00:00:a0 (SET FEATURES)
filtered out
 [  109.627060] ata1.00: configured for UDMA/100
 [  109.627240] sd 0:0:0:0: [sda] Starting disk
 [  110.170015] ata2.01: failed to resume link (SControl 0)
 [  110.181480] ata2.00: SATA link down (SStatus 4 SControl 0)
 [  110.181496] ata2.01: SATA link down (SStatus 0 SControl 0)
 [  110.182124] PM: resume of devices complete after 2066.945 msecs
 [  110.224533] PM: Finishing wakeup.

so old kernels used to be a tiny bit faster despite not doing that
async thing (still slower than I'd like: I'd think that we should be
able to resume devices in less than a second, but I don't know where
all the time goes)

             Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - Jan. 16, 2012, 8:02 p.m.
On Mon, Jan 16, 2012 at 11:54 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> Just ping Matthew about
>
> http://www.codon.org.uk/~mjg59/tmp/ahci_quirk_cleanup.diff

Hmm. This with just a setup option to enable it might well be good
enough. No need for whitelists/blacklists, since we couldn't maintain
those sanely anyway.

Matthew?

Googling shows that there's a few old bugreports for this too (big
surprise: macs):

  https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.20/+bug/96692

and various other ones..

                 Linus

                      Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - Jan. 16, 2012, 8:21 p.m.
On Mon, Jan 16, 2012 at 12:02 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>>
>> http://www.codon.org.uk/~mjg59/tmp/ahci_quirk_cleanup.diff
>
> Hmm. This with just a setup option to enable it might well be good
> enough. No need for whitelists/blacklists, since we couldn't maintain
> those sanely anyway.

That patch doesn't work for me. I get a panic in ahci_enable_ahci,
looks like 'mmio' is bogus there (attempting to dereference 0x01ffe6).

I notice that the PIIX device doesn't have any MMIO at all, it's all
PIO. Maybe there is some "need to enable MMIO thing" too. Or maybe I
used the wrong PCI ID when I added it to the quirk table.

                       Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Jan. 16, 2012, 8:27 p.m.
On 01/16/2012 03:21 PM, Linus Torvalds wrote:
> On Mon, Jan 16, 2012 at 12:02 PM, Linus Torvalds
> <torvalds@linux-foundation.org>  wrote:
>>>
>>> http://www.codon.org.uk/~mjg59/tmp/ahci_quirk_cleanup.diff
>>
>> Hmm. This with just a setup option to enable it might well be good
>> enough. No need for whitelists/blacklists, since we couldn't maintain
>> those sanely anyway.
>
> That patch doesn't work for me. I get a panic in ahci_enable_ahci,
> looks like 'mmio' is bogus there (attempting to dereference 0x01ffe6).
>
> I notice that the PIIX device doesn't have any MMIO at all, it's all
> PIO. Maybe there is some "need to enable MMIO thing" too. Or maybe I
> used the wrong PCI ID when I added it to the quirk table.

Part of the problem with force-enable is that the MMIO BAR may need a 
value, and not have it.  That, and an expectations mismatch between BIOS 
and kernel WRT mode (IDE/AHCI) has always been the reason why 
'ahci=force' never made it in.  It just never seemed to be reliable broadly.

I'm happy with anything we can get working reliably, even with the 
obvious proviso that 'ahci=force' would be a default-off, user-enabled 
option.  It's always been motherboard/BIOS issues that got in the way.

	Jeff





--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett - Jan. 16, 2012, 9:26 p.m.
On Mon, Jan 16, 2012 at 12:02:32PM -0800, Linus Torvalds wrote:
> On Mon, Jan 16, 2012 at 11:54 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >
> > Just ping Matthew about
> >
> > http://www.codon.org.uk/~mjg59/tmp/ahci_quirk_cleanup.diff
> 
> Hmm. This with just a setup option to enable it might well be good
> enough. No need for whitelists/blacklists, since we couldn't maintain
> those sanely anyway.
> 
> Matthew?

Last time I tested it it wasn't dealing well with setting up combination 
mode for devices with optical media, and I don't have much hardware 
lying around which still uses piix. But if I can find some time I'll dig 
something up and see if it still works. Or, if anyone else wants to do 
the same...

> Googling shows that there's a few old bugreports for this too (big
> surprise: macs):

Macs will come up with ahci if you boot via EFI rather than the BIOS 
compatibility module.
Linus Torvalds - Jan. 16, 2012, 9:34 p.m.
On Mon, Jan 16, 2012 at 1:26 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> Macs will come up with ahci if you boot via EFI rather than the BIOS
> compatibility module.

Apparently not this one. Or there is something else subtly wrong. I
use rEFIt to boot Linux on this thing, which should mean that it
booted as an EFI image, even if the kernel itself is not natively
using EFI (or even built to care).

But maybe rEFIt does something else than I expected it to do, so who knows..

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett - Jan. 16, 2012, 10:03 p.m.
On Mon, Jan 16, 2012 at 01:34:53PM -0800, Linus Torvalds wrote:
> On Mon, Jan 16, 2012 at 1:26 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> >
> > Macs will come up with ahci if you boot via EFI rather than the BIOS
> > compatibility module.
> 
> Apparently not this one. Or there is something else subtly wrong. I
> use rEFIt to boot Linux on this thing, which should mean that it
> booted as an EFI image, even if the kernel itself is not natively
> using EFI (or even built to care).

refit is an EFI app, but it's usually used to trigger a BIOS-based boot.
Alan Cox - Jan. 16, 2012, 11:54 p.m.
> Part of the problem with force-enable is that the MMIO BAR may need a 
> value, and not have it.  That, and an expectations mismatch between BIOS 

That's fine - we can happily assign one at boot time.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - Jan. 17, 2012, 12:02 a.m.
On Mon, Jan 16, 2012 at 3:54 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Part of the problem with force-enable is that the MMIO BAR may need a
>> value, and not have it.  That, and an expectations mismatch between BIOS
>
> That's fine - we can happily assign one at boot time.

Actually, I think the reason that Matthews patch doesn't work for me
is that on my device, BAR #5 really is a _port_ BAR.

I didn't play with it much - busy merging and looking at various other
issues - but I'm starting to wonder whether maybe that 8086:1c01 chip
doesn't support AHCI at all. Or maybe it does something differently.

Other reports of this have BAR#5 either clear, or an MMIO BAR.   That

   Region 5: I/O ports at ffe0 [size=16]

looks really odd. Matthew's patch uses

  pci_assign_resource(pdev, 5);

but if it is a PIO region, that won't help anything..

That may be why it oopses for me - the AHCI driver is trying to use a
PIO address as an MMIO one with readl().

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett - Jan. 17, 2012, 12:18 a.m.
On Mon, Jan 16, 2012 at 04:02:31PM -0800, Linus Torvalds wrote:
> On Mon, Jan 16, 2012 at 3:54 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >> Part of the problem with force-enable is that the MMIO BAR may need a
> >> value, and not have it.  That, and an expectations mismatch between BIOS
> >
> > That's fine - we can happily assign one at boot time.
> 
> Actually, I think the reason that Matthews patch doesn't work for me
> is that on my device, BAR #5 really is a _port_ BAR.
> 
> I didn't play with it much - busy merging and looking at various other
> issues - but I'm starting to wonder whether maybe that 8086:1c01 chip
> doesn't support AHCI at all. Or maybe it does something differently.
> 
> Other reports of this have BAR#5 either clear, or an MMIO BAR.   That
> 
>    Region 5: I/O ports at ffe0 [size=16]
> 
> looks really odd. Matthew's patch uses
> 
>   pci_assign_resource(pdev, 5);
> 
> but if it is a PIO region, that won't help anything..

More recent ICHs may need different setup here. The datasheet should 
have enough to figure it out, but I can take a look once I get back from 
LCA.
Lin Ming - Jan. 17, 2012, 5:16 a.m.
On Mon, 2012-01-16 at 11:55 -0800, Linus Torvalds wrote:
> On Mon, Jan 16, 2012 at 11:29 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > and I do think that resume used to be faster in v3.2 than it is even
> > with your patch. But maybe that's some rose-colored glasses.
> >
> > I'll recompile an old kernel to check.
> 
> Confirmed.
> 
> Plain v3.2 really *is* faster than current git, even current git with
> your patch.
> 
> In v3.2 I get this:
> 
>  [   92.035600] ata_piix 0000:00:1f.2: PCI INT B -> GSI 19 (level,
> low) -> IRQ 19
>  [   92.035610] ata_piix 0000:00:1f.2: setting latency timer to 64
>  [   92.036213] sd 0:0:0:0: [sda] Starting disk
>  [   93.060471] ata2.00: failed to resume link (SControl 0)
>  [   93.379963] ata1.01: failed to resume link (SControl 0)
>  [   93.535802] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>  [   93.535815] ata1.01: SATA link down (SStatus 0 SControl 0)
>  [   93.543968] ata1.00: ACPI cmd ef/03:46:00:00:00:a0 (SET FEATURES)
> filtered out
>  [   93.544747] ata1.00: configured for UDMA/100
>  [   93.916453] PM: resume of devices complete after 1883.974 msecs
>  [   93.916677] PM: Finishing wakeup.
>  [   94.086800] ata2.01: failed to resume link (SControl 0)
>  [   94.098408] ata2.00: SATA link down (SStatus 4 SControl 0)
>  [   94.098429] ata2.01: SATA link down (SStatus 0 SControl 0)
> 
> while current git (with your patch) gives me
> 
>  [  108.115373] ata_piix 0000:00:1f.2: setting latency timer to 64
>  [  109.142010] ata2.00: failed to resume link (SControl 0)
>  [  109.462004] ata1.01: failed to resume link (SControl 0)
>  [  109.618065] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>  [  109.618078] ata1.01: SATA link down (SStatus 0 SControl 0)
>  [  109.626242] ata1.00: ACPI cmd ef/03:46:00:00:00:a0 (SET FEATURES)
> filtered out
>  [  109.627060] ata1.00: configured for UDMA/100
>  [  109.627240] sd 0:0:0:0: [sda] Starting disk
>  [  110.170015] ata2.01: failed to resume link (SControl 0)
>  [  110.181480] ata2.00: SATA link down (SStatus 4 SControl 0)
>  [  110.181496] ata2.01: SATA link down (SStatus 0 SControl 0)
>  [  110.182124] PM: resume of devices complete after 2066.945 msecs
>  [  110.224533] PM: Finishing wakeup.
> 
> so old kernels used to be a tiny bit faster despite not doing that
> async thing (still slower than I'd like: I'd think that we should be

No, old kernel did that async thing.

Old kernel embedded the ata port suspend/resume code in host
suspend/resume code and host has async suspend enabled.

I split the ata port suspend/resume code out.

ata: add ata port system PM callbacks
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=5ef4108

But ata port async suspend was not enabled in above patch.
So I think this is a regression.

Thanks,
Lin Ming

> able to resume devices in less than a second, but I don't know where
> all the time goes)
> 
>              Linus


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - Jan. 17, 2012, 5:19 a.m.
On Mon, Jan 16, 2012 at 9:16 PM, Lin Ming <ming.m.lin@intel.com> wrote:
>
> But ata port async suspend was not enabled in above patch.
> So I think this is a regression.

Ok. Jeff? Should I take that patch directly, or will you take it?

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Jan. 17, 2012, 4:51 p.m.
On 01/17/2012 12:19 AM, Linus Torvalds wrote:
> On Mon, Jan 16, 2012 at 9:16 PM, Lin Ming<ming.m.lin@intel.com>  wrote:
>>
>> But ata port async suspend was not enabled in above patch.
>> So I think this is a regression.
>
> Ok. Jeff? Should I take that patch directly, or will you take it?

Coming today, with at least one other change...  still keeping an eye 
out for possible PM problems on older non-AHCI SATA hardware.

	Jeff




--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Jan. 17, 2012, 5 p.m.
On 01/16/2012 02:55 PM, Linus Torvalds wrote:
> so old kernels used to be a tiny bit faster despite not doing that
> async thing (still slower than I'd like: I'd think that we should be
> able to resume devices in less than a second, but I don't know where
> all the time goes)

In libata specifically, we remain peppered liberally with a bunch of 
msleep()'s all over the place, particularly when dealing with SATA phys. 
  10ms here, 20ms there, 200ms in sata_link_resume(), it all adds up 
when you're talking about fast boot.

For most hardware, we can scan ports in parallel, which gets you past 
the biggest annoyances, but not all of them.

	Jeff


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 9a7f0ea..74aaee3 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -291,6 +291,7 @@  int ata_tport_add(struct device *parent,
 		goto tport_err;
 	}
 
+	device_enable_async_suspend(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);