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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b2fffa5beb7941ebaceb8b8ce6e5b133@ausx13mps321.AMER.DELL.COM>
Date:   Tue, 12 Feb 2019 23:57:55 +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/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