libata-acpi: summary, problems, questions and proposal

libata-acpi: summary, problems, questions and proposal

[Tejun Heo]

Hello, ATA and ACPI crowd.

This mail tries to summarize the current state of libata ACPI support
and establish consensus how it should be improved.  If you're familiar
with ACPI or the current libata-acpi implementation, please try to
answer questions in section 3.

Table of Contents

  1. Summary of the current libata ACPI support
  2. Problems of the current implementation
  3. Questions regarding ATA ACPI spec and the current implementation
  4. Proposal for improvements


1. Summary of the current libata ACPI support

Currently there are two branches of acpi support.  One in git tree
libata-dev (also merged into mainline) the other in -mm tree.  The one
in -mm tree contains additional _GTM/_STM support and pata_acpi.c
implementation by Alan Cox.  The additional part currently only in the
-mm tree is necessary and needs to be merged but there is a
disagreement regarding how pata_acpi should be done.

ACPI support implementation in libata-dev supports both IDE and SATA
ACPI object layouts and subset of ATA ACPI methods - _SDD and _GTF.
It incorrectly uses ap->cbl (the port's cable type) to choose between
the two ACPI layouts.  Association between the host and its ACPI
object is performed every time ACPI methods are invoked but the
association between an ATA device and its ACPI object is cached in
ata_device object.

If ap->cbl indicates SATA cable type, _SDD is invoked every time the
associated ATA device is configured (after reset and during
revalidation).  Also, _GTF and the resulting TFs are executed right
after each _SDD.

If ap->cbl does not indicate SATA cable type, ACPI support in
libata-dev currently doesn't do anything because in some cases _GTF
depends on _STM being called before it and executing _GTF before _STM
causes errors in ACPI layer.  As _GTM/_STM support in -mm is only used
with pata_acpi yet, the situation is similar there too.  Nothing is
done if cable type isn't SATA.


2. Problems of the current implementation

2-1. Associating ATA host/device with their ACPI objects is broken.
     For one, it uses ap->cbl to determine which layout to use but
     this isn't dependent on cable type.  This is dependent on the
     programming interface of the controller.  If it implements or
     emulates SFF IDE interface, the IDE ACPI layout should be used.
     If the controller implements its own native SATA aware
     programming interface such as ahci or sil24, it should use SATA
     native layout.

2-2. Missing proper _GTM/_STM support.  As stated above, although -mm
     contains _GTM/_STM support, it does not hook it to regular
     exception handling path and thus _GTF cannot be used in a lot of
     cases.

2-3. Misplaced _GTF hook.  _GTF currently is called prior to every
     device configuration.  This is unnecessary and incorrect.  The
     ACPI spec specifies that _GTM/_STM and _GTF should be executed
     during suspend/resume cycles not on every reset or
     reconfiguration.  This, for example, causes the following
     problem.

     _STM should be called before _GTF and, in turn, _GTF should be
     called before _STM, so, during initial detection, we should be
     doing _GTM -> _STM -> _GTF sequence.


3. Questions regarding ATA ACPI spec and the current implementation

Please feel free to point out errors and it would be great if you can
include or point to example .dat/.dsl files when answering.

3-1. In all ACPI specs form 1.0 to 3.0a, the example IDE hierarchy
     looks like the following.

     \_SB\PCI0\IDE1\{_ADR,_GTM,_STM,_PR0}
                   \DRV1\{_ADR,_GTF}
                   \DRV2\{_ADR,_GTF}
              \IDE2\{_ADR,_GTM,_STM,_PR0}
                   \DRV1\{_ADR,_GTF}
                   \DRV2\{_ADR,_GTF}

     Where the description of IDEx\_ADR is "Indicates address of the
     channel on the PCI bus".  But actual IDE ACPI hierarchy looks
     like the following.

     \_SB\PCI0\PCIDEVX\_ADR
                      \IDE1\{_ADR,_GTM,_STM,_PR0}
                           \DRV1\{_ADR,_GTF}
                           \DRV2\{_ADR,_GTF}
                      \IDE2\{_ADR,_GTM,_STM,_PR0}
                           \DRV1\{_ADR,_GTF}
                           \DRV2\{_ADR,_GTF}

     Where PCIDEVX\_ADR indicates the address of PCI device on the PCI
     bus while IDEx\_ADR indicates the channel number on the PCI
     device not the PCI bus.  Is this a mistake in the spec or am I
     misreading it?

3-2. sata_get_dev_handle() looks for the ACPI object associated with
     the PCI host of the given ATA host by matching the PCI device's
     _ADR in the containing PCI bus, but pata_get_dev_handle() gets
     the ACPI device handle for the PCI device by simply invoking
     DEVICE_ACPI_HANDLE() on the generic device for the host device
     and verifies the ACPI object for the containing bus has the
     matching bus number in its _ADR.

     a. Why can't we just use DEVICE_ACPI_HANDLE() on host->dev in
        SATA case?  What's the difference between IDE and SATA ACPI
        objects?  AFAICS, they are the same PCI device object.

     b. In IDE case, why verify the parent object has matching BUS ID?
        When can it have mismatching one?  That means
        DEVICE_ACPI_HANDLE() may return the wrong acpi_handle for the
        given device.  Shouldn't we be fixing DEVICE_ACPI_HANDLE()
        instead?  Or is IDE ACPI object somehow special?

3-3. When associating an ata_device with its ACPI counterpart, IDE and
     SATA do things differently.  If IDE, it first looks for the ACPI
     object representing the channel under the ACPI object for the
     host by matching _ADR against the port number.  If the channel
     object is found, ACPI object with matching drive number in _ADR
     is looked up under the ACPI object.  This object is the ACPI
     object for the ATA device.

     For SATA, depth first walk is performed below the ACPI object
     representing the host.  Each object under the host object is
     examined for matching _ADR.

     Why can't we just look for matching _ADR below the ACPI object
     for the host PCI device?  The SATA hierarchy has been introduced
     only in ACPI3.0 and I don't see any reason why the recursive walk
     should be done in either 3.0 or 3.0a.  Any special reason?


4. Proposal for improvements

4-1. Depending on how questions in section 3 are answered, fix and
     clean up ATA host/device <-> ACPI object association.  Whether
     IDE or SATA native style hierarchy is used should be determined
     by driver flag not cable type.  e.g. ahci and sata_sil24 should
     use SATA native style hierarchy while ata_piix should use IDE
     hierarchy whether the port is SATA or PATA.

     On controllers which support both modes are supported (many
     BIOSen allow switching between ata_piix and ahci modes), it isn't
     very clear because in many such cases ACPI tree doesn't change
     depending on configuration.  ie. you get the same IDE hierarchy
     whether you're in ata_piix mode or ahci mode.

     We can try to fall back mechanism but that isn't gonna cover all
     cases either - e.g. in ICH8, the same ports use one PCI function
     in AHCI mode while splits into two functions in ata_piix mode.
     It can also be dangerous as IDE _STM/_GTM might touch disabled
     IDE registers when the controller is in AHCI mode.  So, IMHO, we
     shouldn't bother.  Most notebooks BIOSen which implement ATA
     password protection seem to limit the controller into one
     specific mode anyway.

4-2. Only associate once during initialization.  There is no reason to
     try to associate hosts and devices with ACPI objects at each try.
     Do it once during host initialization and use it if available or
     forget about ACPI if not available.

4-3. Integrate _GTM/_STM support and invoke methods only when the spec
     specifies to.  For IDE object, do _GTM during suspend and _STM
     followed by _GTF during resume.  There is no reason to call them
     anytime else.  For SATA native object, do _SDD followed by _GTF
     after every hardreset.

4-4. Implement helpers for cable detection using _GTM/_STM and use it
     in sata_nv if CK804.  This is to substitute independent pata_acpi
     driver.  Low level driver should know when _GTM/_STM should be
     used for cable detection and/or device programming and doing it
     this way reduces user confusion (sata_nv also supports ck804 but
     you probably need to load pata_acpi if ACPI is available) and
     allows better integration with the rest of the low level driver
     (e.g. ADMA mode + _GTM/_STM cable detection).

For 4-3 and 4-4, most code is already there in -mm tree.  Shamelessly
stealing and reorganizing Alan's code should do it.  :-)

Thanks.

--
tejun

Re: libata-acpi: summary, problems, questions and proposal

[Matthew Garrett]

On Wed, Mar 28, 2007 at 04:30:02PM +0900, Tejun Heo wrote:

Hi Tejun,

Firstly, could I ask you to take a look at the patch in 
http://permalink.gmane.org/gmane.linux.acpi.devel/22066/ ? It deals with 
some of these issues.

> ACPI support implementation in libata-dev supports both IDE and SATA
> ACPI object layouts and subset of ATA ACPI methods - _SDD and _GTF.
> It incorrectly uses ap->cbl (the port's cable type) to choose between
> the two ACPI layouts.  Association between the host and its ACPI
> object is performed every time ACPI methods are invoked but the
> association between an ATA device and its ACPI object is cached in
> ata_device object.

These issues are both fixed in my patch, I believe.

> 2-2. Missing proper _GTM/_STM support.  As stated above, although -mm
>     contains _GTM/_STM support, it does not hook it to regular
>     exception handling path and thus _GTF cannot be used in a lot of
>     cases.

I've added _GTM and _STM support over suspend/resume. Right now they're 
in the host power management code - I'm not sure whether they should be 
here or the SCSI glue layer?

> 2-3. Misplaced _GTF hook.  _GTF currently is called prior to every
>     device configuration.  This is unnecessary and incorrect.  The
>     ACPI spec specifies that _GTM/_STM and _GTF should be executed
>     during suspend/resume cycles not on every reset or
>     reconfiguration.  This, for example, causes the following
>     problem.

That should be quite easily fixable with the above patch.

> 4-1. Depending on how questions in section 3 are answered, fix and
>     clean up ATA host/device <-> ACPI object association.  Whether
>     IDE or SATA native style hierarchy is used should be determined
>     by driver flag not cable type.  e.g. ahci and sata_sil24 should
>     use SATA native style hierarchy while ata_piix should use IDE
>     hierarchy whether the port is SATA or PATA.

I think this is just a matter of making sure that the sata and pata 
handle matching code matches reality now :)

> 4-2. Only associate once during initialization.  There is no reason to
>     try to associate hosts and devices with ACPI objects at each try.
>     Do it once during host initialization and use it if available or
>     forget about ACPI if not available.

Fixed.

> 4-3. Integrate _GTM/_STM support and invoke methods only when the spec
>     specifies to.  For IDE object, do _GTM during suspend and _STM
>     followed by _GTF during resume.  There is no reason to call them
>     anytime else.  For SATA native object, do _SDD followed by _GTF
>     after every hardreset.

Patrially fixed.

> 4-4. Implement helpers for cable detection using _GTM/_STM and use it
>     in sata_nv if CK804.  This is to substitute independent pata_acpi
>     driver.  Low level driver should know when _GTM/_STM should be
>     used for cable detection and/or device programming and doing it
>     this way reduces user confusion (sata_nv also supports ck804 but
>     you probably need to load pata_acpi if ACPI is available) and
>     allows better integration with the rest of the low level driver
>     (e.g. ADMA mode + _GTM/_STM cable detection).

Not done.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: libata-acpi: summary, problems, questions and proposal

[Tejun Heo]

Hello, Matthew.

Matthew Garrett wrote:
> On Wed, Mar 28, 2007 at 04:30:02PM +0900, Tejun Heo wrote:
> 
> Hi Tejun,
> 
> Firstly, could I ask you to take a look at the patch in 
> http://permalink.gmane.org/gmane.linux.acpi.devel/22066/ ? It deals with 
> some of these issues.

Yeap, I've seen the patch.  That's why you're on the cc list in the 
first place.  :-)

>> ACPI support implementation in libata-dev supports both IDE and SATA
>> ACPI object layouts and subset of ATA ACPI methods - _SDD and _GTF.
>> It incorrectly uses ap->cbl (the port's cable type) to choose between
>> the two ACPI layouts.  Association between the host and its ACPI
>> object is performed every time ACPI methods are invoked but the
>> association between an ATA device and its ACPI object is cached in
>> ata_device object.
> 
> These issues are both fixed in my patch, I believe.

Yeap, I think it's in the right direction but we need to go further.

* I'm not sure whether the complex walk libata-acpi is doing is justifiable.

* You'll end up doing _STM/_GTM on ahci controller on some BIOSen - 1. 
it can be dangerous 2. you might get partial or incorrect mapping - 
think about ICH8-split-to-two-PCI-fn-in-piix-mode case.

>> 2-2. Missing proper _GTM/_STM support.  As stated above, although -mm
>>     contains _GTM/_STM support, it does not hook it to regular
>>     exception handling path and thus _GTF cannot be used in a lot of
>>     cases.
> 
> I've added _GTM and _STM support over suspend/resume. Right now they're 
> in the host power management code - I'm not sure whether they should be 
> here or the SCSI glue layer?

I think PM functions in libata-eh is better place and you also need to 
do _GTF after _GTM during resume.

>> 2-3. Misplaced _GTF hook.  _GTF currently is called prior to every
>>     device configuration.  This is unnecessary and incorrect.  The
>>     ACPI spec specifies that _GTM/_STM and _GTF should be executed
>>     during suspend/resume cycles not on every reset or
>>     reconfiguration.  This, for example, causes the following
>>     problem.
> 
> That should be quite easily fixable with the above patch.

Yeap.

>> 4-1. Depending on how questions in section 3 are answered, fix and
>>     clean up ATA host/device <-> ACPI object association.  Whether
>>     IDE or SATA native style hierarchy is used should be determined
>>     by driver flag not cable type.  e.g. ahci and sata_sil24 should
>>     use SATA native style hierarchy while ata_piix should use IDE
>>     hierarchy whether the port is SATA or PATA.
> 
> I think this is just a matter of making sure that the sata and pata 
> handle matching code matches reality now :)

Currently 1/2 of libata-acpi code is dealing with the above.  I'm trying 
to figure out why it needs to be that complex.

Anyways, I think your patch is a step in the right direction, so 
depending on how ACPI gurus enlighten us here, we can base further fix 
on your patch.  Let's see how the questions are answered.

Thanks.

-- 
tejun

Re: libata-acpi: summary, problems, questions and proposal

[Matthew Garrett]

On Thu, Mar 29, 2007 at 10:42:03AM +0900, Tejun Heo wrote:

> Yeap, I think it's in the right direction but we need to go further.
> 
> * I'm not sure whether the complex walk libata-acpi is doing is justifiable.

Yeah, that may well be less than ideal.

> * You'll end up doing _STM/_GTM on ahci controller on some BIOSen - 1. 
> it can be dangerous 2. you might get partial or incorrect mapping - 
> think about ICH8-split-to-two-PCI-fn-in-piix-mode case.

So far I haven't seen any DSDTs that include _GTM and _STM methods for 
controllers that come up flagged as ahci, though I'm perfectly willing 
to believe that they're out there...

> I think PM functions in libata-eh is better place and you also need to 
> do _GTF after _GTM during resume.

Well, "need" - I haven't actually found hardware that seems upset about 
the missing _GTF call :) But you're right, it ought to be done. Can you 
point me at the right bits of the libata-eh code? I was trying to work 
through it earlier, but keeping track of the conditional paths is a bit 
tricky.

> >I think this is just a matter of making sure that the sata and pata 
> >handle matching code matches reality now :)
> 
> Currently 1/2 of libata-acpi code is dealing with the above.  I'm trying 
> to figure out why it needs to be that complex.

I wrote a patch at one point that simplified this to an extent 
(http://lkml.org/lkml/2005/12/7/425), but it got somewhat bogged down in 
discussion about where the right place to do the binding was. Personally 
I'd prefer to handle this in a similar way to PCI - that is, register 
the ata bus with ACPI and then find handles as and when new ata devices 
are added to that bus. This has the advantage that it's easy to tie ACPI 
events to specific ata devices, which could then be integrated with the 
bay and dock drivers.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: libata-acpi: summary, problems, questions and proposal

[Tejun Heo]

Matthew Garrett wrote:
>> * You'll end up doing _STM/_GTM on ahci controller on some BIOSen - 1. 
>> it can be dangerous 2. you might get partial or incorrect mapping - 
>> think about ICH8-split-to-two-PCI-fn-in-piix-mode case.
> 
> So far I haven't seen any DSDTs that include _GTM and _STM methods for 
> controllers that come up flagged as ahci, though I'm perfectly willing 
> to believe that they're out there...

My ICH8 board seems to.  I'll further look into it and post dsl files.

>> I think PM functions in libata-eh is better place and you also need to 
>> do _GTF after _GTM during resume.
> 
> Well, "need" - I haven't actually found hardware that seems upset about 
> the missing _GTF call :)

_GTF does the password unlocking and device configuration part of _STM.
 ACPI itself might not whine because no other ACPI method depends on
_GTF being executed but functionally it's the most important piece.

Now I think about it, _GTF is eventually done during device revalidation
but that needs to be removed, so we'll need to add it in the resume path.

> But you're right, it ought to be done. Can you 
> point me at the right bits of the libata-eh code? I was trying to work 
> through it earlier, but keeping track of the conditional paths is a bit 
> tricky.

There is a big suspend/resume rewrite patch pending and the
suspend/resume code will look quiet different (simpler) after it.

I think the correct place for _STM/_GTF would be right after
ata_eh_revalidate_and_attach().  As libata allows device hotplugging
while suspended, we probably need to skip _STM/_GTF if the attached
device isn't the one we've seen during suspending.

Another problem is that _GTF might alter device size (number of blocks).
 Currently libata uses device size as one of the metrics to determine
whether the device is the same one it saw last time during revalidation.
 This is pretty good safeguard as IIRC in some rare cases BIOS not only
cuts sectors at the end of the disk but also offsets all the blocks (was
this even standard?  it can't be done using overlay or hpa.  some vendor
extension maybe?).  So, if the different disk size is due to offset and
you continue to operate on the disk as before, you're seriously screwed.

We need to make sure that the device is the same one that we saw during
suspend before doing _STM/_GTF and before _GTF the disk size might
differ.  Recent HPA handling Alan posted should be considered here too.
 Also, HPA programming might interact with _GTF.

>>> I think this is just a matter of making sure that the sata and pata 
>>> handle matching code matches reality now :)
>> Currently 1/2 of libata-acpi code is dealing with the above.  I'm trying 
>> to figure out why it needs to be that complex.
> 
> I wrote a patch at one point that simplified this to an extent 
> (http://lkml.org/lkml/2005/12/7/425), but it got somewhat bogged down in 
> discussion about where the right place to do the binding was. Personally 
> I'd prefer to handle this in a similar way to PCI - that is, register 
> the ata bus with ACPI and then find handles as and when new ata devices 
> are added to that bus. This has the advantage that it's easy to tie ACPI 
> events to specific ata devices, which could then be integrated with the 
> bay and dock drivers.

Yeap, that would be great.  The problem is that libata isn't struct
device'd (yet).  As libata needs sysfs representation anyway, we might
go all the way and implement ATA bus and all.  Duplication between SCSI
and ATA nodes is worrying but I guess we can figure out something.

Thanks.

-- 
tejun