Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Cc:     <mr.nuke.me@gmail.com>, <bhelgaas@google.com>,
        <Austin.Bolen@dell.com>, <keith.busch@intel.com>,
        <Shyam.Iyer@dell.com>, <gustavo@embeddedor.com>,
        <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>
From:   <Alex_Gagniuc@Dellteam.com>
To:     <lukas@wunner.de>
Subject: Re: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up
 after link
Thread-Topic: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up
 after link
Thread-Index: AQHUvZbN4kEiyNA8xUGy+Uw4G87SXw==
Date:   Wed, 13 Feb 2019 18:55:46 +0000
Message-ID: <205e0fd9995a4f2b80ab370a974b6628@ausx13mps321.AMER.DELL.COM>
References: <20190205210701.25387-1-mr.nuke.me@gmail.com>
 <20190209115849.244u67h7wmn3eb7o@wunner.de>
 <52afa1c45f684dbda6a8771b65583a22@ausx13mps321.AMER.DELL.COM>
 <20190212083031.2no7mzn5xug7nba3@wunner.de>
 <b2fffa5beb7941ebaceb8b8ce6e5b133@ausx13mps321.AMER.DELL.COM>
 <20190213083632.GA3387@wunner.de>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2/13/19 2:36 AM, Lukas Wunner wrote:=0A=
>> (*) A bit hypothetical: There is no hardware yet implementing the ECN.=
=0A=
> =0A=
> Hm, this contradicts Austin Bolen's e-mail of Jan 25 that "Yes, this=0A=
> platform disables in-band presence" (when asked whether your host=0A=
> controller already adheres to the ECN).=0A=
=0A=
Both statements are true. The hardware does indeed disable in-band =0A=
presence, in a rudimentary way that is not compliant with the ECN -- it =0A=
doesn't implement the bits required by the ECN.=0A=
=0A=
=0A=
>> I'm=0A=
>> not sure that there is a spec-justifiable reason to not access a device=
=0A=
>> whose DLL is up, but PD isn't.=0A=
> =0A=
> Austin asked in an e-mail of Jan 24 whether "the hot-inserted device's=0A=
> config space [is] accessed immediately after waiting this 20 + 100 ms=0A=
> delay", which sounded to me like you'd prefer the device not to be=0A=
> accessed until PDS is 1.=0A=
=0A=
"Unless Readiness Notifications mechanisms are used (see Section 6.23), =0A=
the Root Complex and/or system software must allow at least 1.0 s after =0A=
a Conventional Reset of a device, before it may determine that a device =0A=
which fails to return a Successful Completion status for a valid =0A=
Configuration Request is a broken device. This period is independent of =0A=
how quickly Link training completes."=0A=
	(Section 6.6)=0A=
=0A=
The concern was the one second delay, which is addressed by the use of =0A=
pci_bus_wait_crs().=0A=
=0A=
>>> Be mindful however that pcie_wait_for_link() is also called from the=0A=
>>> DPC driver.  Keith should be able to judge whether a change to that=0A=
>>> function breaks DPC.=0A=
>>=0A=
>> That's why I went for ammending pciehp_handle_presence_or_link_change().=
=0A=
>> Smaller bug surface ;). What I'm thinking at this point is, keep the=0A=
>> patch as is, but, also check for in-band PD disable before aborting the=
=0A=
>> shutdown. Old behavior stays the same.=0A=
> =0A=
> I'm worried that amending pciehp_handle_presence_or_link_change() makes=
=0A=
> the event handling logic more difficult to understand.=0A=
=0A=
Don't worry. It's already difficult to understand ;)=0A=
=0A=
>  Polling PDS in=0A=
> pcie_wait_for_link() or disabling either PDC or DLLSC if in-band presence=
=0A=
> is disabled seems simpler to reason about.=0A=
=0A=
pcie_wait_for_link() is generic PCIe layer. I don't think mixing hotplug =
=0A=
concepts is a good layering violation.=0A=
=0A=
Disabling PDC or DLLSC can work. I've sometimes wondered why we even =0A=
care about PDC. I can imagine situations where platform might want to =0A=
signal imminent removal by yanking PD.=0A=
=0A=
>> in-band PD disable (what's a good acronym for that, BTW?)=0A=
> =0A=
> I don't know, maybe inband_presence_disabled?=0A=
=0A=
PCI_EXP_SLTCAP2_IBPD ?=0A=