From: <Alex_Gagniuc@Dellteam.com>
To: <lukas@wunner.de>
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
Date: Tue, 12 Feb 2019 23:57:55 +0000 [thread overview]
Message-ID: <b2fffa5beb7941ebaceb8b8ce6e5b133@ausx13mps321.AMER.DELL.COM> (raw)
In-Reply-To: 20190212083031.2no7mzn5xug7nba3@wunner.de
On 2/12/19 2:30 AM, Lukas Wunner wrote:
> The PCI SIG
> should probably consider granting access to specs to open source
> developers to ease adoption of new features in Linux et al.
Don't get me started on just how ridiculous I think PCI-SIG's policy is
with respect to public availability of standards.
>>> 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.
The recommendation is to set the "in-band PD disable" bit, and it's
possible that platform firmware may have set it at boot time (*). I'm
not sure that there is a spec-justifiable reason to not access a device
whose DLL is up, but PD isn't.
(*) A bit hypothetical: There is no hardware yet implementing the ECN.
> 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'll rework this a little bit for in-band PD disable (what's a good
acronym for that, BTW?), and then quirk those machines that are known to
'disable' this in hardware.
Alex
next prev parent reply other threads:[~2019-02-12 23:58 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-05 21:06 [PATCH] PCI: pciehp: Do not turn off slot if presence comes up after link Alexandru Gagniuc
2019-02-08 19:56 ` Bjorn Helgaas
2019-02-09 11:58 ` Lukas Wunner
2019-02-11 23:48 ` Alex_Gagniuc
2019-02-12 8:30 ` Lukas Wunner
2019-02-12 10:37 ` Lukas Wunner
2019-02-12 23:57 ` Alex_Gagniuc [this message]
2019-02-13 8:36 ` Lukas Wunner
2019-02-13 18:55 ` Alex_Gagniuc
2019-02-14 7:01 ` Lukas Wunner
2019-02-14 19:26 ` Alex_Gagniuc
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b2fffa5beb7941ebaceb8b8ce6e5b133@ausx13mps321.AMER.DELL.COM \
--to=alex_gagniuc@dellteam.com \
--cc=Austin.Bolen@dell.com \
--cc=Shyam.Iyer@dell.com \
--cc=bhelgaas@google.com \
--cc=gustavo@embeddedor.com \
--cc=keith.busch@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=mr.nuke.me@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).