[PATCH] PCI: pciehp: Remove surprise bit checks

[PATCH] PCI: pciehp: Remove surprise bit checks

[Takashi Iwai]

Currently pciehp driver checks the surprise removal bit and handles
the hotplug event only when the bit is set.  But there are devices
that don't set that bit but yet expect the hotplug working, e.g. the
PCI card reader on HP ProBook 445 and 455 laptops appears only when
you insert a card, and it needs the hotplug event handling.

For fixing this, basically we may ignore the surprise bit and always
handle the event.  The only big concern in the past was the KVM device
assignment, but this has been fixed in KVM side (ignoring hotplug
events during secondary bus resets), there should be no obstacle
ahead.

The earlier discussion thread is found at:
    https://lkml.org/lkml/2013/3/20/274

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=70261
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 11 -----------
 drivers/pci/hotplug/pciehp_hpc.c  | 19 +++++++------------
 2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 50628487597d..8f6c626b4e21 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -434,8 +434,6 @@ static void interrupt_event_handler(struct work_struct *work)
 		break;
 	case INT_PRESENCE_ON:
 	case INT_PRESENCE_OFF:
-		if (!HP_SUPR_RM(ctrl))
-			break;
 		ctrl_dbg(ctrl, "Surprise Removal\n");
 		handle_surprise_event(p_slot);
 		break;
@@ -494,15 +492,6 @@ int pciehp_disable_slot(struct slot *p_slot)
 	if (!p_slot->ctrl)
 		return 1;
 
-	if (!HP_SUPR_RM(p_slot->ctrl)) {
-		pciehp_get_adapter_status(p_slot, &getstatus);
-		if (!getstatus) {
-			ctrl_info(ctrl, "No adapter on slot(%s)\n",
-				  slot_name(p_slot));
-			return -ENODEV;
-		}
-	}
-
 	if (MRL_SENS(p_slot->ctrl)) {
 		pciehp_get_latch_status(p_slot, &getstatus);
 		if (getstatus) {
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 14acfccb7670..5f9196a85a53 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -636,21 +636,16 @@ int pciehp_reset_slot(struct slot *slot, int probe)
 	if (probe)
 		return 0;
 
-	if (HP_SUPR_RM(ctrl)) {
-		pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_PDCE);
-		if (pciehp_poll_mode)
-			del_timer_sync(&ctrl->poll_timer);
-	}
+	pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_PDCE);
+	if (pciehp_poll_mode)
+		del_timer_sync(&ctrl->poll_timer);
 
 	pci_reset_bridge_secondary_bus(ctrl->pcie->port);
 
-	if (HP_SUPR_RM(ctrl)) {
-		pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-					   PCI_EXP_SLTSTA_PDC);
-		pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PDCE, PCI_EXP_SLTCTL_PDCE);
-		if (pciehp_poll_mode)
-			int_poll_timeout(ctrl->poll_timer.data);
-	}
+	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_PDC);
+	pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PDCE, PCI_EXP_SLTCTL_PDCE);
+	if (pciehp_poll_mode)
+		int_poll_timeout(ctrl->poll_timer.data);
 
 	return 0;
 }
-- 
1.8.5.2

RE: [PATCH] PCI: pciehp: Remove surprise bit checks

[Rajat Jain]

SGVsbG8gVGFrYXNoaSwNCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBs
aW51eC1wY2ktb3duZXJAdmdlci5rZXJuZWwub3JnIFttYWlsdG86bGludXgtcGNpLQ0KPiBvd25l
ckB2Z2VyLmtlcm5lbC5vcmddIE9uIEJlaGFsZiBPZiBUYWthc2hpIEl3YWkNCj4gU2VudDogVHVl
c2RheSwgRmVicnVhcnkgMTEsIDIwMTQgMzozOSBBTQ0KPiBUbzogQmpvcm4gSGVsZ2Fhcw0KPiBD
YzogQWxleCBXaWxsaWFtc29uOyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBsaW51eC0NCj4g
a2VybmVsQHZnZXIua2VybmVsLm9yZw0KPiBTdWJqZWN0OiBbUEFUQ0hdIFBDSTogcGNpZWhwOiBS
ZW1vdmUgc3VycHJpc2UgYml0IGNoZWNrcw0KPiANCj4gQ3VycmVudGx5IHBjaWVocCBkcml2ZXIg
Y2hlY2tzIHRoZSBzdXJwcmlzZSByZW1vdmFsIGJpdCBhbmQgaGFuZGxlcw0KPiB0aGUgaG90cGx1
ZyBldmVudCBvbmx5IHdoZW4gdGhlIGJpdCBpcyBzZXQuICBCdXQgdGhlcmUgYXJlIGRldmljZXMN
Cj4gdGhhdCBkb24ndCBzZXQgdGhhdCBiaXQgYnV0IHlldCBleHBlY3QgdGhlIGhvdHBsdWcgd29y
a2luZywgZS5nLiB0aGUNCj4gUENJIGNhcmQgcmVhZGVyIG9uIEhQIFByb0Jvb2sgNDQ1IGFuZCA0
NTUgbGFwdG9wcyBhcHBlYXJzIG9ubHkgd2hlbg0KPiB5b3UgaW5zZXJ0IGEgY2FyZCwgYW5kIGl0
IG5lZWRzIHRoZSBob3RwbHVnIGV2ZW50IGhhbmRsaW5nLg0KPiANCj4gRm9yIGZpeGluZyB0aGlz
LCBiYXNpY2FsbHkgd2UgbWF5IGlnbm9yZSB0aGUgc3VycHJpc2UgYml0IGFuZCBhbHdheXMNCj4g
aGFuZGxlIHRoZSBldmVudC4NCg0KU29tZSBvZiB0aGlzIGhhcyBiZWVuIHRha2VuIGNhcmUgb2Yg
aW4gdGhlIHJlY2VudCBjaGFuZ2VzIHRvIHBjaWVoY3AgKGxhc3QgbmlnaHQpLg0KDQpQbGVhc2Ug
c2VlDQoNCmh0dHBzOi8vZ2l0Lmtlcm5lbC5vcmcvY2dpdC9saW51eC9rZXJuZWwvZ2l0L2hlbGdh
YXMvcGNpLmdpdC9jb21taXQvP2g9cGNpL3BjaWVocCZpZD1hM2ViYTM4ODc5NjBkMDBhYmI5MTJm
ZTM3MDNjZDNhMDU4YzcyMWI4DQoNCkFsc28sIGxpbmsgc3RhdGUgaG90cGx1ZyBoYXMgYmVlbiBh
ZGRlZCBhcyBwYXJ0IG9mIHRoaXMgcGF0Y2ggc2V0LiBDYW4geW91IHBsZWFzZSB0cnkgd2l0aCBC
am9ybidzIGxhdGVzdCBwY2kgdHJlZT8NCg0KV2l0aCB0aGF0LCBJIHRoaW5rIHlvdSBzaG91bGQg
bm90IHNlZSB0aGUgaXNzdWUgeW91IGFyZSBkZXNjcmliaW5nLCBiZWNhdXNlIGlmIHRoZSBQQ0ll
IExpbmsgY29tZXMgdXAgd2hlbiB5b3UgaW5zZXJ0IHRoZSBjYXJkLCBpdCBzaGFsbCBiZSBhZGRl
ZCAobm93KSBpcnJlc3BlY3RpdmUgb2Ygd2hldGhlciBvciBub3QgeW91IGhhdmUgdGhlIHN1cnBy
aXNlIGJpdCBzZXQuDQoNClRoYW5rcywNCg0KUmFqYXQNCg0KDQo+IFRoZSBvbmx5IGJpZyBjb25j
ZXJuIGluIHRoZSBwYXN0IHdhcyB0aGUgS1ZNIGRldmljZQ0KPiBhc3NpZ25tZW50LCBidXQgdGhp
cyBoYXMgYmVlbiBmaXhlZCBpbiBLVk0gc2lkZSAoaWdub3JpbmcgaG90cGx1Zw0KPiBldmVudHMg
ZHVyaW5nIHNlY29uZGFyeSBidXMgcmVzZXRzKSwgdGhlcmUgc2hvdWxkIGJlIG5vIG9ic3RhY2xl
DQo+IGFoZWFkLg0KPiANCj4gVGhlIGVhcmxpZXIgZGlzY3Vzc2lvbiB0aHJlYWQgaXMgZm91bmQg
YXQ6DQo+ICAgICBodHRwczovL2xrbWwub3JnL2xrbWwvMjAxMy8zLzIwLzI3NA0KPiANCj4gQnVn
emlsbGE6IGh0dHBzOi8vYnVnemlsbGEua2VybmVsLm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzAyNjEN
Cj4gU2lnbmVkLW9mZi1ieTogVGFrYXNoaSBJd2FpIDx0aXdhaUBzdXNlLmRlPg0KPiAtLS0NCj4g
IGRyaXZlcnMvcGNpL2hvdHBsdWcvcGNpZWhwX2N0cmwuYyB8IDExIC0tLS0tLS0tLS0tDQo+ICBk
cml2ZXJzL3BjaS9ob3RwbHVnL3BjaWVocF9ocGMuYyAgfCAxOSArKysrKysrLS0tLS0tLS0tLS0t
DQo+ICAyIGZpbGVzIGNoYW5nZWQsIDcgaW5zZXJ0aW9ucygrKSwgMjMgZGVsZXRpb25zKC0pDQo+
IA0KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9wY2kvaG90cGx1Zy9wY2llaHBfY3RybC5jDQo+IGIv
ZHJpdmVycy9wY2kvaG90cGx1Zy9wY2llaHBfY3RybC5jDQo+IGluZGV4IDUwNjI4NDg3NTk3ZC4u
OGY2YzYyNmI0ZTIxIDEwMDY0NA0KPiAtLS0gYS9kcml2ZXJzL3BjaS9ob3RwbHVnL3BjaWVocF9j
dHJsLmMNCj4gKysrIGIvZHJpdmVycy9wY2kvaG90cGx1Zy9wY2llaHBfY3RybC5jDQo+IEBAIC00
MzQsOCArNDM0LDYgQEAgc3RhdGljIHZvaWQgaW50ZXJydXB0X2V2ZW50X2hhbmRsZXIoc3RydWN0
DQo+IHdvcmtfc3RydWN0ICp3b3JrKQ0KPiAgCQlicmVhazsNCj4gIAljYXNlIElOVF9QUkVTRU5D
RV9PTjoNCj4gIAljYXNlIElOVF9QUkVTRU5DRV9PRkY6DQo+IC0JCWlmICghSFBfU1VQUl9STShj
dHJsKSkNCj4gLQkJCWJyZWFrOw0KPiAgCQljdHJsX2RiZyhjdHJsLCAiU3VycHJpc2UgUmVtb3Zh
bFxuIik7DQo+ICAJCWhhbmRsZV9zdXJwcmlzZV9ldmVudChwX3Nsb3QpOw0KPiAgCQlicmVhazsN
Cj4gQEAgLTQ5NCwxNSArNDkyLDYgQEAgaW50IHBjaWVocF9kaXNhYmxlX3Nsb3Qoc3RydWN0IHNs
b3QgKnBfc2xvdCkNCj4gIAlpZiAoIXBfc2xvdC0+Y3RybCkNCj4gIAkJcmV0dXJuIDE7DQo+IA0K
PiAtCWlmICghSFBfU1VQUl9STShwX3Nsb3QtPmN0cmwpKSB7DQo+IC0JCXBjaWVocF9nZXRfYWRh
cHRlcl9zdGF0dXMocF9zbG90LCAmZ2V0c3RhdHVzKTsNCj4gLQkJaWYgKCFnZXRzdGF0dXMpIHsN
Cj4gLQkJCWN0cmxfaW5mbyhjdHJsLCAiTm8gYWRhcHRlciBvbiBzbG90KCVzKVxuIiwNCj4gLQkJ
CQkgIHNsb3RfbmFtZShwX3Nsb3QpKTsNCj4gLQkJCXJldHVybiAtRU5PREVWOw0KPiAtCQl9DQo+
IC0JfQ0KPiAtDQo+ICAJaWYgKE1STF9TRU5TKHBfc2xvdC0+Y3RybCkpIHsNCj4gIAkJcGNpZWhw
X2dldF9sYXRjaF9zdGF0dXMocF9zbG90LCAmZ2V0c3RhdHVzKTsNCj4gIAkJaWYgKGdldHN0YXR1
cykgew0KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9wY2kvaG90cGx1Zy9wY2llaHBfaHBjLmMNCj4g
Yi9kcml2ZXJzL3BjaS9ob3RwbHVnL3BjaWVocF9ocGMuYw0KPiBpbmRleCAxNGFjZmNjYjc2NzAu
LjVmOTE5NmE4NWE1MyAxMDA2NDQNCj4gLS0tIGEvZHJpdmVycy9wY2kvaG90cGx1Zy9wY2llaHBf
aHBjLmMNCj4gKysrIGIvZHJpdmVycy9wY2kvaG90cGx1Zy9wY2llaHBfaHBjLmMNCj4gQEAgLTYz
NiwyMSArNjM2LDE2IEBAIGludCBwY2llaHBfcmVzZXRfc2xvdChzdHJ1Y3Qgc2xvdCAqc2xvdCwg
aW50DQo+IHByb2JlKQ0KPiAgCWlmIChwcm9iZSkNCj4gIAkJcmV0dXJuIDA7DQo+IA0KPiAtCWlm
IChIUF9TVVBSX1JNKGN0cmwpKSB7DQo+IC0JCXBjaWVfd3JpdGVfY21kKGN0cmwsIDAsIFBDSV9F
WFBfU0xUQ1RMX1BEQ0UpOw0KPiAtCQlpZiAocGNpZWhwX3BvbGxfbW9kZSkNCj4gLQkJCWRlbF90
aW1lcl9zeW5jKCZjdHJsLT5wb2xsX3RpbWVyKTsNCj4gLQl9DQo+ICsJcGNpZV93cml0ZV9jbWQo
Y3RybCwgMCwgUENJX0VYUF9TTFRDVExfUERDRSk7DQo+ICsJaWYgKHBjaWVocF9wb2xsX21vZGUp
DQo+ICsJCWRlbF90aW1lcl9zeW5jKCZjdHJsLT5wb2xsX3RpbWVyKTsNCj4gDQo+ICAJcGNpX3Jl
c2V0X2JyaWRnZV9zZWNvbmRhcnlfYnVzKGN0cmwtPnBjaWUtPnBvcnQpOw0KPiANCj4gLQlpZiAo
SFBfU1VQUl9STShjdHJsKSkgew0KPiAtCQlwY2llX2NhcGFiaWxpdHlfd3JpdGVfd29yZChwZGV2
LCBQQ0lfRVhQX1NMVFNUQSwNCj4gLQkJCQkJICAgUENJX0VYUF9TTFRTVEFfUERDKTsNCj4gLQkJ
cGNpZV93cml0ZV9jbWQoY3RybCwgUENJX0VYUF9TTFRDVExfUERDRSwNCj4gUENJX0VYUF9TTFRD
VExfUERDRSk7DQo+IC0JCWlmIChwY2llaHBfcG9sbF9tb2RlKQ0KPiAtCQkJaW50X3BvbGxfdGlt
ZW91dChjdHJsLT5wb2xsX3RpbWVyLmRhdGEpOw0KPiAtCX0NCj4gKwlwY2llX2NhcGFiaWxpdHlf
d3JpdGVfd29yZChwZGV2LCBQQ0lfRVhQX1NMVFNUQSwNCj4gUENJX0VYUF9TTFRTVEFfUERDKTsN
Cj4gKwlwY2llX3dyaXRlX2NtZChjdHJsLCBQQ0lfRVhQX1NMVENUTF9QRENFLCBQQ0lfRVhQX1NM
VENUTF9QRENFKTsNCj4gKwlpZiAocGNpZWhwX3BvbGxfbW9kZSkNCj4gKwkJaW50X3BvbGxfdGlt
ZW91dChjdHJsLT5wb2xsX3RpbWVyLmRhdGEpOw0KPiANCj4gIAlyZXR1cm4gMDsNCj4gIH0NCj4g
LS0NCj4gMS44LjUuMg0KPiANCj4gLS0NCj4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6
IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LXBjaSIgaW4NCj4gdGhlIGJvZHkgb2Yg
YSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcNCj4gTW9yZSBtYWpvcmRvbW8g
aW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5odG1sDQo+IA0K
DQo=

Re: [PATCH] PCI: pciehp: Remove surprise bit checks

[Bjorn Helgaas]

On Tue, Feb 11, 2014 at 06:52:30PM +0000, Rajat Jain wrote:
> Hello Takashi,
> 
> > -----Original Message-----
> > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > owner@vger.kernel.org] On Behalf Of Takashi Iwai
> > Sent: Tuesday, February 11, 2014 3:39 AM
> > To: Bjorn Helgaas
> > Cc: Alex Williamson; linux-pci@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: [PATCH] PCI: pciehp: Remove surprise bit checks
> > 
> > Currently pciehp driver checks the surprise removal bit and handles
> > the hotplug event only when the bit is set.  But there are devices
> > that don't set that bit but yet expect the hotplug working, e.g. the
> > PCI card reader on HP ProBook 445 and 455 laptops appears only when
> > you insert a card, and it needs the hotplug event handling.
> > 
> > For fixing this, basically we may ignore the surprise bit and always
> > handle the event.
> 
> Some of this has been taken care of in the recent changes to pciehcp (last night).
> 
> Please see
> 
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pciehp&id=a3eba3887960d00abb912fe3703cd3a058c721b8
> 
> Also, link state hotplug has been added as part of this patch set. Can you please try with Bjorn's latest pci tree?
> 
> With that, I think you should not see the issue you are describing, because if the PCIe Link comes up when you insert the card, it shall be added (now) irrespective of whether or not you have the surprise bit set.

Takashi's patch removed more HP_SUPR_RM() usage than yours, and I think
those additional changes are probably necessary.  I rebased Takashi's patch
on top of my pci/pciehp branch, resulting in the patch below.

I pushed this, so you can try the whole thing out at:

http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pciehp&id=237e0218bcad52d3df6746cd8d15a0d353bae84f

Bjorn



PCI: pciehp: Remove surprise bit checks

From: Takashi Iwai <tiwai@suse.de>

Currently pciehp driver checks the surprise removal bit and handles the
hotplug event only when the bit is set.  But there are devices that don't
set that bit, yet expect hotplug to work, e.g., the SD/MMC card reader on
HP ProBook 445 and 455 laptops appears only when you insert a card, and it
needs the hotplug event handling.

For fixing this, basically we may ignore the surprise bit and always handle
the event.  The only big concern in the past was the KVM device assignment,
but this has been fixed in KVM side (ignoring hotplug events during
secondary bus resets), there should be no obstacle ahead.

The earlier discussion thread is found at:
    https://lkml.kernel.org/r/s5h38vqjhk6.wl%25tiwai@suse.de

[bhelgaas: rebase on Rajat's changes, remove HP_SUPR_RM() completely]
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=70261
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp.h      |    1 -
 drivers/pci/hotplug/pciehp_ctrl.c |    2 --
 drivers/pci/hotplug/pciehp_hpc.c  |   11 +++++------
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 8a66866b8cf1..d2a67aa0e49d 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -124,7 +124,6 @@ struct controller {
 #define MRL_SENS(ctrl)		((ctrl)->slot_cap & PCI_EXP_SLTCAP_MRLSP)
 #define ATTN_LED(ctrl)		((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP)
 #define PWR_LED(ctrl)		((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP)
-#define HP_SUPR_RM(ctrl)	((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS)
 #define EMI(ctrl)		((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP)
 #define NO_CMD_CMPL(ctrl)	((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)
 #define PSN(ctrl)		((ctrl)->slot_cap >> 19)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index fec99a164d93..a4d29d24057d 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -535,8 +535,6 @@ static void interrupt_event_handler(struct work_struct *work)
 		break;
 	case INT_PRESENCE_ON:
 	case INT_PRESENCE_OFF:
-		if (!HP_SUPR_RM(ctrl))
-			break;
 		ctrl_dbg(ctrl, "Surprise Removal\n");
 		handle_surprise_event(p_slot);
 		break;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index da4b0204b4f7..1076a51623b5 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -628,17 +628,18 @@ int pciehp_reset_slot(struct slot *slot, int probe)
 {
 	struct controller *ctrl = slot->ctrl;
 	struct pci_dev *pdev = ctrl_dev(ctrl);
-	u16 stat_mask = 0, ctrl_mask = 0;
+	u16 stat_mask, ctrl_mask;
 
 	if (probe)
 		return 0;
 
-	if (HP_SUPR_RM(ctrl) && !ATTN_BUTTN(ctrl)) {
+	ctrl_mask = PCI_EXP_SLTCTL_DLLSCE;
+	stat_mask = PCI_EXP_SLTSTA_DLLSC;
+
+	if (!ATTN_BUTTN(ctrl)) {	/* as in pcie_enable_notification() */
 		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
 		stat_mask |= PCI_EXP_SLTSTA_PDC;
 	}
-	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
-	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
 
 	pcie_write_cmd(ctrl, 0, ctrl_mask);
 	if (pciehp_poll_mode)
@@ -741,8 +742,6 @@ static inline void dbg_ctrl(struct controller *ctrl)
 		  ATTN_LED(ctrl)   ? "yes" : "no");
 	ctrl_info(ctrl, "  Power Indicator      : %3s\n",
 		  PWR_LED(ctrl)    ? "yes" : "no");
-	ctrl_info(ctrl, "  Hot-Plug Surprise    : %3s\n",
-		  HP_SUPR_RM(ctrl) ? "yes" : "no");
 	ctrl_info(ctrl, "  EMI Present          : %3s\n",
 		  EMI(ctrl)        ? "yes" : "no");
 	ctrl_info(ctrl, "  Command Completed    : %3s\n",

Re: [PATCH] PCI: pciehp: Remove surprise bit checks

[Takashi Iwai]

At Tue, 11 Feb 2014 16:18:35 -0700,
Bjorn Helgaas wrote:
> 
> On Tue, Feb 11, 2014 at 06:52:30PM +0000, Rajat Jain wrote:
> > Hello Takashi,
> > 
> > > -----Original Message-----
> > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > > owner@vger.kernel.org] On Behalf Of Takashi Iwai
> > > Sent: Tuesday, February 11, 2014 3:39 AM
> > > To: Bjorn Helgaas
> > > Cc: Alex Williamson; linux-pci@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: [PATCH] PCI: pciehp: Remove surprise bit checks
> > > 
> > > Currently pciehp driver checks the surprise removal bit and handles
> > > the hotplug event only when the bit is set.  But there are devices
> > > that don't set that bit but yet expect the hotplug working, e.g. the
> > > PCI card reader on HP ProBook 445 and 455 laptops appears only when
> > > you insert a card, and it needs the hotplug event handling.
> > > 
> > > For fixing this, basically we may ignore the surprise bit and always
> > > handle the event.
> > 
> > Some of this has been taken care of in the recent changes to pciehcp (last night).
> > 
> > Please see
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pciehp&id=a3eba3887960d00abb912fe3703cd3a058c721b8
> > 
> > Also, link state hotplug has been added as part of this patch set. Can you please try with Bjorn's latest pci tree?
> > 
> > With that, I think you should not see the issue you are describing, because if the PCIe Link comes up when you insert the card, it shall be added (now) irrespective of whether or not you have the surprise bit set.
> 
> Takashi's patch removed more HP_SUPR_RM() usage than yours, and I think
> those additional changes are probably necessary.  I rebased Takashi's patch
> on top of my pci/pciehp branch, resulting in the patch below.
> 
> I pushed this, so you can try the whole thing out at:
> 
> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pciehp&id=237e0218bcad52d3df6746cd8d15a0d353bae84f

Thanks, I confirmed that the tree works.


Takashi

> 
> Bjorn
> 
> 
> 
> PCI: pciehp: Remove surprise bit checks
> 
> From: Takashi Iwai <tiwai@suse.de>
> 
> Currently pciehp driver checks the surprise removal bit and handles the
> hotplug event only when the bit is set.  But there are devices that don't
> set that bit, yet expect hotplug to work, e.g., the SD/MMC card reader on
> HP ProBook 445 and 455 laptops appears only when you insert a card, and it
> needs the hotplug event handling.
> 
> For fixing this, basically we may ignore the surprise bit and always handle
> the event.  The only big concern in the past was the KVM device assignment,
> but this has been fixed in KVM side (ignoring hotplug events during
> secondary bus resets), there should be no obstacle ahead.
> 
> The earlier discussion thread is found at:
>     https://lkml.kernel.org/r/s5h38vqjhk6.wl%25tiwai@suse.de
> 
> [bhelgaas: rebase on Rajat's changes, remove HP_SUPR_RM() completely]
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=70261
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/hotplug/pciehp.h      |    1 -
>  drivers/pci/hotplug/pciehp_ctrl.c |    2 --
>  drivers/pci/hotplug/pciehp_hpc.c  |   11 +++++------
>  3 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 8a66866b8cf1..d2a67aa0e49d 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -124,7 +124,6 @@ struct controller {
>  #define MRL_SENS(ctrl)		((ctrl)->slot_cap & PCI_EXP_SLTCAP_MRLSP)
>  #define ATTN_LED(ctrl)		((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP)
>  #define PWR_LED(ctrl)		((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP)
> -#define HP_SUPR_RM(ctrl)	((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS)
>  #define EMI(ctrl)		((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP)
>  #define NO_CMD_CMPL(ctrl)	((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)
>  #define PSN(ctrl)		((ctrl)->slot_cap >> 19)
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index fec99a164d93..a4d29d24057d 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -535,8 +535,6 @@ static void interrupt_event_handler(struct work_struct *work)
>  		break;
>  	case INT_PRESENCE_ON:
>  	case INT_PRESENCE_OFF:
> -		if (!HP_SUPR_RM(ctrl))
> -			break;
>  		ctrl_dbg(ctrl, "Surprise Removal\n");
>  		handle_surprise_event(p_slot);
>  		break;
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index da4b0204b4f7..1076a51623b5 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -628,17 +628,18 @@ int pciehp_reset_slot(struct slot *slot, int probe)
>  {
>  	struct controller *ctrl = slot->ctrl;
>  	struct pci_dev *pdev = ctrl_dev(ctrl);
> -	u16 stat_mask = 0, ctrl_mask = 0;
> +	u16 stat_mask, ctrl_mask;
>  
>  	if (probe)
>  		return 0;
>  
> -	if (HP_SUPR_RM(ctrl) && !ATTN_BUTTN(ctrl)) {
> +	ctrl_mask = PCI_EXP_SLTCTL_DLLSCE;
> +	stat_mask = PCI_EXP_SLTSTA_DLLSC;
> +
> +	if (!ATTN_BUTTN(ctrl)) {	/* as in pcie_enable_notification() */
>  		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
>  		stat_mask |= PCI_EXP_SLTSTA_PDC;
>  	}
> -	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
> -	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
>  
>  	pcie_write_cmd(ctrl, 0, ctrl_mask);
>  	if (pciehp_poll_mode)
> @@ -741,8 +742,6 @@ static inline void dbg_ctrl(struct controller *ctrl)
>  		  ATTN_LED(ctrl)   ? "yes" : "no");
>  	ctrl_info(ctrl, "  Power Indicator      : %3s\n",
>  		  PWR_LED(ctrl)    ? "yes" : "no");
> -	ctrl_info(ctrl, "  Hot-Plug Surprise    : %3s\n",
> -		  HP_SUPR_RM(ctrl) ? "yes" : "no");
>  	ctrl_info(ctrl, "  EMI Present          : %3s\n",
>  		  EMI(ctrl)        ? "yes" : "no");
>  	ctrl_info(ctrl, "  Command Completed    : %3s\n",
>