libata-acpi: summary, problems, questions and proposal

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

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.

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

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?

That should be quite easily fixable with the above patch.

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

Fixed.

Patrially fixed.

Not done.

Matthew Garrett | [email protected]

Hello, Matthew.

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

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.

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

Yeap.

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

Yeah, that may well be less than ideal.

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...

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 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 | [email protected]

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

_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.

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.

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