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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20190213083632.GA3387@wunner.de>
Date:   Wed, 13 Feb 2019 09:36:32 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Alex_Gagniuc@...lteam.com
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 Tue, Feb 12, 2019 at 11:57:55PM +0000, Alex_Gagniuc@...lteam.com wrote:
> 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

Ok, then I'd suggest to check in pcie_init() whether the feature is
supported and enable it if so.  And store that fact in a flag, either
in struct pci_dev or struct controller.

> (*) A bit hypothetical: There is no hardware yet implementing the ECN.

Hm, this contradicts Austin Bolen's e-mail of Jan 25 that "Yes, this
platform disables in-band presence" (when asked whether your host
controller already adheres to the ECN).

> I'm 
> not sure that there is a spec-justifiable reason to not access a device 
> whose DLL is up, but PD isn't.

Austin asked in an e-mail of Jan 24 whether "the hot-inserted device's
config space [is] accessed immediately after waiting this 20 + 100 ms
delay", which sounded to me like you'd prefer the device not to be
accessed until PDS is 1.  This can be achieved by polling PDS in
pcie_wait_for_link() if in-band presence is disabled, which is why
I suggested it.

> > 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'm worried that amending pciehp_handle_presence_or_link_change() makes
the event handling logic more difficult to understand.  Polling PDS in
pcie_wait_for_link() or disabling either PDC or DLLSC if in-band presence
is disabled seems simpler to reason about.

> in-band PD disable (what's a good acronym for that, BTW?)

I don't know, maybe inband_presence_disabled?

Thanks,

Lukas