Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID

[Dan Williams]

On Mon, Aug 19, 2019 at 9:30 AM Stephen Douthit
<stephend@xxxxxxxxxxxxxxx> wrote:
>
> On 8/14/19 1:17 PM, Dan Williams wrote:
> >> Can you get someone from the controller design team to give us a clear
> >> answer on a revision where this PCS change happened?
> >>
> >> It would be nice if we could just check PCI_REVISION_ID or something
> >> similar.
> >
> > I don't think such a reliable association with rev-id exists, the> intent was that the OS need never consider PCS.
>
> Can you please ask to confirm?  It would be a much simpler check if it
> is possible.

No. Even if it were accidentally the case today the Linux driver can't
trust that rev-id across the different implementations will be
maintained for this purpose because the OS driver is not meant to
touch this register. Just look at a sampling of rev-id from a few
different systems, and note that rev-id applies to the chipset not
just the ahci controller.

    rev 08
    rev 11
    rev 31

...which one of those is Denverton?

The intent is that PCS is a platform-firmware concern and that any
software that cares about PCS is caring about it by explicit
identification.

> > The way Linux is using
> > it is already broken with the assumption that it is performed after
> > every HOST_CTL based reset which only resets mmio space. At most it
> > should only be required at initial PCI discovery iff platform firmware
> > failed to run.
>
> This is a separate issue.
>
> It's broken in the sense that the code is called more often that it
> needs to be, but reset isn't a hot path, and there are no side effects
> to doing this multiple times that I can see.

The problem is that there is no known need to do it for Denverton, and
likely more platform implementations.

>  And as you point out, no
> bug reports, so pretty benign all things considered.
>
> We could move the PCS quirk code to ahci_init_one() to address this
> concern once there's agreement on what the criteria is to run/not-run
> this code.
>
> > There are no bug reports with the current
> > implementation that only attempts to enable bits based on PORTS_IMPL,
> > so I think we are firmer ground trying to draw a line where the driver
> > just stops worrying about PCS rather than try to detect the layout.
>
> Someone at Intel is going to need to decide where/how to draw this line.

This is a case of Linux touching a "BIOS only" register and assuming
that the quirk is widely applicable. I think a reasonable fix is to
just whitelist all the known Intel ids, apply the PCS fixup assuming
the legacy configuration register location, and stop applying the
quirk by default.

Here is a proposed patch along these lines. I can send a
non-whitespace damaged version if this approach looks acceptable:

---