Patchwork Regression 3.2 -> 3.3-rc1 10 sec hang at boot and resume, COMRESET failed

login
register
mail settings
Submitter Lin Ming
Date Feb. 6, 2012, 1:36 a.m.
Message ID <1328492218.15079.38.camel@minggr>
Download mbox | patch
Permalink /patch/139676/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Lin Ming - Feb. 6, 2012, 1:36 a.m.
On Mon, 2012-02-06 at 09:46 +0900, Norbert Preining wrote:
> Hi Lin,
> 
> 
> sorry for the delay, weekend I was off ...
> 
> On Fr, 03 Feb 2012, Lin Ming wrote:
> > > Confirmed. Reverted 7faa33da9b7 on top of 6c073a7ee250 made
> > > the boot delay go away. dmesg from this boot attached.
> > 
> > Dig into the code, but I can't find where the problem is.
> > 
> > Anyway, does below DEBUG patch help?
> > Let's always stop the engine during hard reset.
> 
> If you meant: 
>   "Try that patch on top of HEAD *without* reverting 7faa33da9b7?"
> then I can report that it does NOT help. With *only* this patch I still
> get 10sec delay, and otherwise nothing changes.

Does below help?





--
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
Norbert Preining - Feb. 6, 2012, 2:40 a.m.
On Mo, 06 Feb 2012, Lin Ming wrote:
> > > Anyway, does below DEBUG patch help?
> > > Let's always stop the engine during hard reset.
> > 
> > If you meant: 
> >   "Try that patch on top of HEAD *without* reverting 7faa33da9b7?"
> > then I can report that it does NOT help. With *only* this patch I still
> > get 10sec delay, and otherwise nothing changes.
> 
> Does below help?

In which combination - with or without the first patch?

Best wishes

Norbert
------------------------------------------------------------------------
Norbert Preining            preining@{jaist.ac.jp, logic.at, debian.org}
JAIST, Japan                                 TeX Live & Debian Developer
DSA: 0x09C5B094   fp: 14DF 2E6C 0307 BE6D AD76  A9C0 D2BF 4AA3 09C5 B094
------------------------------------------------------------------------
HERSTMONCEUX (n.)
The correct name for the gold medallion worn by someone who is in the
habit of wearing their shirt open to the waist.
			--- Douglas Adams, The Meaning of Liff
--
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
Lin Ming - Feb. 6, 2012, 2:49 a.m.
On Mon, 2012-02-06 at 11:40 +0900, Norbert Preining wrote:
> On Mo, 06 Feb 2012, Lin Ming wrote:
> > > > Anyway, does below DEBUG patch help?
> > > > Let's always stop the engine during hard reset.
> > > 
> > > If you meant: 
> > >   "Try that patch on top of HEAD *without* reverting 7faa33da9b7?"
> > > then I can report that it does NOT help. With *only* this patch I still
> > > get 10sec delay, and otherwise nothing changes.
> > 
> > Does below help?
> 
> In which combination - with or without the first patch?

Without.

Only this patch on top of HEAD *without* reverting 7faa33da9b7.

> 
> Best wishes
> 
> Norbert
> ------------------------------------------------------------------------
> Norbert Preining            preining@{jaist.ac.jp, logic.at, debian.org}
> JAIST, Japan                                 TeX Live & Debian Developer
> DSA: 0x09C5B094   fp: 14DF 2E6C 0307 BE6D AD76  A9C0 D2BF 4AA3 09C5 B094
> ------------------------------------------------------------------------
> HERSTMONCEUX (n.)
> The correct name for the gold medallion worn by someone who is in the
> habit of wearing their shirt open to the waist.
> 			--- Douglas Adams, The Meaning of Liff


--
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
Norbert Preining - Feb. 6, 2012, 3:15 a.m.
On Mo, 06 Feb 2012, Lin Ming wrote:
> Only this patch on top of HEAD *without* reverting 7faa33da9b7.

Works. Am I right that it differs from 7faa33da9b7 only in that
the later also changes:
@@ -2019,7 +2022,7 @@ static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
                ahci_power_down(ap);
        else {
                ata_port_err(ap, "%s (%d)\n", emsg, rc);
-               ata_port_freeze(ap);
+               ahci_start_port(ap);
        }


Best wishes

Norbert
------------------------------------------------------------------------
Norbert Preining            preining@{jaist.ac.jp, logic.at, debian.org}
JAIST, Japan                                 TeX Live & Debian Developer
DSA: 0x09C5B094   fp: 14DF 2E6C 0307 BE6D AD76  A9C0 D2BF 4AA3 09C5 B094
------------------------------------------------------------------------
HOVE (adj.)
Descriptive of the expression seen on the face of one person in the
presence of another who clearly isn't going to stop talking for a very
long time.
			--- Douglas Adams, The Meaning of Liff
--
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
Lin Ming - Feb. 6, 2012, 4:42 a.m.
On Mon, 2012-02-06 at 12:15 +0900, Norbert Preining wrote:
> On Mo, 06 Feb 2012, Lin Ming wrote:
> > Only this patch on top of HEAD *without* reverting 7faa33da9b7.
> 
> Works. Am I right that it differs from 7faa33da9b7 only in that
> the later also changes:

Right.

Tejun,

This regression is caused by 7faa33da9b7(ahci: start engine only during
soft/hard resets).

But I can't reproduce it.

What's your guess?

Thanks,
Lin Ming

> @@ -2019,7 +2022,7 @@ static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
>                 ahci_power_down(ap);
>         else {
>                 ata_port_err(ap, "%s (%d)\n", emsg, rc);
> -               ata_port_freeze(ap);
> +               ahci_start_port(ap);
>         }
> 
> 
> Best wishes
> 
> Norbert
> ------------------------------------------------------------------------
> Norbert Preining            preining@{jaist.ac.jp, logic.at, debian.org}
> JAIST, Japan                                 TeX Live & Debian Developer
> DSA: 0x09C5B094   fp: 14DF 2E6C 0307 BE6D AD76  A9C0 D2BF 4AA3 09C5 B094
> ------------------------------------------------------------------------
> HOVE (adj.)
> Descriptive of the expression seen on the face of one person in the
> presence of another who clearly isn't going to stop talking for a very
> long time.
> 			--- Douglas Adams, The Meaning of Liff


--
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
Tejun Heo - Feb. 6, 2012, 4:19 p.m.
Hello,

(cc'ing Jian Peng, hi)

On Mon, Feb 06, 2012 at 12:42:56PM +0800, Lin Ming wrote:
> On Mon, 2012-02-06 at 12:15 +0900, Norbert Preining wrote:
> > On Mo, 06 Feb 2012, Lin Ming wrote:
> > > Only this patch on top of HEAD *without* reverting 7faa33da9b7.
> > 
> > Works. Am I right that it differs from 7faa33da9b7 only in that
> > the later also changes:
> 
> Right.
> 
> Tejun,
> 
> This regression is caused by 7faa33da9b7(ahci: start engine only during
> soft/hard resets).
> 
> But I can't reproduce it.
> 
> What's your guess?

Urgh.... yeah, following standard can sometimes be silly thing to do.
Jian, I think we'll have to add a flag for your controller and revert
to the original behavior for others.  How can your controller be
distinguished?

Thanks.
Brian Norris - Feb. 6, 2012, 7:20 p.m.
On Mon, Feb 6, 2012 at 8:19 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> (cc'ing Jian Peng, hi)

Hello,

Jian Peng is no longer working on this project; I have taken over for his work.

> On Mon, Feb 06, 2012 at 12:42:56PM +0800, Lin Ming wrote:
>> On Mon, 2012-02-06 at 12:15 +0900, Norbert Preining wrote:
>> > On Mo, 06 Feb 2012, Lin Ming wrote:
>> > > Only this patch on top of HEAD *without* reverting 7faa33da9b7.
>> >
>> > Works. Am I right that it differs from 7faa33da9b7 only in that
>> > the later also changes:
>>
>> Right.
>>
>> Tejun,
>>
>> This regression is caused by 7faa33da9b7(ahci: start engine only during
>> soft/hard resets).
>>
>> But I can't reproduce it.
>>
>> What's your guess?
>
> Urgh.... yeah, following standard can sometimes be silly thing to do.
> Jian, I think we'll have to add a flag for your controller and revert
> to the original behavior for others.  How can your controller be
> distinguished?

Our controller utilizes the ahci_platform.c driver and does not have a
distinguishing interface ID (e.g., PCI ID). It is maintained in an
out-of-tree distribution, but we would like to have a
standards-compliant solution in mainline so that we do not have to
support a fork of the main codebase.

Is there any possibility of debugging this regression instead of
effectively reverting it for mainline? Or perhaps can I have better
information regarding the hardware on which this regression is seen?
With some time, I can try to debug it further.

I see the following options:
(1) implement a flag that can be passed through ahci_platform; this
would not be very useful, as we would still have to tweak the driver
out of tree.
(2) Drop the fix entirely. This is a spec. violation, but we can
simply try to maintain the fix out-of-tree.
(3) Debug Norbert's hardware problems.

(1) or (2) aren't ideal, and they are effectively very similar. The
only advantage to (1) is that there will be at least some illusion of
our controller's support in future kernel development.

Brian
--
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
Norbert Preining - Feb. 6, 2012, 10:46 p.m.
Hi Brian,

On Mo, 06 Feb 2012, Brian Norris wrote:
> Is there any possibility of debugging this regression instead of
> effectively reverting it for mainline? Or perhaps can I have better
> information regarding the hardware on which this regression is seen?

Sure, Sony VAIO Z11 laptop. I have attached several dmesg output
to emails in this thread, that should give you a good idea what
the hardware is. FOr the ahci:
$ lspci -v -s 00:1f.2
00:1f.2 SATA controller: Intel Corporation 82801IBM/IEM (ICH9M/ICH9M-E) 4 port SATA Controller [AHCI mode] (rev 03) (prog-if 01 [AHCI 1.0])
	Subsystem: Sony Corporation Device 9025
	Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 44
	I/O ports at 7128 [size=8]
	I/O ports at 713c [size=4]
	I/O ports at 7120 [size=8]
	I/O ports at 7138 [size=4]
	I/O ports at 7020 [size=32]
	Memory at daa25000 (32-bit, non-prefetchable) [size=2K]
	Capabilities: [80] MSI: Enable+ Count=1/16 Maskable- 64bit-
	Capabilities: [70] Power Management version 3
	Capabilities: [a8] SATA HBA v1.0
	Capabilities: [b0] PCI Advanced Features
	Kernel driver in use: ahci

If you need any other information, please let me know.

Best wishes

Norbert
------------------------------------------------------------------------
Norbert Preining            preining@{jaist.ac.jp, logic.at, debian.org}
JAIST, Japan                                 TeX Live & Debian Developer
DSA: 0x09C5B094   fp: 14DF 2E6C 0307 BE6D AD76  A9C0 D2BF 4AA3 09C5 B094
------------------------------------------------------------------------
FIUNARY (n.)
The safe place you put something and then forget where it was.
			--- Douglas Adams, The Meaning of Liff
--
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
Valdis.Kletnieks@vt.edu - Feb. 8, 2012, 9:10 a.m.
On Mon, 06 Feb 2012 11:20:45 PST, Brian Norris said:

> (3) Debug Norbert's hardware problems.

It's not just Norbert - my Dell Latitude E6500 trips over this as well (didn't
we go through this same song-and-dance *before* with this same exact
patch?  Yes we did, back in May 2011:

http://lkml.indiana.edu/hypermail/linux/kernel/1105.1/02970.html

Somebody hand me a stake and mallet to pount through the heart of this
patch so it *stays* dead already.. Geez.. ;)

Norbert's box has this:
00:1f.2 SATA controller: Intel Corporation 82801IBM/IEM (ICH9M/ICH9M-E) 4 port SATA Controller [AHCI mode] (rev 03) (prog-if 01 [AH
CI 1.0])

While mine apparentlyh has a different PCI ID:
lspci -nn -v -s 00:1f.2
00:1f.2 RAID bus controller [0104]: Intel Corporation Mobile 82801 SATA RAID Controller [8086:282a] (rev 03)
        Subsystem: Dell Device [1028:024f]
        Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 45
        I/O ports at 6e70 [size=8]
        I/O ports at 6e78 [size=4]
        I/O ports at 6e80 [size=8]
        I/O ports at 6e88 [size=4]
        I/O ports at 6ea0 [size=32]
        Memory at fed1c800 (32-bit, non-prefetchable) [size=2K]
        Capabilities: [80] MSI: Enable+ Count=1/16 Maskable- 64bit-
        Capabilities: [70] Power Management version 3
        Capabilities: [a8] SATA HBA v1.0
        Capabilities: [b0] PCI Advanced Features
        Kernel driver in use: ahci



from dmesg:
[    1.295050] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[    1.296037] ata1.00: ATA-8: WDC WD1600BJKT-75F4T0, 11.01A11, max UDMA/133
[    1.296041] ata1.00: 312581808 sectors, multi 0: LBA48 NCQ (depth 31/32), AA
[    1.297051] ata1.00: configured for UDMA/133
[    1.297773] scsi 0:0:0:0: Direct-Access     ATA      WDC WD1600BJKT-7 11.0 PQ: 0 ANSI: 5
[    1.298623] sd 0:0:0:0: [sda] 312581808 512-byte logical blocks: (160 GB/149 GiB)
[    1.298811] sd 0:0:0:0: [sda] Write Protect is off
[    1.298816] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
[    1.298891] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
[    1.329096]  sda: sda1 sda2
[    1.330321] sd 0:0:0:0: [sda] Attached SCSI disk
[    6.652045] ata2: link is slow to respond, please be patient (ready=0)
[   11.344043] ata2: COMRESET failed (errno=-16)
[   11.649049] ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
[   11.650654] ata2.00: ATAPI: MATSHITA DVD+/-RW UJ892, 1.01, max UDMA/100
[   11.653036] ata2.00: configured for UDMA/100
[   11.656392] scsi 1:0:0:0: CD-ROM            MATSHITA DVD+-RW UJ892    1.01 PQ: 0 ANSI: 5
[   11.659202] sr0: scsi3-mmc drive: 24x/24x writer dvd-ram cd/rw xa/form2 cdda tray
[   11.659206] cdrom: Uniform CD-ROM driver Revision: 3.20
[   11.659794] sr 1:0:0:0: Attached scsi CD-ROM sr0
[   11.965047] ata5: SATA link down (SStatus 0 SControl 300)
[   12.270047] ata6: SATA link down (SStatus 0 SControl 300)
Tejun Heo - Feb. 13, 2012, 5:44 p.m.
Hello,

On Mon, Feb 06, 2012 at 11:20:45AM -0800, Brian Norris wrote:
> I see the following options:
> (1) implement a flag that can be passed through ahci_platform; this
> would not be very useful, as we would still have to tweak the driver
> out of tree.

Yeah, please add module param to make this behavior conditional.

> (2) Drop the fix entirely. This is a spec. violation, but we can
> simply try to maintain the fix out-of-tree.

Nothing is perfect and real hardware should come before spec.

Thanks.
Brian Norris - Feb. 15, 2012, 5:15 a.m.
On Mon, Feb 13, 2012 at 9:44 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Mon, Feb 06, 2012 at 11:20:45AM -0800, Brian Norris wrote:
>> I see the following options:
>> (1) implement a flag that can be passed through ahci_platform; this
>> would not be very useful, as we would still have to tweak the driver
>> out of tree.
>
> Yeah, please add module param to make this behavior conditional.

Perhaps a module param (for ahci_platform) that sets a flag in
ata_port_info? I'm not sure if/how I'm allowed to introduce new ATA
flags...

>> (2) Drop the fix entirely. This is a spec. violation, but we can
>> simply try to maintain the fix out-of-tree.
>
> Nothing is perfect and real hardware should come before spec.

You have ignored my option (3): to fix the observed problems. I
already tested this last patch against the previous regression
reports, found here:
http://marc.info/?l=linux-ide&m=130529205513940&w=2

However, I had not noticed that the delayed DVD drive recognition was
a separate issue. Since then, I've noticed that I have tested the
exact same AHCI controller as Valdis' issue (a little different than
Norbert's) but the key difference is the MATSHITA DVD drive (both
Norbert and Valdis have MATSHITA). The same controller works with
non-MATSHITA DVD.

I also noticed that Mark Lord commented that he needed the patch in
question. Is this a different controller that needs my same fix?
http://marc.info/?l=linux-ide&m=131967244009803&w=2

So it appears that we are weighing the MATSHITA DVD issues against the
issues seen by me and possibly Mark Lord. If the decision really
stands that finding a unified solution is impossible, then I can just
drop the issue and make conditional behavior.

Brian
--
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
Tejun Heo - Feb. 15, 2012, 4:57 p.m.
Hello,

On Tue, Feb 14, 2012 at 09:15:32PM -0800, Brian Norris wrote:
> Perhaps a module param (for ahci_platform) that sets a flag in
> ata_port_info? I'm not sure if/how I'm allowed to introduce new ATA
> flags...

I think adding a module param directly to libahci.c should do.  Just
add it after ignore_sss and apply it to all ahci's on the host.

> So it appears that we are weighing the MATSHITA DVD issues against the
> issues seen by me and possibly Mark Lord. If the decision really
> stands that finding a unified solution is impossible, then I can just
> drop the issue and make conditional behavior.

My memory is already fuzzy but strict adherence to the spec didn't
make whole lot of sense to me.  I think I wrote several times on the
issue already.  Plus, we're talking about introducing regressions to
generic x86 setups against few specific platforms.  If somebody can
come up with generic solution, fine, but for now, let's just revert to
the original behavior, please.

Thanks.
Jeff Garzik - Feb. 15, 2012, 6:29 p.m.
On 02/15/2012 11:57 AM, Tejun Heo wrote:
> Hello,
>
> On Tue, Feb 14, 2012 at 09:15:32PM -0800, Brian Norris wrote:
>> Perhaps a module param (for ahci_platform) that sets a flag in
>> ata_port_info? I'm not sure if/how I'm allowed to introduce new ATA
>> flags...
>
> I think adding a module param directly to libahci.c should do.  Just
> add it after ignore_sss and apply it to all ahci's on the host.

A module parameter is not necessarily the best/only option. 
ahci_platform already has infrastructure set up to deal with 
platform-specific quirks.  An internal flag seems more appropriate to 
enable automatic handling of this on the specific platforms where it 
applies (plus the revert Tejun has already mentioned).

Nothing wrong with debugging the regression further (Brian's option #3), 
but in the meantime we need to be actively using the best known working 
state, which means making the "fix" opt-in rather than unconditional or 
opt-out.

	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
Tejun Heo - Feb. 15, 2012, 6:31 p.m.
Hey, Jeff.

On Wed, Feb 15, 2012 at 01:29:29PM -0500, Jeff Garzik wrote:
> A module parameter is not necessarily the best/only option.
> ahci_platform already has infrastructure set up to deal with
> platform-specific quirks.  An internal flag seems more appropriate
> to enable automatic handling of this on the specific platforms where
> it applies (plus the revert Tejun has already mentioned).

The problem is that there's no way to identify the controller in
question, so we can't do this automatically, so might just as well do
it in the simplest way for now.  :(

Thanks.
Jeff Garzik - Feb. 15, 2012, 7:18 p.m.
On 02/15/2012 01:31 PM, Tejun Heo wrote:
> Hey, Jeff.
>
> On Wed, Feb 15, 2012 at 01:29:29PM -0500, Jeff Garzik wrote:
>> A module parameter is not necessarily the best/only option.
>> ahci_platform already has infrastructure set up to deal with
>> platform-specific quirks.  An internal flag seems more appropriate
>> to enable automatic handling of this on the specific platforms where
>> it applies (plus the revert Tejun has already mentioned).
>
> The problem is that there's no way to identify the controller in
> question, so we can't do this automatically, so might just as well do
> it in the simplest way for now.  :(

See ahci_devtype[] and ahci_port_info[] in ahci_platform.c for how to do 
this.  Brian would not have to tweak the driver out of tree as claimed; 
we put all changes in tree, and the platform calls itself 
"spec-strict-ahci" or whatever string you prefer.

	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
Brian Norris - Feb. 15, 2012, 11:39 p.m.
On Wed, Feb 15, 2012 at 11:18 AM, Jeff Garzik <jeff@garzik.org> wrote:
> On 02/15/2012 01:31 PM, Tejun Heo wrote:
>> The problem is that there's no way to identify the controller in
>> question, so we can't do this automatically, so might just as well do
>> it in the simplest way for now.  :(
>
>
> See ahci_devtype[] and ahci_port_info[] in ahci_platform.c for how to do
> this.  Brian would not have to tweak the driver out of tree as claimed; we
> put all changes in tree, and the platform calls itself "spec-strict-ahci" or
> whatever string you prefer.

OK, so can it seems I would add a flag in include/linux/libata.h that
can be added to a new entry in ahci_port_info[]? It seems a little
awkward to put in "libata.h", plus I'm not sure if all the flags have
direct meaning to ATA or if they can be AHCI-specific.

Brian
--
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
Mark Lord - Feb. 16, 2012, 2:20 p.m.
On 12-02-15 01:31 PM, Tejun Heo wrote:
> Hey, Jeff.
> 
> On Wed, Feb 15, 2012 at 01:29:29PM -0500, Jeff Garzik wrote:
>> A module parameter is not necessarily the best/only option.
>> ahci_platform already has infrastructure set up to deal with
>> platform-specific quirks.  An internal flag seems more appropriate
>> to enable automatic handling of this on the specific platforms where
>> it applies (plus the revert Tejun has already mentioned).
> 
> The problem is that there's no way to identify the controller in
> question, so we can't do this automatically, so might just as well do
> it in the simplest way for now.  :(

Well, a module parameter is no good,
because that method would affect all attached controllers
rather than just the one(s) with the issue.

Something in sysfs perhaps.
--
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 - Feb. 16, 2012, 4:19 p.m.
On 02/16/2012 09:20 AM, Mark Lord wrote:
> On 12-02-15 01:31 PM, Tejun Heo wrote:
>> Hey, Jeff.
>>
>> On Wed, Feb 15, 2012 at 01:29:29PM -0500, Jeff Garzik wrote:
>>> A module parameter is not necessarily the best/only option.
>>> ahci_platform already has infrastructure set up to deal with
>>> platform-specific quirks.  An internal flag seems more appropriate
>>> to enable automatic handling of this on the specific platforms where
>>> it applies (plus the revert Tejun has already mentioned).
>>
>> The problem is that there's no way to identify the controller in
>> question, so we can't do this automatically, so might just as well do
>> it in the simplest way for now.  :(
>
> Well, a module parameter is no good,
> because that method would affect all attached controllers
> rather than just the one(s) with the issue.
>
> Something in sysfs perhaps.

See the method already described in my previous message.
grep for IMX53_AHCI.

	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 - Feb. 16, 2012, 4:22 p.m.
On 02/15/2012 06:39 PM, Brian Norris wrote:
> On Wed, Feb 15, 2012 at 11:18 AM, Jeff Garzik<jeff@garzik.org>  wrote:
>> On 02/15/2012 01:31 PM, Tejun Heo wrote:
>>> The problem is that there's no way to identify the controller in
>>> question, so we can't do this automatically, so might just as well do
>>> it in the simplest way for now.  :(
>>
>>
>> See ahci_devtype[] and ahci_port_info[] in ahci_platform.c for how to do
>> this.  Brian would not have to tweak the driver out of tree as claimed; we
>> put all changes in tree, and the platform calls itself "spec-strict-ahci" or
>> whatever string you prefer.
>
> OK, so can it seems I would add a flag in include/linux/libata.h that
> can be added to a new entry in ahci_port_info[]? It seems a little
> awkward to put in "libata.h", plus I'm not sure if all the flags have
> direct meaning to ATA or if they can be AHCI-specific.

Nothing needs to go into libata.h.  grep for IMX53_AHCI, and see how 
that differentiates ata_port_info information.  There you may vary 
AHCI-specific flags (defined in ahci.h), port operations, or anything 
else specific to the controller.

Create a flag, add it to ahci.h, guard the spec-strict behavior creating 
problems with said flag, and apply said flag to your controller in 
ahci_platform.

	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/libahci.c b/drivers/ata/libahci.c
index a72bfd0..33f7333 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -746,6 +746,9 @@  static void ahci_start_port(struct ata_port *ap)
 	/* enable FIS reception */
 	ahci_start_fis_rx(ap);
 
+	/* enable DMA */
+	ahci_start_engine(ap);
+
 	/* turn on LEDs */
 	if (ap->flags & ATA_FLAG_EM) {
 		ata_for_each_link(link, ap, EDGE) {