[PATCH 0/2] pcie/dpc: Fixes

[PATCH 0/2] pcie/dpc: Fixes

[Keith Busch]

Hi Bjorn,

Two small patches for bugs found from testing on new platforms and
hardware.

Keith Busch (2):
  pcie/dpc: Skip DPC event if device is not present
  pcie/dpc: Fix control register setting

 drivers/pci/pcie/pcie-dpc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.2

[PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Keith Busch]

The DPC interupt may be executed on a device that is being removed. Skip
queuing event handling if the status is all 1's, which should be seen
only if the device is not present.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/pcie-dpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index d4d70ef..fc2a4a7 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -87,7 +87,7 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_SOURCE_ID,
 			     &source);
-	if (!status)
+	if (!status || status == (u16)(~0))
 		return IRQ_NONE;
 
 	dev_info(&dpc->dev->device, "DPC containment event, status:%#06x source:%#06x\n",
-- 
2.7.2

Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Wei Zhang]

IEhpIEtlaXRoIGFuZCBXZXMsDQoNCkkgd29uZGVyIGlmIGdldHRpbmcgYW4gQWxsIDHigJlzIHJl
YWQgb24gdGhlIERQQyBzdGF0dXMgcmVnaXN0ZXIgaXMgYSB2YWxpZCBzY2VuYXJpby4gICBUaGUg
RFBDIHJlZ2lzdGVyIGlzIG9uIHRoZSBzd2l0Y2gsIHdoeSB3b3VsZCB0aGUgc3RhdHVzIHJlZ2lz
dGVyIHJldHVybnMgQWxsIDHigJlzIGV2ZW4gaWYgdGhlIGRldmljZSBpcyByZW1vdmVkPw0KDQpU
aGFua3MsDQotV2VpDQoNCi0tDQp3ZWkgemhhbmcgfCBzb2Z0d2FyZSBlbmdpbmVlciB8IGZhY2Vi
b29rDQp3emhhbmdAZmIuY29tIHwgKDQwOCkgNDYwLTQ4MDMNCg0KT24gNC8yOS8xNywgMTI6MDIg
QU0sICJLZWl0aCBCdXNjaCIgPGtlaXRoLmJ1c2NoQGludGVsLmNvbT4gd3JvdGU6DQoNCiAgICBU
aGUgRFBDIGludGVydXB0IG1heSBiZSBleGVjdXRlZCBvbiBhIGRldmljZSB0aGF0IGlzIGJlaW5n
IHJlbW92ZWQuIFNraXANCiAgICBxdWV1aW5nIGV2ZW50IGhhbmRsaW5nIGlmIHRoZSBzdGF0dXMg
aXMgYWxsIDEncywgd2hpY2ggc2hvdWxkIGJlIHNlZW4NCiAgICBvbmx5IGlmIHRoZSBkZXZpY2Ug
aXMgbm90IHByZXNlbnQuDQogICAgDQogICAgU2lnbmVkLW9mZi1ieTogS2VpdGggQnVzY2ggPGtl
aXRoLmJ1c2NoQGludGVsLmNvbT4NCiAgICAtLS0NCiAgICAgZHJpdmVycy9wY2kvcGNpZS9wY2ll
LWRwYy5jIHwgMiArLQ0KICAgICAxIGZpbGUgY2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDEgZGVs
ZXRpb24oLSkNCiAgICANCiAgICBkaWZmIC0tZ2l0IGEvZHJpdmVycy9wY2kvcGNpZS9wY2llLWRw
Yy5jIGIvZHJpdmVycy9wY2kvcGNpZS9wY2llLWRwYy5jDQogICAgaW5kZXggZDRkNzBlZi4uZmMy
YTRhNyAxMDA2NDQNCiAgICAtLS0gYS9kcml2ZXJzL3BjaS9wY2llL3BjaWUtZHBjLmMNCiAgICAr
KysgYi9kcml2ZXJzL3BjaS9wY2llL3BjaWUtZHBjLmMNCiAgICBAQCAtODcsNyArODcsNyBAQCBz
dGF0aWMgaXJxcmV0dXJuX3QgZHBjX2lycShpbnQgaXJxLCB2b2lkICpjb250ZXh0KQ0KICAgICAJ
cGNpX3JlYWRfY29uZmlnX3dvcmQocGRldiwgZHBjLT5jYXBfcG9zICsgUENJX0VYUF9EUENfU1RB
VFVTLCAmc3RhdHVzKTsNCiAgICAgCXBjaV9yZWFkX2NvbmZpZ193b3JkKHBkZXYsIGRwYy0+Y2Fw
X3BvcyArIFBDSV9FWFBfRFBDX1NPVVJDRV9JRCwNCiAgICAgCQkJICAgICAmc291cmNlKTsNCiAg
ICAtCWlmICghc3RhdHVzKQ0KICAgICsJaWYgKCFzdGF0dXMgfHwgc3RhdHVzID09ICh1MTYpKH4w
KSkNCiAgICAgCQlyZXR1cm4gSVJRX05PTkU7DQogICAgIA0KICAgICAJZGV2X2luZm8oJmRwYy0+
ZGV2LT5kZXZpY2UsICJEUEMgY29udGFpbm1lbnQgZXZlbnQsIHN0YXR1czolIzA2eCBzb3VyY2U6
JSMwNnhcbiIsDQogICAgLS0gDQogICAgMi43LjINCiAgICANCiAgICANCg0K

Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Keith Busch]

On Wed, May 10, 2017 at 03:39:27AM +0000, Wei Zhang wrote:
>  Hi Keith and Wes,
> 
> I wonder if getting an All 1’s read on the DPC status register is a
> valid scenario.   The DPC register is on the switch, why would the
> status register returns All 1’s even if the device is removed?

Ah, this isn't about the downstream device precense. This is about
the DPC switch device itself, like if you pull the cable out of the
enclosure. Reading anything off the DSPs in it will see all 1's.

Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Wesley Yung]

T2ggSSBzZWUuIElmIHRoZSB3aG9sZSBzd2l0Y2ggaXMgcmVtb3ZlZCBpZiB0aGV5IHRyeSB0byBy
ZWFkIHRoZSBkc3BzIHRoYXQgd2VyZSB0aGVyZSB3b3VsZCByZXN1bHQgaW4gYSB0aW1lb3V0IGFu
ZCB0aGF0IGxvb2tzIGxpa2UgYWxsIDFzLiANCg0KQ2hlZXJzDQpXZXMNCg0KT24gTWF5IDEwLCAy
MDE3LCBhdCA2OjEwIEFNLCBLZWl0aCBCdXNjaCA8a2VpdGguYnVzY2hAaW50ZWwuY29tPiB3cm90
ZToNCg0KRVhURVJOQUwgRU1BSUwNCg0KDQo+IE9uIFdlZCwgTWF5IDEwLCAyMDE3IGF0IDAzOjM5
OjI3QU0gKzAwMDAsIFdlaSBaaGFuZyB3cm90ZToNCj4gSGkgS2VpdGggYW5kIFdlcywNCj4gDQo+
IEkgd29uZGVyIGlmIGdldHRpbmcgYW4gQWxsIDGhr3MgcmVhZCBvbiB0aGUgRFBDIHN0YXR1cyBy
ZWdpc3RlciBpcyBhDQo+IHZhbGlkIHNjZW5hcmlvLiAgIFRoZSBEUEMgcmVnaXN0ZXIgaXMgb24g
dGhlIHN3aXRjaCwgd2h5IHdvdWxkIHRoZQ0KPiBzdGF0dXMgcmVnaXN0ZXIgcmV0dXJucyBBbGwg
MaGvcyBldmVuIGlmIHRoZSBkZXZpY2UgaXMgcmVtb3ZlZD8NCg0KQWgsIHRoaXMgaXNuJ3QgYWJv
dXQgdGhlIGRvd25zdHJlYW0gZGV2aWNlIHByZWNlbnNlLiBUaGlzIGlzIGFib3V0DQp0aGUgRFBD
IHN3aXRjaCBkZXZpY2UgaXRzZWxmLCBsaWtlIGlmIHlvdSBwdWxsIHRoZSBjYWJsZSBvdXQgb2Yg
dGhlDQplbmNsb3N1cmUuIFJlYWRpbmcgYW55dGhpbmcgb2ZmIHRoZSBEU1BzIGluIGl0IHdpbGwg
c2VlIGFsbCAxJ3MuDQo=

Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Wei Zhang]

SGkgS2VpdGgsDQoNCkkgc2VlLiAgSSB0aG91Z2h0IHRoZSBjdXJyZW50IENQVSByb290IGNvbXBs
ZXggZG9lcyBub3Qgc3VwcG9ydCBzdWNoIGEgdXNlIGNhc2UsIGllIHJlbW92aW5nIHRoZSBEUEMg
c3dpdGNoIGRldmljZSBpdHNlbGYgYW5kIG1pZ2h0IHJlc3VsdCBpbiBrZXJuZWwgcGFuaWMuICBC
dXQgSSBhZ3JlZSB0aGlzIHdpbGwgbWFrZSB0aGUgY29kZSBmdXR1cmUtcHJvb2Ygd2hlbiBDUFUg
ZG9lcyBzdXBwb3J0IHN1Y2ggYSBjYXNlIGluIHRoZSBmdXR1cmUuDQoNClRoYW5rcyBmb3IgdGhl
IGNsYXJpZmljYXRpb24hDQotV2VpDQoNCi0tDQp3ZWkgemhhbmcgfCBzb2Z0d2FyZSBlbmdpbmVl
ciB8IGZhY2Vib29rDQp3emhhbmdAZmIuY29tIHwgKDQwOCkgNDYwLTQ4MDMNCg0KT24gNS8xMC8x
NywgOToxNyBQTSwgIktlaXRoIEJ1c2NoIiA8a2VpdGguYnVzY2hAaW50ZWwuY29tPiB3cm90ZToN
Cg0KICAgIE9uIFdlZCwgTWF5IDEwLCAyMDE3IGF0IDAzOjM5OjI3QU0gKzAwMDAsIFdlaSBaaGFu
ZyB3cm90ZToNCiAgICA+ICBIaSBLZWl0aCBhbmQgV2VzLA0KICAgID4gDQogICAgPiBJIHdvbmRl
ciBpZiBnZXR0aW5nIGFuIEFsbCAx4oCZcyByZWFkIG9uIHRoZSBEUEMgc3RhdHVzIHJlZ2lzdGVy
IGlzIGENCiAgICA+IHZhbGlkIHNjZW5hcmlvLiAgIFRoZSBEUEMgcmVnaXN0ZXIgaXMgb24gdGhl
IHN3aXRjaCwgd2h5IHdvdWxkIHRoZQ0KICAgID4gc3RhdHVzIHJlZ2lzdGVyIHJldHVybnMgQWxs
IDHigJlzIGV2ZW4gaWYgdGhlIGRldmljZSBpcyByZW1vdmVkPw0KICAgIA0KICAgIEFoLCB0aGlz
IGlzbid0IGFib3V0IHRoZSBkb3duc3RyZWFtIGRldmljZSBwcmVjZW5zZS4gVGhpcyBpcyBhYm91
dA0KICAgIHRoZSBEUEMgc3dpdGNoIGRldmljZSBpdHNlbGYsIGxpa2UgaWYgeW91IHB1bGwgdGhl
IGNhYmxlIG91dCBvZiB0aGUNCiAgICBlbmNsb3N1cmUuIFJlYWRpbmcgYW55dGhpbmcgb2ZmIHRo
ZSBEU1BzIGluIGl0IHdpbGwgc2VlIGFsbCAxJ3MuDQogICAgDQoNCg==

Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Keith Busch]

On Wed, May 10, 2017 at 04:35:06PM +0000, Wei Zhang wrote:
> Hi Keith,
> 
> I see.  I thought the current CPU root complex does not support such a use case, ie removing the DPC switch device itself and might result in kernel panic.  But I agree this will make the code future-proof when CPU does support such a case in the future.

What do you mean in the future? I do this today (hotplug enclosures),
but I need this fix in place otherwise we get a use-after-free when
the DPC work queue runs after the hotplug code freed the topology that
includes the DPC parts.

RE: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Wesley Yung]

Hey Keith,

Does the RC Downstream ports also have DPC capability so the driver actuall=
y loads for the Root Port DSPs?  Just trying to understand the mechanism.

For Wei's specific use case, removing a switch from the RP is not officiall=
y supported but technically it should be possible to do so.  On the switch =
end we just need to know where the ecosystem is at so we can make sure to p=
lay nice.

Wes

-----Original Message-----
From: Keith Busch [mailto:keith.busch@intel.com]=20
Sent: Wednesday, May 10, 2017 9:45 AM
To: Wei Zhang <wzhang@fb.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org; Wesley =
Yung <wesley.yung@microsemi.com>; Sammy Hui <sammy.hui@intel.com>; Joseph G=
ruher <joseph.r.gruher@intel.com>; Krishna Dhulipala <krishnad@fb.com>
Subject: Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

EXTERNAL EMAIL


On Wed, May 10, 2017 at 04:35:06PM +0000, Wei Zhang wrote:
> Hi Keith,
>
> I see.  I thought the current CPU root complex does not support such a us=
e case, ie removing the DPC switch device itself and might result in kernel=
 panic.  But I agree this will make the code future-proof when CPU does sup=
port such a case in the future.

What do you mean in the future? I do this today (hotplug enclosures), but I=
 need this fix in place otherwise we get a use-after-free when the DPC work=
 queue runs after the hotplug code freed the topology that includes the DPC=
 parts.

RE: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Wesley Yung]

Just to add more colour:

If a switch is removed from the RP.  All non posted's will time out.  This =
includes a config read of any switch P2P.  I believe Keith is saying if a r=
ead comes down to read the DPC status on a Switch DS P2P that read will tim=
e out after the host TMO.  Once that occurs, the IIO will generate all 1s b=
ack to the DPC driver.=20

It means all the back end drivers (DPC, AER, Hot Plug, NVMe) need to have s=
ome understanding that all 1s means "something happened".  In the case of t=
he DPC driver, we know the DPC status should never have 1s in the most sign=
ificant bits so returning all 1s is a special return type that needs to be =
handled.

Wes

-----Original Message-----
From: Wesley Yung=20
Sent: Wednesday, May 10, 2017 9:43 AM
To: 'Keith Busch' <keith.busch@intel.com>; Wei Zhang <wzhang@fb.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org; Sammy H=
ui <sammy.hui@intel.com>; Joseph Gruher <joseph.r.gruher@intel.com>; Krishn=
a Dhulipala <krishnad@fb.com>
Subject: RE: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

Hey Keith,

Does the RC Downstream ports also have DPC capability so the driver actuall=
y loads for the Root Port DSPs?  Just trying to understand the mechanism.

For Wei's specific use case, removing a switch from the RP is not officiall=
y supported but technically it should be possible to do so.  On the switch =
end we just need to know where the ecosystem is at so we can make sure to p=
lay nice.

Wes

-----Original Message-----
From: Keith Busch [mailto:keith.busch@intel.com]=20
Sent: Wednesday, May 10, 2017 9:45 AM
To: Wei Zhang <wzhang@fb.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org; Wesley =
Yung <wesley.yung@microsemi.com>; Sammy Hui <sammy.hui@intel.com>; Joseph G=
ruher <joseph.r.gruher@intel.com>; Krishna Dhulipala <krishnad@fb.com>
Subject: Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

EXTERNAL EMAIL


On Wed, May 10, 2017 at 04:35:06PM +0000, Wei Zhang wrote:
> Hi Keith,
>
> I see.  I thought the current CPU root complex does not support such a us=
e case, ie removing the DPC switch device itself and might result in kernel=
 panic.  But I agree this will make the code future-proof when CPU does sup=
port such a case in the future.

What do you mean in the future? I do this today (hotplug enclosures), but I=
 need this fix in place otherwise we get a use-after-free when the DPC work=
 queue runs after the hotplug code freed the topology that includes the DPC=
 parts.

Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

[Keith Busch]

Oh, I see what you're asking about. I'm not testing DPC capable RP's. I
do not have such capabilities on my platforms. In fact, the machines I
have are quite old, Sandy Bridge generation.

I've a switch connected to the RP, then the external enclosures are
downstream that.  This topology has multiple levels of switches and
devices, but I still can't unplug the top level as you expected; only
from anywhere in the middle.

Removing the enclosure triggers DPC on the switch connected to the RP.
Sometimes the interrupt handler for the now removed enclosure switches
triggers. The driver checking the DPC status gets a Completer Abort and
the reader see's all 1's, so this is not a CTO type situation.

On Wed, May 10, 2017 at 04:52:16PM +0000, Wesley Yung wrote:
> Just to add more colour:
> 
> If a switch is removed from the RP.  All non posted's will time out.  This includes a config read of any switch P2P.  I believe Keith is saying if a read comes down to read the DPC status on a Switch DS P2P that read will time out after the host TMO.  Once that occurs, the IIO will generate all 1s back to the DPC driver. 
> 
> It means all the back end drivers (DPC, AER, Hot Plug, NVMe) need to have some understanding that all 1s means "something happened".  In the case of the DPC driver, we know the DPC status should never have 1s in the most significant bits so returning all 1s is a special return type that needs to be handled.
> 
> Wes
> 
> -----Original Message-----
> From: Wesley Yung 
> Sent: Wednesday, May 10, 2017 9:43 AM
> To: 'Keith Busch' <keith.busch@intel.com>; Wei Zhang <wzhang@fb.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org; Sammy Hui <sammy.hui@intel.com>; Joseph Gruher <joseph.r.gruher@intel.com>; Krishna Dhulipala <krishnad@fb.com>
> Subject: RE: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present
> 
> Hey Keith,
> 
> Does the RC Downstream ports also have DPC capability so the driver actually loads for the Root Port DSPs?  Just trying to understand the mechanism.
> 
> For Wei's specific use case, removing a switch from the RP is not officially supported but technically it should be possible to do so.  On the switch end we just need to know where the ecosystem is at so we can make sure to play nice.
> 
> Wes

[PATCH 2/2] pcie/dpc: Fix control register setting

[Keith Busch]

This driver was OR'ing desired bits from the existing control
setting. That could create an invalid DPC Trigger Enabled configuration
if the platform previously set this to "ERR_FATAL", 01b. The driver
currently wants to set this to ERR_NONFATAL/ERR_FATAL, 10b, and the
logical OR of this gets 11b, which is reserved. This patch fixes that
by masking off the fields it is setting.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/pcie-dpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index fc2a4a7..88a66b4 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -139,7 +139,7 @@ static int dpc_probe(struct pcie_device *dev)
 
 	dpc->rp = (cap & PCI_EXP_DPC_CAP_RP_EXT);
 
-	ctl |= PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN;
+	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN;
 	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
 
 	dev_info(&dev->device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
-- 
2.7.2

Re: [PATCH 0/2] pcie/dpc: Fixes

[Bjorn Helgaas]

On Fri, Apr 28, 2017 at 12:02:47PM -0400, Keith Busch wrote:
> Hi Bjorn,
> 
> Two small patches for bugs found from testing on new platforms and
> hardware.
> 
> Keith Busch (2):
>   pcie/dpc: Skip DPC event if device is not present
>   pcie/dpc: Fix control register setting
> 
>  drivers/pci/pcie/pcie-dpc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied to pci/dpc for v4.13, thanks, Keith!