linux-kernel - Re: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up after link

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: <205e0fd9995a4f2b80ab370a974b6628@ausx13mps321.AMER.DELL.COM>
Date:   Wed, 13 Feb 2019 18:55:46 +0000
From:   <Alex_Gagniuc@...lteam.com>
To:     <lukas@...ner.de>
Cc:     <mr.nuke.me@...il.com>, <bhelgaas@...gle.com>,
        <Austin.Bolen@...l.com>, <keith.busch@...el.com>,
        <Shyam.Iyer@...l.com>, <gustavo@...eddedor.com>,
        <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up
 after link

On 2/13/19 2:36 AM, Lukas Wunner wrote:
>> (*) A bit hypothetical: There is no hardware yet implementing the ECN.
> 
> Hm, this contradicts Austin Bolen's e-mail of Jan 25 that "Yes, this
> platform disables in-band presence" (when asked whether your host
> controller already adheres to the ECN).

Both statements are true. The hardware does indeed disable in-band 
presence, in a rudimentary way that is not compliant with the ECN -- it 
doesn't implement the bits required by the ECN.


>> I'm
>> not sure that there is a spec-justifiable reason to not access a device
>> whose DLL is up, but PD isn't.
> 
> Austin asked in an e-mail of Jan 24 whether "the hot-inserted device's
> config space [is] accessed immediately after waiting this 20 + 100 ms
> delay", which sounded to me like you'd prefer the device not to be
> accessed until PDS is 1.

"Unless Readiness Notifications mechanisms are used (see Section 6.23), 
the Root Complex and/or system software must allow at least 1.0 s after 
a Conventional Reset of a device, before it may determine that a device 
which fails to return a Successful Completion status for a valid 
Configuration Request is a broken device. This period is independent of 
how quickly Link training completes."
	(Section 6.6)

The concern was the one second delay, which is addressed by the use of 
pci_bus_wait_crs().

>>> Be mindful however that pcie_wait_for_link() is also called from the
>>> DPC driver.  Keith should be able to judge whether a change to that
>>> function breaks DPC.
>>
>> That's why I went for ammending pciehp_handle_presence_or_link_change().
>> Smaller bug surface ;). What I'm thinking at this point is, keep the
>> patch as is, but, also check for in-band PD disable before aborting the
>> shutdown. Old behavior stays the same.
> 
> I'm worried that amending pciehp_handle_presence_or_link_change() makes
> the event handling logic more difficult to understand.

Don't worry. It's already difficult to understand ;)

>  Polling PDS in
> pcie_wait_for_link() or disabling either PDC or DLLSC if in-band presence
> is disabled seems simpler to reason about.

pcie_wait_for_link() is generic PCIe layer. I don't think mixing hotplug 
concepts is a good layering violation.

Disabling PDC or DLLSC can work. I've sometimes wondered why we even 
care about PDC. I can imagine situations where platform might want to 
signal imminent removal by yanking PD.

>> in-band PD disable (what's a good acronym for that, BTW?)
> 
> I don't know, maybe inband_presence_disabled?

PCI_EXP_SLTCAP2_IBPD ?