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:   Tue, 12 Feb 2019 23:57:55 +0000
Message-ID: <b2fffa5beb7941ebaceb8b8ce6e5b133@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>
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/12/19 2:30 AM, Lukas Wunner wrote:=0A=
> The PCI SIG=0A=
> should probably consider granting access to specs to open source=0A=
> developers to ease adoption of new features in Linux et al.=0A=
=0A=
Don't get me started on just how ridiculous I think PCI-SIG's policy is =0A=
with respect to public availability of standards.=0A=
=0A=
>>> Table 4-14 in sec 4.2.6 is also important because it shows the differen=
ce=0A=
>>> between Link Up and in-band presence.=0A=
>>=0A=
>> So, we'd expect PD to come up at the same time or before DLL?=0A=
> =0A=
> Per table 4-14 and figure 4-23 (immediately below the table) in r4.0,=0A=
> PDC ought to be signaled before DLLSC.  As said, Linux can handle PDC=0A=
> after DLLSC if they're not too far apart (100 ms, see pcie_wait_for_link(=
)).=0A=
> =0A=
> =0A=
>> I'd like a generic solution. I suspect supporting 'in-band PD disabled'=
=0A=
>> and quirking for that would be a saner way to do things. If we support=
=0A=
>> it, then we need to handle PDSC after DLLSC scenarios anyway.=0A=
> =0A=
> I agree, having support for this new ECN would be great.=0A=
> =0A=
> If you look at struct controller in drivers/pci/hotplug/pciehp.h,=0A=
> there's a section labelled /* capabilities and quirks */.  An idea=0A=
> would be to add an unsigned int there to indicate whether in-band=0A=
> presence is disabled.  An alternative would be to add a flag to=0A=
> struct pci_dev.=0A=
=0A=
> Instead of modifying the logic in pciehp_handle_presence_or_link_change()=
,=0A=
> you could amend pcie_wait_for_link() to poll PDS until it's set, in=0A=
> addition to DLLLA.  The rationale would be that although the link is up,=
=0A=
> the hot-added device cannot really be considered accessible until PDS=0A=
> is also set.  Unfortunately we cannot do this for all devices because=0A=
> (as I've said before), some broken devices hardwire PDS to zero.  But=0A=
> it should be safe to constrain it to those which can disable in-band=0A=
> presence.=0A=
=0A=
=0A=
The recommendation is to set the "in-band PD disable" bit, and it's =0A=
possible that platform firmware may have set it at boot time (*). 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=
(*) A bit hypothetical: There is no hardware yet implementing the ECN.=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'll rework this a little bit for in-band PD disable (what's a good =0A=
acronym for that, BTW?), and then quirk those machines that are known to =
=0A=
'disable' this in hardware.=0A=
=0A=
Alex=0A=