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;
Date:   Tue, 12 Feb 2019 09:30:31 +0100
From:   Lukas Wunner <lukas@wunner.de>
To:     Alex_Gagniuc@Dellteam.com
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
Subject: Re: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up
 after link
Message-ID: <20190212083031.2no7mzn5xug7nba3@wunner.de>
References: <20190205210701.25387-1-mr.nuke.me@gmail.com>
 <20190209115849.244u67h7wmn3eb7o@wunner.de>
 <52afa1c45f684dbda6a8771b65583a22@ausx13mps321.AMER.DELL.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <52afa1c45f684dbda6a8771b65583a22@ausx13mps321.AMER.DELL.COM>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 11, 2019 at 11:48:12PM +0000, Alex_Gagniuc@Dellteam.com wrote:
> On 2/9/19 5:58 AM, Lukas Wunner wrote:
> ECN [1] was approved last November, so it's normative spec text. Sorry 
> if the Ukranians didn't get ahold of it yet. I'll try to explain the delta.
> There's an in-band PD supported/disable set of bits. If 'supported' is 
> set, then you can set 'disable', which removes the 'logical OR" and now 
> PD is only out-of-band presence.

I see, thanks a lot for relaying this information.  The PCI SIG
should probably consider granting access to specs to open source
developers to ease adoption of new features in Linux et al.


> > Table 4-14 in sec 4.2.6 is also important because it shows the difference
> > between Link Up and in-band presence.
> 
> So, we'd expect PD to come up at the same time or before DLL?

Per table 4-14 and figure 4-23 (immediately below the table) in r4.0,
PDC ought to be signaled before DLLSC.  As said, Linux can handle PDC
after DLLSC if they're not too far apart (100 ms, see pcie_wait_for_link()).


> I'd like a generic solution. I suspect supporting 'in-band PD disabled' 
> and quirking for that would be a saner way to do things. If we support 
> it, then we need to handle PDSC after DLLSC scenarios anyway.

I agree, having support for this new ECN would be great.

If you look at struct controller in drivers/pci/hotplug/pciehp.h,
there's a section labelled /* capabilities and quirks */.  An idea
would be to add an unsigned int there to indicate whether in-band
presence is disabled.  An alternative would be to add a flag to
struct pci_dev.

Instead of modifying the logic in pciehp_handle_presence_or_link_change(),
you could amend pcie_wait_for_link() to poll PDS until it's set, in
addition to DLLLA.  The rationale would be that although the link is up,
the hot-added device cannot really be considered accessible until PDS
is also set.  Unfortunately we cannot do this for all devices because
(as I've said before), some broken devices hardwire PDS to zero.  But
it should be safe to constrain it to those which can disable in-band
presence.

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.

You'll probably also need to add code to disable in-band presence if
that is supported, either in pcie_init() in pciehp_hpc.c or in generic
PCI code.

Thanks,

Lukas