linux-kernel - Re: [PATCH] nvme-pci: Force NVME_QUIRK_SIMPLE

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: <0fac5de3-3f35-4fc2-bbdc-411dc1018a85@oss.qualcomm.com>
Date: Fri, 25 Oct 2024 17:30:05 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Christoph Hellwig <hch@....de>, Konrad Dybcio <konradybcio@...nel.org>
Cc: Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...nel.dk>,
        Sagi Grimberg <sagi@...mberg.me>,
        Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nvme-pci: Force NVME_QUIRK_SIMPLE_SUSPEND on Qualcomm
 hosts

On 25.10.2024 1:35 PM, Christoph Hellwig wrote:
> On Thu, Oct 24, 2024 at 07:33:07PM +0200, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
>>
>> The Qualcomm SC8280XP SoC requires that all PCIe hosts are powered down
>> before the platform can reach S3-like sleep states. This is very much
>> similar in nature to the issue described in [1].
>>
>> Other Qualcomm platforms we support upstream require more complex code
>> additions across both the PCIe RC driver and other platform-specific
>> ones, before the link can be sustained in suspend. Hence, they
>> effectively need the same treatment for now.
>>
>> Force NVME_QUIRK_SIMPLE_SUSPEND on all Qualcomm platforms (as
>> identified by the upstream bridge having a Qualcomm VID) to address
>> that. Once the aforementioned issues on non-SC8280XP platforms are
>> addressed, the condition will be made more specific, with a PID check
>> limiting it to only the platform(s) that require it due to HW design.
> 
> The NVMe driver is the wrong place for this, it needs to happen in the
> core by making acpi_storage_d3() evaluate to true.  Preferably by
> actually setting the right ACPI attributes because a check for 
> PCI vendor ID absolutely will never do the right thing in the long run.

(Un?)fortunately, said platforms use FDT, so we can't fix that in ACPI.

We also considered a DT property to indicate this, but:

a) PCIe devices are discoverable and it's really really really
   discouraged to hardcode devices that are likely to be present
   on the bus (and this wouldn't work if a NVMe device showed up
   on a different-than-usual RC)

b) Adding such a property to the PCIe host node sounds a bit
   saner, but the NVMe code isn't aware of the RC. We could add
   something like:


struct pci_bus *pbus = pdev->bus;

while (!(pci_is_root_bus(pbus)))
	pbus = pbus->parent;

if (of_property_present(pbus->dev.of_node, "broken-sleep-foo-bar"))
	return NVME_QUIRK_SIMPLE_SUSPEND;


..but that implies we have to set that quirk in DT on all platforms
which only require an equivalent workaround temporarily. That in turn
is later much harder to undo than a simple driver change (e.g. if your
FDT is provided by the firmware).

In general, I don't think we can at all rely on firmware updates for
devices that are already on the market.

Konrad