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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f6399f3a-3899-2663-6667-e967eb43b156@silicom-usa.com>
Date:   Mon, 19 Aug 2019 16:30:41 +0000
From:   Stephen Douthit <stephend@...icom-usa.com>
To:     Dan Williams <dan.j.williams@...el.com>
CC:     Christoph Hellwig <hch@...radead.org>,
        Jens Axboe <axboe@...nel.dk>,
        "linux-ide@...r.kernel.org" <linux-ide@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device
 ID

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.

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