pcie/dpc: Fixes

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

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.

---
 drivers/pci/pcie/pcie-dpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d4d70ef..fc2a4a7 100644
@@ -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

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

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.

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

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

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.

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:[email protected]]=20
Sent: Wednesday, May 10, 2017 9:45 AM
To: Wei Zhang <[email protected]>
Cc: Bjorn Helgaas <[email protected]>; [email protected]; Wesley = Yung <[email protected]>; Sammy Hui <[email protected]>; Joseph G= ruher <[email protected]>; Krishna Dhulipala <[email protected]>
Subject: Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

EXTERNAL EMAIL

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.

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' <[email protected]>; Wei Zhang <[email protected]>
Cc: Bjorn Helgaas <[email protected]>; [email protected]; Sammy H= ui <[email protected]>; Joseph Gruher <[email protected]>; Krishn= a Dhulipala <[email protected]>
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:[email protected]]=20
Sent: Wednesday, May 10, 2017 9:45 AM
To: Wei Zhang <[email protected]>
Cc: Bjorn Helgaas <[email protected]>; [email protected]; Wesley = Yung <[email protected]>; Sammy Hui <[email protected]>; Joseph G= ruher <[email protected]>; Krishna Dhulipala <[email protected]>
Subject: Re: [PATCH 1/2] pcie/dpc: Skip DPC event if device is not present

EXTERNAL EMAIL

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.

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.

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.

---
 drivers/pci/pcie/pcie-dpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fc2a4a7..88a66b4 100644
@@ -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

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