[PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Rajat Jain]

We need future link up events for hot-add, thus don't disable
the link permanently during device removal. Also, remove the static
functions that are now left unused.

Signed-off-by: Rajat Jain <rajatjain@juniper.net>
Signed-off-by: Guenter Roeck <groeck@juniper.net>
---
v3: no change, created by splitting the patch v2 [2/4]
v2: (non existent)
v1: (non existent)

 drivers/pci/hotplug/pciehp_hpc.c |   18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index b45b568..ab12555 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -278,11 +278,6 @@ static void pcie_wait_link_active(struct controller *ctrl)
 	__pcie_wait_link_active(ctrl, true);
 }
 
-static void pcie_wait_link_not_active(struct controller *ctrl)
-{
-	__pcie_wait_link_active(ctrl, false);
-}
-
 static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
 {
 	u32 l;
@@ -383,11 +378,6 @@ static int pciehp_link_enable(struct controller *ctrl)
 	return __pciehp_link_set(ctrl, true);
 }
 
-static int pciehp_link_disable(struct controller *ctrl)
-{
-	return __pciehp_link_set(ctrl, false);
-}
-
 int pciehp_get_attention_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
@@ -620,14 +610,6 @@ int pciehp_power_off_slot(struct slot * slot)
 	u16 cmd_mask;
 	int retval;
 
-	/* Disable the link at first */
-	pciehp_link_disable(ctrl);
-	/* wait the link is down */
-	if (ctrl->link_active_reporting)
-		pcie_wait_link_not_active(ctrl);
-	else
-		msleep(1000);
-
 	slot_cmd = POWER_OFF;
 	cmd_mask = PCI_EXP_SLTCTL_PCC;
 	retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);
-- 
1.7.9.5

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Bjorn Helgaas]

[+cc yinghai@kernel.org (seems to be Yinghai's preferred email]

On Tue, Dec 17, 2013 at 12:06:05PM -0800, Rajat Jain wrote:
> We need future link up events for hot-add, thus don't disable
> the link permanently during device removal. Also, remove the static
> functions that are now left unused.

The changelog should mention that this reverts part of 2debd9289997 ("PCI:
pciehp: Disable/enable link during slot power off/on").

Yinghai, can you tell us whether this is an issue on your systems?

> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> ---
> v3: no change, created by splitting the patch v2 [2/4]
> v2: (non existent)
> v1: (non existent)
> 
>  drivers/pci/hotplug/pciehp_hpc.c |   18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index b45b568..ab12555 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -278,11 +278,6 @@ static void pcie_wait_link_active(struct controller *ctrl)
>  	__pcie_wait_link_active(ctrl, true);
>  }
>  
> -static void pcie_wait_link_not_active(struct controller *ctrl)
> -{
> -	__pcie_wait_link_active(ctrl, false);
> -}
> -
>  static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>  {
>  	u32 l;
> @@ -383,11 +378,6 @@ static int pciehp_link_enable(struct controller *ctrl)
>  	return __pciehp_link_set(ctrl, true);
>  }
>  
> -static int pciehp_link_disable(struct controller *ctrl)
> -{
> -	return __pciehp_link_set(ctrl, false);
> -}
> -
>  int pciehp_get_attention_status(struct slot *slot, u8 *status)
>  {
>  	struct controller *ctrl = slot->ctrl;
> @@ -620,14 +610,6 @@ int pciehp_power_off_slot(struct slot * slot)
>  	u16 cmd_mask;
>  	int retval;
>  
> -	/* Disable the link at first */
> -	pciehp_link_disable(ctrl);
> -	/* wait the link is down */
> -	if (ctrl->link_active_reporting)
> -		pcie_wait_link_not_active(ctrl);
> -	else
> -		msleep(1000);
> -
>  	slot_cmd = POWER_OFF;
>  	cmd_mask = PCI_EXP_SLTCTL_PCC;
>  	retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);
> -- 
> 1.7.9.5
> 

RE: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Rajat Jain]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmpvcm4gSGVsZ2FhcyBb
bWFpbHRvOmJoZWxnYWFzQGdvb2dsZS5jb21dDQo+IA0KPiBbK2NjIHlpbmdoYWlAa2VybmVsLm9y
ZyAoc2VlbXMgdG8gYmUgWWluZ2hhaSdzIHByZWZlcnJlZCBlbWFpbF0NCj4gDQo+IE9uIFR1ZSwg
RGVjIDE3LCAyMDEzIGF0IDEyOjA2OjA1UE0gLTA4MDAsIFJhamF0IEphaW4gd3JvdGU6DQo+ID4g
V2UgbmVlZCBmdXR1cmUgbGluayB1cCBldmVudHMgZm9yIGhvdC1hZGQsIHRodXMgZG9uJ3QgZGlz
YWJsZSB0aGUgbGluaw0KPiA+IHBlcm1hbmVudGx5IGR1cmluZyBkZXZpY2UgcmVtb3ZhbC4gQWxz
bywgcmVtb3ZlIHRoZSBzdGF0aWMgZnVuY3Rpb25zDQo+ID4gdGhhdCBhcmUgbm93IGxlZnQgdW51
c2VkLg0KPiANCj4gVGhlIGNoYW5nZWxvZyBzaG91bGQgbWVudGlvbiB0aGF0IHRoaXMgcmV2ZXJ0
cyBwYXJ0IG9mIDJkZWJkOTI4OTk5Nw0KPiAoIlBDSToNCj4gcGNpZWhwOiBEaXNhYmxlL2VuYWJs
ZSBsaW5rIGR1cmluZyBzbG90IHBvd2VyIG9mZi9vbiIpLg0KDQpTdXJlLCBJJ2xsIGFkZCB0aGF0
LiANCg0KPiANCj4gWWluZ2hhaSwgY2FuIHlvdSB0ZWxsIHVzIHdoZXRoZXIgdGhpcyBpcyBhbiBp
c3N1ZSBvbiB5b3VyIHN5c3RlbXM/DQo+DQoNClRoYW5rcyBpbiBhZHZhbmNlIFlpbmdoYWkuIA0K
DQpBY3R1YWxseSBJIGRpZCBub3QgdW5kZXJzdGFuZCB0aGUgb3JpZ2luYWwgcHJvYmxlbSBhbmQg
dGhlIHNvbHV0aW9uIGluIHRoZSBmaXJzdA0KcGxhY2UgKHNvIEkgYWxzbyBkbyBub3QgdW5kZXJz
dGFuZCBob3cgbWlnaHQgZGlzYWJsaW5nIG9mIHByZXNlbmNlIGRldGVjdCBub3RpZmljYXRpb24N
CmhlbHApLiBJZiB5b3UgY2FuIGdpdmUgbW9yZSBkZXRhaWxzIG9uIHRoZSBvcmlnaW5hbCBwcm9i
bGVtIHRoYXQgc2hhbGwgYmUgZ3JlYXQuIEhlcmUNCmlzIHdoYXQgSSB1bmRlcnN0b29kIGZyb20g
dGhlIGNvbW1pdCBsb2c6DQoNClRoZSBiZWxpZXZlIHRoZSBIVyBsb29rcyBsaWtlIHRoaXM6DQoN
ClBDSWUgcG9ydCA8LS0tLT4gUmVwZWF0ZXIgPC0tLS0+IERldmljZS4NCg0KQW4gaW4gYWRkaXRp
b24gdGhlcmUgaXMgdGhlIHByZXNlbmNlIGRldGVjdCBwaW4gdGhhdCBpcyBjb25uZWN0ZWQgZGly
ZWN0bHkgZnJvbQ0KVGhlIHBvcnQgdG8gdGhlIGRldmljZS4gTm93LCB3aGVuIHRoZSBkZXZpY2Ug
aXMgcGx1Z2dlZCBvdXQsIHRoZSBwaW4gaW5kaWNhdGVzDQpObyBwcmVzZW5jZS4gQnV0IGFyZSB5
b3Ugc2F5aW5nIHRoZSBQQ0llIGxpbmsgZnJvbSBwb3J0IHRvIHJlcGVhdGVyIGlzIHN0aWxsIHVw
Pw0KDQpQYXJ0IG9mIGxvZzoNCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0K
ICAgIC4uLg0KICAgIEl0IHR1cm5zIG91dCB0aGUgcm9vdCBjb21wbGV4IGlzIGNvbnRpbnVhbGx5
IHRyeWluZyB0byB0cmFpbiB0aGUgbGluayB0bw0KICAgIHRoZSByZXBlYXRlciBiZWNhdXNlIHRo
ZSByZXBlYXRlciBoYXMgbm90IGJlZW4gcmVzZXQuDQoNCiAgICBUaGlzIHBhdGNoIHdpbGwgZGlz
YWJsZSB0aGUgbGluayBhdCByZW1vdmFsIHRpbWUgdG8gYWxsb3cgdGhlIHJlcGVhdGVyDQogICAg
dG8gYmUgcmVzZXQgcHJvcGVybHkuDQogICAgLi4uDQo9PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT0NCg0KMSkgSSBkaWQgbm90IHVuZGVyc3RhbmQgd2h5IHdvdWxkIHBvcnQgdHJ5
IHRvIHJldHJhaW4gdG8gdGhlIGxpbmsgaW4gZWl0aGVyIGNhc2VzIC0NCldoZXRoZXIgdGhlIGxp
bmsgdG8gdGhlIHJlcGVhdGVyIGlzIHVwIG9yIG5vdD8NCg0KMikgV2hlbiBubyBsaW5rIGlzIHNl
ZW4sIGlzIFRIQVQgd2hhdCBjYXVzZXMgcmVwZWF0ZXIgdG8gZ28gZG93biBhbmQgaGVuY2Ugc29s
dmUgdGhlDQpQcm9ibGVtPw0KDQozKSBJJ2QgZXhwZWN0IHlvdSdkIGNvbnRpbnVvdXNseSBnZXQg
ImFkYXB0ZXIgbm90IHByZXNlbnQiIG1lc3NhZ2VzIGlmIHNvbWUgaG93DQpUaGUgZHJpdmVyIHdh
cyB0cnlpbmcgdG8gYWRkIHRoZSBkZXZpY2UuIEJ1dCBJIGRpZCBub3QgdW5kZXJzdGFuZCB3aGVy
ZSBkaWQNCiJhZGFwdGVyIHByZXNlbnQiIG1lc3NhZ2VzIGNhbWUgZnJvbT8NCg0KU29ycnksIGJ1
dCBJIGd1ZXNzIEkgYW0gdG90YWxseSBjb25mdXNlZCBub3cuIEknbGwgcHJvYmFibHkgZ28gdG8g
c2xlZXAgOi0pIA0KDQpKdXN0IHRyeWluZyB0byB1bmRlcnN0YW5kLg0KDQpUaGFua3MsDQoNClJh
amF0DQoNCj4gPiBTaWduZWQtb2ZmLWJ5OiBSYWphdCBKYWluIDxyYWphdGphaW5AanVuaXBlci5u
ZXQ+DQo+ID4gU2lnbmVkLW9mZi1ieTogR3VlbnRlciBSb2VjayA8Z3JvZWNrQGp1bmlwZXIubmV0
Pg0KPiA+IC0tLQ0KPiA+IHYzOiBubyBjaGFuZ2UsIGNyZWF0ZWQgYnkgc3BsaXR0aW5nIHRoZSBw
YXRjaCB2MiBbMi80XQ0KPiA+IHYyOiAobm9uIGV4aXN0ZW50KQ0KPiA+IHYxOiAobm9uIGV4aXN0
ZW50KQ0KPiA+DQo+ID4gIGRyaXZlcnMvcGNpL2hvdHBsdWcvcGNpZWhwX2hwYy5jIHwgICAxOCAt
LS0tLS0tLS0tLS0tLS0tLS0NCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDE4IGRlbGV0aW9ucygtKQ0K
PiA+DQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL2hvdHBsdWcvcGNpZWhwX2hwYy5jDQo+
ID4gYi9kcml2ZXJzL3BjaS9ob3RwbHVnL3BjaWVocF9ocGMuYw0KPiA+IGluZGV4IGI0NWI1Njgu
LmFiMTI1NTUgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9wY2kvaG90cGx1Zy9wY2llaHBfaHBj
LmMNCj4gPiArKysgYi9kcml2ZXJzL3BjaS9ob3RwbHVnL3BjaWVocF9ocGMuYw0KPiA+IEBAIC0y
NzgsMTEgKzI3OCw2IEBAIHN0YXRpYyB2b2lkIHBjaWVfd2FpdF9saW5rX2FjdGl2ZShzdHJ1Y3QN
Cj4gY29udHJvbGxlciAqY3RybCkNCj4gPiAgCV9fcGNpZV93YWl0X2xpbmtfYWN0aXZlKGN0cmws
IHRydWUpOyAgfQ0KPiA+DQo+ID4gLXN0YXRpYyB2b2lkIHBjaWVfd2FpdF9saW5rX25vdF9hY3Rp
dmUoc3RydWN0IGNvbnRyb2xsZXIgKmN0cmwpIC17DQo+ID4gLQlfX3BjaWVfd2FpdF9saW5rX2Fj
dGl2ZShjdHJsLCBmYWxzZSk7DQo+ID4gLX0NCj4gPiAtDQo+ID4gIHN0YXRpYyBib29sIHBjaV9i
dXNfY2hlY2tfZGV2KHN0cnVjdCBwY2lfYnVzICpidXMsIGludCBkZXZmbikgIHsNCj4gPiAgCXUz
MiBsOw0KPiA+IEBAIC0zODMsMTEgKzM3OCw2IEBAIHN0YXRpYyBpbnQgcGNpZWhwX2xpbmtfZW5h
YmxlKHN0cnVjdCBjb250cm9sbGVyDQo+ICpjdHJsKQ0KPiA+ICAJcmV0dXJuIF9fcGNpZWhwX2xp
bmtfc2V0KGN0cmwsIHRydWUpOyAgfQ0KPiA+DQo+ID4gLXN0YXRpYyBpbnQgcGNpZWhwX2xpbmtf
ZGlzYWJsZShzdHJ1Y3QgY29udHJvbGxlciAqY3RybCkgLXsNCj4gPiAtCXJldHVybiBfX3BjaWVo
cF9saW5rX3NldChjdHJsLCBmYWxzZSk7DQo+ID4gLX0NCj4gPiAtDQo+ID4gIGludCBwY2llaHBf
Z2V0X2F0dGVudGlvbl9zdGF0dXMoc3RydWN0IHNsb3QgKnNsb3QsIHU4ICpzdGF0dXMpICB7DQo+
ID4gIAlzdHJ1Y3QgY29udHJvbGxlciAqY3RybCA9IHNsb3QtPmN0cmw7IEBAIC02MjAsMTQgKzYx
MCw2IEBAIGludA0KPiA+IHBjaWVocF9wb3dlcl9vZmZfc2xvdChzdHJ1Y3Qgc2xvdCAqIHNsb3Qp
DQo+ID4gIAl1MTYgY21kX21hc2s7DQo+ID4gIAlpbnQgcmV0dmFsOw0KPiA+DQo+ID4gLQkvKiBE
aXNhYmxlIHRoZSBsaW5rIGF0IGZpcnN0ICovDQo+ID4gLQlwY2llaHBfbGlua19kaXNhYmxlKGN0
cmwpOw0KPiA+IC0JLyogd2FpdCB0aGUgbGluayBpcyBkb3duICovDQo+ID4gLQlpZiAoY3RybC0+
bGlua19hY3RpdmVfcmVwb3J0aW5nKQ0KPiA+IC0JCXBjaWVfd2FpdF9saW5rX25vdF9hY3RpdmUo
Y3RybCk7DQo+ID4gLQllbHNlDQo+ID4gLQkJbXNsZWVwKDEwMDApOw0KPiA+IC0NCj4gPiAgCXNs
b3RfY21kID0gUE9XRVJfT0ZGOw0KPiA+ICAJY21kX21hc2sgPSBQQ0lfRVhQX1NMVENUTF9QQ0M7
DQo+ID4gIAlyZXR2YWwgPSBwY2llX3dyaXRlX2NtZChjdHJsLCBzbG90X2NtZCwgY21kX21hc2sp
Ow0KPiA+IC0tDQo+ID4gMS43LjkuNQ0KPiA+DQo+IA0KDQo=

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Guenter Roeck]

On Wed, Dec 18, 2013 at 03:20:58AM +0000, Rajat Jain wrote:
> 
> 
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> > 
> > [+cc yinghai@kernel.org (seems to be Yinghai's preferred email]
> > 
> > On Tue, Dec 17, 2013 at 12:06:05PM -0800, Rajat Jain wrote:
> > > We need future link up events for hot-add, thus don't disable the link
> > > permanently during device removal. Also, remove the static functions
> > > that are now left unused.
> > 
> > The changelog should mention that this reverts part of 2debd9289997
> > ("PCI:
> > pciehp: Disable/enable link during slot power off/on").
> 
> Sure, I'll add that. 
> 
> > 
> > Yinghai, can you tell us whether this is an issue on your systems?
> >
> 
> Thanks in advance Yinghai. 
> 
> Actually I did not understand the original problem and the solution in the first
> place (so I also do not understand how might disabling of presence detect notification
> help). If you can give more details on the original problem that shall be great. Here
> is what I understood from the commit log:
> 
> The believe the HW looks like this:
> 
> PCIe port <----> Repeater <----> Device.
> 
> An in addition there is the presence detect pin that is connected directly from
> The port to the device. Now, when the device is plugged out, the pin indicates
> No presence. But are you saying the PCIe link from port to repeater is still up?
> 
> Part of log:
> ====================================
>     ...
>     It turns out the root complex is continually trying to train the link to
>     the repeater because the repeater has not been reset.
> 
>     This patch will disable the link at removal time to allow the repeater
>     to be reset properly.
>     ...
> ====================================
> 
> 1) I did not understand why would port try to retrain to the link in either cases -
> Whether the link to the repeater is up or not?
> 
> 2) When no link is seen, is THAT what causes repeater to go down and hence solve the
> Problem?
> 
> 3) I'd expect you'd continuously get "adapter not present" messages if some how
> The driver was trying to add the device. But I did not understand where did
> "adapter present" messages came from?
> 
> Sorry, but I guess I am totally confused now. I'll probably go to sleep :-) 
> 

What, that early ? :-)

I think we may have a similar setup on some of our boards. Maybe we can
reproduce the situation (after we understand what exactly it is and how to
trigger it).

Related to this patch set, I learned today that our Space product uses mpt2sas.
Just in case we need one for testing reset functionality.

Guenter

RE: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Rajat Jain]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogR3VlbnRlciBSb2VjayBb
bWFpbHRvOmdyb2VjazdAZ21haWwuY29tXSBPbiBCZWhhbGYgT2YgR3VlbnRlcg0KPiBSb2Vjaw0K
PiBTZW50OiBUdWVzZGF5LCBEZWNlbWJlciAxNywgMjAxMyA3OjQwIFBNDQo+IFRvOiBSYWphdCBK
YWluDQo+IENjOiBCam9ybiBIZWxnYWFzOyBSYWphdCBKYWluOyBLZW5qaSBLYW5lc2hpZ2U7IEFs
ZXggV2lsbGlhbXNvbjsgWWlqaW5nDQo+IFdhbmc7IGxpbnV4LXBjaUB2Z2VyLmtlcm5lbC5vcmc7
IGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7DQo+IFlAamFzcGVyLmVzDQo+IFN1YmplY3Q6
IFJlOiBbUEFUQ0ggdjMgNC84XSBwY2llaHA6IERvbid0IGRpc2FibGUgdGhlIGxpbmsgcGVybWFu
ZW50bHksDQo+IGR1cmluZyByZW1vdmFsDQo+IA0KPiBPbiBXZWQsIERlYyAxOCwgMjAxMyBhdCAw
MzoyMDo1OEFNICswMDAwLCBSYWphdCBKYWluIHdyb3RlOg0KPiA+DQo+ID4NCj4gPiA+IC0tLS0t
T3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4gPiBGcm9tOiBCam9ybiBIZWxnYWFzIFttYWlsdG86
YmhlbGdhYXNAZ29vZ2xlLmNvbV0NCj4gPiA+DQo+ID4gPiBbK2NjIHlpbmdoYWlAa2VybmVsLm9y
ZyAoc2VlbXMgdG8gYmUgWWluZ2hhaSdzIHByZWZlcnJlZCBlbWFpbF0NCj4gPiA+DQo+ID4gPiBP
biBUdWUsIERlYyAxNywgMjAxMyBhdCAxMjowNjowNVBNIC0wODAwLCBSYWphdCBKYWluIHdyb3Rl
Og0KPiA+ID4gPiBXZSBuZWVkIGZ1dHVyZSBsaW5rIHVwIGV2ZW50cyBmb3IgaG90LWFkZCwgdGh1
cyBkb24ndCBkaXNhYmxlIHRoZQ0KPiA+ID4gPiBsaW5rIHBlcm1hbmVudGx5IGR1cmluZyBkZXZp
Y2UgcmVtb3ZhbC4gQWxzbywgcmVtb3ZlIHRoZSBzdGF0aWMNCj4gPiA+ID4gZnVuY3Rpb25zIHRo
YXQgYXJlIG5vdyBsZWZ0IHVudXNlZC4NCj4gPiA+DQo+ID4gPiBUaGUgY2hhbmdlbG9nIHNob3Vs
ZCBtZW50aW9uIHRoYXQgdGhpcyByZXZlcnRzIHBhcnQgb2YgMmRlYmQ5Mjg5OTk3DQo+ID4gPiAo
IlBDSToNCj4gPiA+IHBjaWVocDogRGlzYWJsZS9lbmFibGUgbGluayBkdXJpbmcgc2xvdCBwb3dl
ciBvZmYvb24iKS4NCj4gPg0KPiA+IFN1cmUsIEknbGwgYWRkIHRoYXQuDQo+ID4NCj4gPiA+DQo+
ID4gPiBZaW5naGFpLCBjYW4geW91IHRlbGwgdXMgd2hldGhlciB0aGlzIGlzIGFuIGlzc3VlIG9u
IHlvdXIgc3lzdGVtcz8NCj4gPiA+DQo+ID4NCj4gPiBUaGFua3MgaW4gYWR2YW5jZSBZaW5naGFp
Lg0KPiA+DQo+ID4gQWN0dWFsbHkgSSBkaWQgbm90IHVuZGVyc3RhbmQgdGhlIG9yaWdpbmFsIHBy
b2JsZW0gYW5kIHRoZSBzb2x1dGlvbiBpbg0KPiA+IHRoZSBmaXJzdCBwbGFjZSAoc28gSSBhbHNv
IGRvIG5vdCB1bmRlcnN0YW5kIGhvdyBtaWdodCBkaXNhYmxpbmcgb2YNCj4gPiBwcmVzZW5jZSBk
ZXRlY3Qgbm90aWZpY2F0aW9uIGhlbHApLiBJZiB5b3UgY2FuIGdpdmUgbW9yZSBkZXRhaWxzIG9u
DQo+ID4gdGhlIG9yaWdpbmFsIHByb2JsZW0gdGhhdCBzaGFsbCBiZSBncmVhdC4gSGVyZSBpcyB3
aGF0IEkgdW5kZXJzdG9vZA0KPiBmcm9tIHRoZSBjb21taXQgbG9nOg0KPiA+DQo+ID4gVGhlIGJl
bGlldmUgdGhlIEhXIGxvb2tzIGxpa2UgdGhpczoNCj4gPg0KPiA+IFBDSWUgcG9ydCA8LS0tLT4g
UmVwZWF0ZXIgPC0tLS0+IERldmljZS4NCj4gPg0KPiA+IEFuIGluIGFkZGl0aW9uIHRoZXJlIGlz
IHRoZSBwcmVzZW5jZSBkZXRlY3QgcGluIHRoYXQgaXMgY29ubmVjdGVkDQo+ID4gZGlyZWN0bHkg
ZnJvbSBUaGUgcG9ydCB0byB0aGUgZGV2aWNlLiBOb3csIHdoZW4gdGhlIGRldmljZSBpcyBwbHVn
Z2VkDQo+ID4gb3V0LCB0aGUgcGluIGluZGljYXRlcyBObyBwcmVzZW5jZS4gQnV0IGFyZSB5b3Ug
c2F5aW5nIHRoZSBQQ0llIGxpbmsNCj4gZnJvbSBwb3J0IHRvIHJlcGVhdGVyIGlzIHN0aWxsIHVw
Pw0KPiA+DQo+ID4gUGFydCBvZiBsb2c6DQo+ID4gPT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09DQo+ID4gICAgIC4uLg0KPiA+ICAgICBJdCB0dXJucyBvdXQgdGhlIHJvb3QgY29t
cGxleCBpcyBjb250aW51YWxseSB0cnlpbmcgdG8gdHJhaW4gdGhlDQo+IGxpbmsgdG8NCj4gPiAg
ICAgdGhlIHJlcGVhdGVyIGJlY2F1c2UgdGhlIHJlcGVhdGVyIGhhcyBub3QgYmVlbiByZXNldC4N
Cj4gPg0KPiA+ICAgICBUaGlzIHBhdGNoIHdpbGwgZGlzYWJsZSB0aGUgbGluayBhdCByZW1vdmFs
IHRpbWUgdG8gYWxsb3cgdGhlDQo+IHJlcGVhdGVyDQo+ID4gICAgIHRvIGJlIHJlc2V0IHByb3Bl
cmx5Lg0KPiA+ICAgICAuLi4NCj4gPiA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT0NCj4gPg0KPiA+IDEpIEkgZGlkIG5vdCB1bmRlcnN0YW5kIHdoeSB3b3VsZCBwb3J0IHRyeSB0
byByZXRyYWluIHRvIHRoZSBsaW5rIGluDQo+ID4gZWl0aGVyIGNhc2VzIC0gV2hldGhlciB0aGUg
bGluayB0byB0aGUgcmVwZWF0ZXIgaXMgdXAgb3Igbm90Pw0KPiA+DQo+ID4gMikgV2hlbiBubyBs
aW5rIGlzIHNlZW4sIGlzIFRIQVQgd2hhdCBjYXVzZXMgcmVwZWF0ZXIgdG8gZ28gZG93biBhbmQN
Cj4gPiBoZW5jZSBzb2x2ZSB0aGUgUHJvYmxlbT8NCj4gPg0KPiA+IDMpIEknZCBleHBlY3QgeW91
J2QgY29udGludW91c2x5IGdldCAiYWRhcHRlciBub3QgcHJlc2VudCIgbWVzc2FnZXMgaWYNCj4g
PiBzb21lIGhvdyBUaGUgZHJpdmVyIHdhcyB0cnlpbmcgdG8gYWRkIHRoZSBkZXZpY2UuIEJ1dCBJ
IGRpZCBub3QNCj4gPiB1bmRlcnN0YW5kIHdoZXJlIGRpZCAiYWRhcHRlciBwcmVzZW50IiBtZXNz
YWdlcyBjYW1lIGZyb20/DQo+ID4NCj4gPiBTb3JyeSwgYnV0IEkgZ3Vlc3MgSSBhbSB0b3RhbGx5
IGNvbmZ1c2VkIG5vdy4gSSdsbCBwcm9iYWJseSBnbyB0bw0KPiA+IHNsZWVwIDotKQ0KPiA+DQo+
IA0KPiBXaGF0LCB0aGF0IGVhcmx5ID8gOi0pDQoNCkkgd2FzIHVwIHVudGlsIDMgQU0gbGFzdCBu
aWdodC4gImdpdCIgKG9yIHRoZSBpZ25vcmFuY2Ugb2YgaXQpIGtlZXBzIG1lIGJ1c3kgSSBndWVz
cyA6LSkNCg0KPiANCj4gSSB0aGluayB3ZSBtYXkgaGF2ZSBhIHNpbWlsYXIgc2V0dXAgb24gc29t
ZSBvZiBvdXIgYm9hcmRzLiBNYXliZSB3ZSBjYW4NCj4gcmVwcm9kdWNlIHRoZSBzaXR1YXRpb24g
KGFmdGVyIHdlIHVuZGVyc3RhbmQgd2hhdCBleGFjdGx5IGl0IGlzIGFuZCBob3cNCj4gdG8gdHJp
Z2dlciBpdCkuDQo+IA0KPiBSZWxhdGVkIHRvIHRoaXMgcGF0Y2ggc2V0LCBJIGxlYXJuZWQgdG9k
YXkgdGhhdCBvdXIgU3BhY2UgcHJvZHVjdCB1c2VzDQo+IG1wdDJzYXMuDQo+IEp1c3QgaW4gY2Fz
ZSB3ZSBuZWVkIG9uZSBmb3IgdGVzdGluZyByZXNldCBmdW5jdGlvbmFsaXR5Lg0KPiANCj4gR3Vl
bnRlcg0KPiANCg0K

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Yinghai Lu]

On Tue, Dec 17, 2013 at 7:20 PM, Rajat Jain <rajatjain@juniper.net> wrote:
>
> Actually I did not understand the original problem and the solution in the first
> place (so I also do not understand how might disabling of presence detect notification
> help). If you can give more details on the original problem that shall be great. Here
> is what I understood from the commit log:
>
> The believe the HW looks like this:
>
> PCIe port <----> Repeater <----> Device.
>
> An in addition there is the presence detect pin that is connected directly from
> The port to the device. Now, when the device is plugged out, the pin indicates
> No presence. But are you saying the PCIe link from port to repeater is still up?

After the card is removed from the slot.

PCIe port try to retrain the link to repeater, like the link will keep
up and down.

so the presence bit will keep report one card present and not present.
that present bit should be OR of inband input and outband input.
We check the outband input and it always report correctly.

According to HW guys and Intel, that should be bug of repeater.

Disable the link from pcie to repeater, likely to reset the repeater....

Thanks

Yinghai

RE: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Rajat Jain]

DQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IHlobHUua2VybmVsQGdtYWls
LmNvbSBbbWFpbHRvOnlobHUua2VybmVsQGdtYWlsLmNvbV0gT24gQmVoYWxmIE9mDQo+IA0KPiBP
biBUdWUsIERlYyAxNywgMjAxMyBhdCA3OjIwIFBNLCBSYWphdCBKYWluIDxyYWphdGphaW5AanVu
aXBlci5uZXQ+DQo+IHdyb3RlOg0KPiA+DQo+ID4gQWN0dWFsbHkgSSBkaWQgbm90IHVuZGVyc3Rh
bmQgdGhlIG9yaWdpbmFsIHByb2JsZW0gYW5kIHRoZSBzb2x1dGlvbiBpbg0KPiA+IHRoZSBmaXJz
dCBwbGFjZSAoc28gSSBhbHNvIGRvIG5vdCB1bmRlcnN0YW5kIGhvdyBtaWdodCBkaXNhYmxpbmcg
b2YNCj4gPiBwcmVzZW5jZSBkZXRlY3Qgbm90aWZpY2F0aW9uIGhlbHApLiBJZiB5b3UgY2FuIGdp
dmUgbW9yZSBkZXRhaWxzIG9uDQo+ID4gdGhlIG9yaWdpbmFsIHByb2JsZW0gdGhhdCBzaGFsbCBi
ZSBncmVhdC4gSGVyZSBpcyB3aGF0IEkgdW5kZXJzdG9vZA0KPiBmcm9tIHRoZSBjb21taXQgbG9n
Og0KPiA+DQo+ID4gVGhlIGJlbGlldmUgdGhlIEhXIGxvb2tzIGxpa2UgdGhpczoNCj4gPg0KPiA+
IFBDSWUgcG9ydCA8LS0tLT4gUmVwZWF0ZXIgPC0tLS0+IERldmljZS4NCj4gPg0KPiA+IEFuIGlu
IGFkZGl0aW9uIHRoZXJlIGlzIHRoZSBwcmVzZW5jZSBkZXRlY3QgcGluIHRoYXQgaXMgY29ubmVj
dGVkDQo+ID4gZGlyZWN0bHkgZnJvbSBUaGUgcG9ydCB0byB0aGUgZGV2aWNlLiBOb3csIHdoZW4g
dGhlIGRldmljZSBpcyBwbHVnZ2VkDQo+ID4gb3V0LCB0aGUgcGluIGluZGljYXRlcyBObyBwcmVz
ZW5jZS4gQnV0IGFyZSB5b3Ugc2F5aW5nIHRoZSBQQ0llIGxpbmsNCj4gZnJvbSBwb3J0IHRvIHJl
cGVhdGVyIGlzIHN0aWxsIHVwPw0KPiANCj4gQWZ0ZXIgdGhlIGNhcmQgaXMgcmVtb3ZlZCBmcm9t
IHRoZSBzbG90Lg0KPiANCj4gUENJZSBwb3J0IHRyeSB0byByZXRyYWluIHRoZSBsaW5rIHRvIHJl
cGVhdGVyLCBsaWtlIHRoZSBsaW5rIHdpbGwga2VlcA0KPiB1cCBhbmQgZG93bi4NCg0KSSBzdGls
bCBkaWQgbm90IHVuZGVyc3RhbmQgdGhlIHdoeSB3b3VsZCB0aGUgUENJZSBwb3J0IHRyeSB0byBk
byBzby4gQXJlIHlvdQ0KU2F5aW5nIHRoZSByZXBlYXRlciBrZWVwcyBvbiBmbGFwcGluZyB0aGUg
bGluaz8gTmV2ZXJ0aGVsZXNzLCBJIGFzc3VtZSBpdCBpcw0KZHVlIHRvIHNvbWUgcmVwZWF0ZXIg
YnVnICh0aGF0IHlvdSBtZW50aW9uIGJlbG93KSB0aGF0IEkgZG8gbm90IG5lZWQgdG8NCnVuZGVy
c3RhbmQuDQoNCj4gDQo+IHNvIHRoZSBwcmVzZW5jZSBiaXQgd2lsbCBrZWVwIHJlcG9ydCBvbmUg
Y2FyZCBwcmVzZW50IGFuZCBub3QgcHJlc2VudC4NCj4gdGhhdCBwcmVzZW50IGJpdCBzaG91bGQg
YmUgT1Igb2YgaW5iYW5kIGlucHV0IGFuZCBvdXRiYW5kIGlucHV0Lg0KPiBXZSBjaGVjayB0aGUg
b3V0YmFuZCBpbnB1dCBhbmQgaXQgYWx3YXlzIHJlcG9ydCBjb3JyZWN0bHkuDQoNCkp1c3QgYSBz
dWdnZXN0aW9uIGZvciBhIHdvcmsgYXJvdW5kLiBJIGNoZWNrZWQgdGhlIG1hbnVhbCBvZiB0aGUg
UENJZSBzd2l0Y2gNCndlIHVzZSBpbiBvdXIgc3lzdGVtIChJRFQgODlIUEVTNDhIMTJHMiksIGFu
ZCBpdCBpcyBwb3NzaWJsZSBhdCBsZWFzdCBpbiB0aGlzDQpzd2l0Y2ggdG8gY29udHJvbCB3aGV0
aGVyIG9yIG5vdCBpbi1iYW5kIGlzIGNvbnNpZGVyZWQuIEFuIGludGVybmFsIGNvbmZpZ3VyYXRp
b24NCnJlZ2lzdGVyIHNheXM6DQoNClByZXNlbmNlIERldGVjdCBDb250cm9sLiBUaGlzIGZpZWxk
IGNvbnRyb2xzIHRoZSBtYW5uZXIgaW4gd2hpY2gNCnByZXNlbmNlIG9mIGFuIGFkYXB0ZXIgaW4g
YSBzbG90IGlzIHJlcG9ydGVkIHRvIHRoZSBob3QtcGx1ZyBjb250cm9sbGVyDQphc3NvY2lhdGVk
IHdpdGggYSBkb3duc3RyZWFtIHN3aXRjaCBwb3J0Lg0KMHgwIC0gKGJvdGgpIFByZXNlbmNlIG9m
IGFuIGFkYXB0ZXIgaW4gdGhlIHNsb3QgaXMgcmVwb3J0ZWQgYXMgdGhlDQpsb2dpY2FsIOKAnE9S
4oCdIG9mIHRoZSByZWNlaXZlciBkZXRlY3QgbWVjaGFuaXNtIGFuZCB0aGUgaG90cGx1Zw0KcHJl
c2VuY2UgZGV0ZWN0IGlucHV0IChQeFBETikuDQoweDEgLSAoc2lnbmFsKSBQcmVzZW5jZSBvZiBh
biBhZGFwdGVyIGluIHRoZSBzbG90IGlzIHJlcG9ydGVkIGFzIHRoZQ0Kc3RhdGUgb2YgdGhlIGhv
dC1wbHVnIHByZXNlbmNlIGRldGVjdCBpbnB1dCAoUHhQRE4pLg0KMHgyIC0gKGFsd2F5cykgV2hl
biBzZWxlY3RlZCwgdGhpcyBtb2RlIGFsd2F5cyBpbmZvcm1zIHRoZSBob3RwbHVnDQpjb250cm9s
bGVyIHRoYXQgYW4gYWRhcHRlciBpcyBwcmVzZW50Lg0KMHgzIC0gKG5ldmVyKSBXaGVuIHNlbGVj
dGVkLCB0aGlzIG1vZGUgYWx3YXlzIGluZm9ybXMgdGhlIGhvdHBsdWcNCmNvbnRyb2xsZXIgdGhh
dCBhbiBhZGFwdGVyIGlzIG5vdCBwcmVzZW50Lg0KDQpNYXkgYmUgc29tZSBzaW1pbGFyIGNvbmZp
ZyBtYXkgYmUgcHJlc2VudCBpbiB0aGUgc3dpdGNoIG9yIHRoZSBDUFUgdGhhdCB5b3UgdXNlLg0K
DQpKdXN0IGEgcmFuZG9tIHRob3VnaHQuDQoNCj4gDQo+IEFjY29yZGluZyB0byBIVyBndXlzIGFu
ZCBJbnRlbCwgdGhhdCBzaG91bGQgYmUgYnVnIG9mIHJlcGVhdGVyLg0KDQpXZWxsLCBpbiB0aGF0
IGNhc2UgSSBkb3VidCBpZiB0aGUgcGF0Y2ggd2lsbCBzb2x2ZSB0aGUgcHJvYmxlbS4gSSB0aGlu
aw0KTW9zdCBsaWtlbHkgdGhlICJjYXJkIHByZXNlbnQgLyBub3QgcHJlc2VudCIgbWVzc2FnZXMg
bWF5IGJlIHJlcGxhY2VkDQpCeSAibGluay11cCAvIGxpbmstZG93biIgbWVzc2FnZXMuIEJ1dCBJ
J2QgYXBwcmVjaWF0ZSB5b3VyIHRlc3RpbmcuDQoNClRoYW5rcywNCg0KUmFqYXQNCg0KPiANCj4g
RGlzYWJsZSB0aGUgbGluayBmcm9tIHBjaWUgdG8gcmVwZWF0ZXIsIGxpa2VseSB0byByZXNldCB0
aGUgcmVwZWF0ZXIuLi4uDQo+IA0KPiBUaGFua3MNCj4gDQo+IFlpbmdoYWkNCj4gDQoNCg==

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Tue, Dec 17, 2013 at 10:17 PM, Rajat Jain <rajatjain@juniper.net> wrote:
> Well, in that case I doubt if the patch will solve the problem. I think
> Most likely the "card present / not present" messages may be replaced
> By "link-up / link-down" messages. But I'd appreciate your testing.

I did have a debug patch at that time to report link status change.
And it did report link up and link down.

Thanks

Yinghai

[-- Attachment #2: link_change_report.patch --]
[-- Type: text/x-patch, Size: 2328 bytes --]

Subject: [PATCH] PCI, pciehp: Report pcie link state change

To see if there is some link state change.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/hotplug/pciehp_hpc.c |   29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
@@ -603,6 +603,25 @@ int pciehp_power_off_slot(struct slot *
 	return 0;
 }
 
+static u8 pciehp_handle_linkstate_change(struct slot *slot)
+{
+	struct controller *ctrl = slot->ctrl;
+	u16 lnk_status;
+	int retval;
+
+        /* LinkState Change */
+	ctrl_dbg(ctrl, "LinkState change\n");
+
+	retval = pciehp_readw(ctrl, PCI_EXP_LNKSTA, &lnk_status);
+	if (retval) {
+		ctrl_err(ctrl, "Cannot read LNKSTATUS register\n");
+		return 1;
+	}
+	ctrl_info(ctrl, "lnk_status = %x\n", lnk_status);
+
+	return 1;
+}
+
 static irqreturn_t pcie_isr(int irq, void *dev_id)
 {
 	struct controller *ctrl = (struct controller *)dev_id;
@@ -624,7 +643,7 @@ static irqreturn_t pcie_isr(int irq, voi
 
 		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
 			     PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
-			     PCI_EXP_SLTSTA_CC);
+			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
 		detected &= ~intr_loc;
 		intr_loc |= detected;
 		if (!intr_loc)
@@ -648,6 +667,10 @@ static irqreturn_t pcie_isr(int irq, voi
 	if (!(intr_loc & ~PCI_EXP_SLTSTA_CC))
 		return IRQ_HANDLED;
 
+	/* Check Link State Changed */
+	if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
+		pciehp_handle_linkstate_change(slot);
+
 	/* Check MRL Sensor Changed */
 	if (intr_loc & PCI_EXP_SLTSTA_MRLSC)
 		pciehp_handle_switch_change(slot);
@@ -689,10 +712,12 @@ int pcie_enable_notification(struct cont
 		cmd |= PCI_EXP_SLTCTL_MRLSCE;
 	if (!pciehp_poll_mode)
 		cmd |= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE;
+	cmd |= PCI_EXP_SLTCTL_DLLSCE;
 
 	mask = (PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_ABPE |
 		PCI_EXP_SLTCTL_MRLSCE | PCI_EXP_SLTCTL_PFDE |
-		PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE);
+		PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE |
+		PCI_EXP_SLTCTL_DLLSCE);
 
 	if (pcie_write_cmd(ctrl, cmd, mask)) {
 		ctrl_err(ctrl, "Cannot enable software notification\n");

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Rajat Jain]

Hello Bjorn,

Just checking on the fate of this patch set...

On Tue, Dec 17, 2013 at 5:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc yinghai@kernel.org (seems to be Yinghai's preferred email]
>
> On Tue, Dec 17, 2013 at 12:06:05PM -0800, Rajat Jain wrote:
>> We need future link up events for hot-add, thus don't disable
>> the link permanently during device removal. Also, remove the static
>> functions that are now left unused.
>
> The changelog should mention that this reverts part of 2debd9289997 ("PCI:
> pciehp: Disable/enable link during slot power off/on").

Sure. Do you want me to submit another patch set (bumping up the
version) with this change log, or you'd want to add this change log
while merging?

>
> Yinghai, can you tell us whether this is an issue on your systems?

As Yinghai confirms further down this thread, his issue was confirmed
by Intel to be a bug in the repeater chip.
----------------------------------
Yinghai writes:
> According to HW guys and Intel, that should be bug of repeater.
>
---------------------------------
 I don't know about the details of his scenario, except that when the
adapter was disabled the repeater kept on flapping the link up & down
(and hence disabling the link solved the problem then). Yinghai
couldn't test, but I believe with this patch even if we disable
presence detect interrupt, the "adapter present / no present" messages
would (rightly) convert to "Link Up / Link Down" messages (since the
repeater keeps on flapping the link).

Since it is a platform specific bug, I'm not sure what can be done to
remove those messages except may be reduce the verbosity? If you'd
like I could change all the INFO messages to DBG messages.

Please let me know how to proceed further on this. Also, did you get a
chance to look at the subsequent patches of this patch set, I was
wondering if you had any comments there?

Thanks,

Rajat

>
>> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
>> Signed-off-by: Guenter Roeck <groeck@juniper.net>
>> ---
>> v3: no change, created by splitting the patch v2 [2/4]
>> v2: (non existent)
>> v1: (non existent)
>>
>>  drivers/pci/hotplug/pciehp_hpc.c |   18 ------------------
>>  1 file changed, 18 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index b45b568..ab12555 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -278,11 +278,6 @@ static void pcie_wait_link_active(struct controller *ctrl)
>>       __pcie_wait_link_active(ctrl, true);
>>  }
>>
>> -static void pcie_wait_link_not_active(struct controller *ctrl)
>> -{
>> -     __pcie_wait_link_active(ctrl, false);
>> -}
>> -
>>  static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>>  {
>>       u32 l;
>> @@ -383,11 +378,6 @@ static int pciehp_link_enable(struct controller *ctrl)
>>       return __pciehp_link_set(ctrl, true);
>>  }
>>
>> -static int pciehp_link_disable(struct controller *ctrl)
>> -{
>> -     return __pciehp_link_set(ctrl, false);
>> -}
>> -
>>  int pciehp_get_attention_status(struct slot *slot, u8 *status)
>>  {
>>       struct controller *ctrl = slot->ctrl;
>> @@ -620,14 +610,6 @@ int pciehp_power_off_slot(struct slot * slot)
>>       u16 cmd_mask;
>>       int retval;
>>
>> -     /* Disable the link at first */
>> -     pciehp_link_disable(ctrl);
>> -     /* wait the link is down */
>> -     if (ctrl->link_active_reporting)
>> -             pcie_wait_link_not_active(ctrl);
>> -     else
>> -             msleep(1000);
>> ->>       slot_cmd = POWER_OFF;
>>       cmd_mask = PCI_EXP_SLTCTL_PCC;
>>       retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);
>> --
>> 1.7.9.5
>>

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Bjorn Helgaas]

On Sun, Jan 5, 2014 at 10:53 AM, Rajat Jain <rajatxjain@gmail.com> wrote:
> Hello Bjorn,
>
> Just checking on the fate of this patch set...
>
> On Tue, Dec 17, 2013 at 5:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc yinghai@kernel.org (seems to be Yinghai's preferred email]
>>
>> On Tue, Dec 17, 2013 at 12:06:05PM -0800, Rajat Jain wrote:
>>> We need future link up events for hot-add, thus don't disable
>>> the link permanently during device removal. Also, remove the static
>>> functions that are now left unused.
>>
>> The changelog should mention that this reverts part of 2debd9289997 ("PCI:
>> pciehp: Disable/enable link during slot power off/on").
>
> Sure. Do you want me to submit another patch set (bumping up the
> version) with this change log, or you'd want to add this change log
> while merging?
>
>>
>> Yinghai, can you tell us whether this is an issue on your systems?
>
> As Yinghai confirms further down this thread, his issue was confirmed
> by Intel to be a bug in the repeater chip.
> ----------------------------------
> Yinghai writes:
>> According to HW guys and Intel, that should be bug of repeater.
>>
> ---------------------------------
>  I don't know about the details of his scenario, except that when the
> adapter was disabled the repeater kept on flapping the link up & down
> (and hence disabling the link solved the problem then). Yinghai
> couldn't test, but I believe with this patch even if we disable
> presence detect interrupt, the "adapter present / no present" messages
> would (rightly) convert to "Link Up / Link Down" messages (since the
> repeater keeps on flapping the link).
>
> Since it is a platform specific bug, I'm not sure what can be done to
> remove those messages except may be reduce the verbosity? If you'd
> like I could change all the INFO messages to DBG messages.

Even if it's a defect in a particular piece of hardware, I don't want
to regress on that hardware, even if the regression is just extra
messages that we didn't see before.

I think ideally we would add some sort of quirk for that hardware so
it works just as well as it does today.  I think extra messages will
lead to a bug reports from users.

Bjorn

RE: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Rajat Jain]

SGVsbG8gQmpvcm4gLyBZaW5naGFpLA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+
IEZyb206IEJqb3JuIEhlbGdhYXMgW21haWx0bzpiaGVsZ2Fhc0Bnb29nbGUuY29tXQ0KPiBTZW50
OiBNb25kYXksIEphbnVhcnkgMDYsIDIwMTQgNDowNCBQTQ0KPiBUbzogUmFqYXQgSmFpbg0KPiBD
YzogUmFqYXQgSmFpbjsgS2VuamkgS2FuZXNoaWdlOyBBbGV4IFdpbGxpYW1zb247IFlpamluZyBX
YW5nOyBsaW51eC0NCj4gcGNpQHZnZXIua2VybmVsLm9yZzsgbGludXgta2VybmVsQHZnZXIua2Vy
bmVsLm9yZzsgWWluZ2hhaSBMdTsgR3VlbnRlcg0KPiBSb2VjazsgUmFqYXQgSmFpbjsgWWluZ2hh
aSBMdQ0KPiBTdWJqZWN0OiBSZTogW1BBVENIIHYzIDQvOF0gcGNpZWhwOiBEb24ndCBkaXNhYmxl
IHRoZSBsaW5rIHBlcm1hbmVudGx5LA0KPiBkdXJpbmcgcmVtb3ZhbA0KPiANCj4gT24gU3VuLCBK
YW4gNSwgMjAxNCBhdCAxMDo1MyBBTSwgUmFqYXQgSmFpbiA8cmFqYXR4amFpbkBnbWFpbC5jb20+
DQo+IHdyb3RlOg0KPiA+IEhlbGxvIEJqb3JuLA0KPiA+DQo+ID4gSnVzdCBjaGVja2luZyBvbiB0
aGUgZmF0ZSBvZiB0aGlzIHBhdGNoIHNldC4uLg0KPiA+DQo+ID4gT24gVHVlLCBEZWMgMTcsIDIw
MTMgYXQgNTowMiBQTSwgQmpvcm4gSGVsZ2FhcyA8YmhlbGdhYXNAZ29vZ2xlLmNvbT4NCj4gd3Jv
dGU6DQo+ID4+IFsrY2MgeWluZ2hhaUBrZXJuZWwub3JnIChzZWVtcyB0byBiZSBZaW5naGFpJ3Mg
cHJlZmVycmVkIGVtYWlsXQ0KPiA+Pg0KPiA+PiBPbiBUdWUsIERlYyAxNywgMjAxMyBhdCAxMjow
NjowNVBNIC0wODAwLCBSYWphdCBKYWluIHdyb3RlOg0KPiA+Pj4gV2UgbmVlZCBmdXR1cmUgbGlu
ayB1cCBldmVudHMgZm9yIGhvdC1hZGQsIHRodXMgZG9uJ3QgZGlzYWJsZSB0aGUNCj4gPj4+IGxp
bmsgcGVybWFuZW50bHkgZHVyaW5nIGRldmljZSByZW1vdmFsLiBBbHNvLCByZW1vdmUgdGhlIHN0
YXRpYw0KPiA+Pj4gZnVuY3Rpb25zIHRoYXQgYXJlIG5vdyBsZWZ0IHVudXNlZC4NCj4gPj4NCj4g
Pj4gVGhlIGNoYW5nZWxvZyBzaG91bGQgbWVudGlvbiB0aGF0IHRoaXMgcmV2ZXJ0cyBwYXJ0IG9m
IDJkZWJkOTI4OTk5Nw0KPiAoIlBDSToNCj4gPj4gcGNpZWhwOiBEaXNhYmxlL2VuYWJsZSBsaW5r
IGR1cmluZyBzbG90IHBvd2VyIG9mZi9vbiIpLg0KPiA+DQo+ID4gU3VyZS4gRG8geW91IHdhbnQg
bWUgdG8gc3VibWl0IGFub3RoZXIgcGF0Y2ggc2V0IChidW1waW5nIHVwIHRoZQ0KPiA+IHZlcnNp
b24pIHdpdGggdGhpcyBjaGFuZ2UgbG9nLCBvciB5b3UnZCB3YW50IHRvIGFkZCB0aGlzIGNoYW5n
ZSBsb2cNCj4gPiB3aGlsZSBtZXJnaW5nPw0KPiA+DQo+ID4+DQo+ID4+IFlpbmdoYWksIGNhbiB5
b3UgdGVsbCB1cyB3aGV0aGVyIHRoaXMgaXMgYW4gaXNzdWUgb24geW91ciBzeXN0ZW1zPw0KPiA+
DQo+ID4gQXMgWWluZ2hhaSBjb25maXJtcyBmdXJ0aGVyIGRvd24gdGhpcyB0aHJlYWQsIGhpcyBp
c3N1ZSB3YXMgY29uZmlybWVkDQo+ID4gYnkgSW50ZWwgdG8gYmUgYSBidWcgaW4gdGhlIHJlcGVh
dGVyIGNoaXAuDQo+ID4gLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiA+IFlp
bmdoYWkgd3JpdGVzOg0KPiA+PiBBY2NvcmRpbmcgdG8gSFcgZ3V5cyBhbmQgSW50ZWwsIHRoYXQg
c2hvdWxkIGJlIGJ1ZyBvZiByZXBlYXRlci4NCj4gPj4NCj4gPiAtLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0NCj4gPiAgSSBkb24ndCBrbm93IGFib3V0IHRoZSBkZXRhaWxzIG9mIGhp
cyBzY2VuYXJpbywgZXhjZXB0IHRoYXQgd2hlbiB0aGUNCj4gPiBhZGFwdGVyIHdhcyBkaXNhYmxl
ZCB0aGUgcmVwZWF0ZXIga2VwdCBvbiBmbGFwcGluZyB0aGUgbGluayB1cCAmIGRvd24NCj4gPiAo
YW5kIGhlbmNlIGRpc2FibGluZyB0aGUgbGluayBzb2x2ZWQgdGhlIHByb2JsZW0gdGhlbikuIFlp
bmdoYWkNCj4gPiBjb3VsZG4ndCB0ZXN0LCBidXQgSSBiZWxpZXZlIHdpdGggdGhpcyBwYXRjaCBl
dmVuIGlmIHdlIGRpc2FibGUNCj4gPiBwcmVzZW5jZSBkZXRlY3QgaW50ZXJydXB0LCB0aGUgImFk
YXB0ZXIgcHJlc2VudCAvIG5vIHByZXNlbnQiIG1lc3NhZ2VzDQo+ID4gd291bGQgKHJpZ2h0bHkp
IGNvbnZlcnQgdG8gIkxpbmsgVXAgLyBMaW5rIERvd24iIG1lc3NhZ2VzIChzaW5jZSB0aGUNCj4g
PiByZXBlYXRlciBrZWVwcyBvbiBmbGFwcGluZyB0aGUgbGluaykuDQo+ID4NCj4gPiBTaW5jZSBp
dCBpcyBhIHBsYXRmb3JtIHNwZWNpZmljIGJ1ZywgSSdtIG5vdCBzdXJlIHdoYXQgY2FuIGJlIGRv
bmUgdG8NCj4gPiByZW1vdmUgdGhvc2UgbWVzc2FnZXMgZXhjZXB0IG1heSBiZSByZWR1Y2UgdGhl
IHZlcmJvc2l0eT8gSWYgeW91J2QNCj4gPiBsaWtlIEkgY291bGQgY2hhbmdlIGFsbCB0aGUgSU5G
TyBtZXNzYWdlcyB0byBEQkcgbWVzc2FnZXMuDQo+IA0KPiBFdmVuIGlmIGl0J3MgYSBkZWZlY3Qg
aW4gYSBwYXJ0aWN1bGFyIHBpZWNlIG9mIGhhcmR3YXJlLCBJIGRvbid0IHdhbnQgdG8NCj4gcmVn
cmVzcyBvbiB0aGF0IGhhcmR3YXJlLCBldmVuIGlmIHRoZSByZWdyZXNzaW9uIGlzIGp1c3QgZXh0
cmEgbWVzc2FnZXMNCj4gdGhhdCB3ZSBkaWRuJ3Qgc2VlIGJlZm9yZS4NCj4gDQo+IEkgdGhpbmsg
aWRlYWxseSB3ZSB3b3VsZCBhZGQgc29tZSBzb3J0IG9mIHF1aXJrIGZvciB0aGF0IGhhcmR3YXJl
IHNvIGl0DQo+IHdvcmtzIGp1c3QgYXMgd2VsbCBhcyBpdCBkb2VzIHRvZGF5LiAgSSB0aGluayBl
eHRyYSBtZXNzYWdlcyB3aWxsIGxlYWQNCj4gdG8gYSBidWcgcmVwb3J0cyBmcm9tIHVzZXJzLg0K
DQpTdXJlLCBJIGNhbiBkbyB0aGF0LiBJIHRoaW5rIHdoYXQgdGhlIHF1aXJrIHdvdWxkIGhhdmUg
dG8gZG8gaXMgdGhhdCBmb3IgdGhhdCBwYXJ0aWN1bGFyIHBsYXRmb3JtLCBkb24ndCBlbmFibGUg
dGhlIGxpbmstc3RhdGUgYmFzZWQgaG90cGx1Zy4gKFNpbmNlIGxpbmstc3RhdGUgaG90cGx1ZyB3
aWxsIG5vdCB3b3JrIGlmIHdlIGRpc2FibGUgdGhlIGxpbmsgcGVybWFuZW50bHkgYXMgd2UgZG8g
dG9kYXkgb24gY2FyZCByZW1vdmFsKS4NCg0KQnV0IHRoZSBxdWVzdGlvbiBpcyBob3cgdG8gZGV0
ZXJtaW5lIHRoYXQgdGhlIHF1aXJrIGhhcyB0byBiZSBhcHBsaWVkPyBJIHRoaW5rIHRoZSBvYmpl
Y3RpdmUgaXMgdG8gYXBwbHkgdGhlIHF1aXJrIHRvIHRoZSBwbGF0Zm9ybXMgdGhhdCBoYXZlIGEg
IlBDSWUgcmVwZWF0ZXIiLiBTaW5jZSB0aGlzIGRvZXMgbm90IGRlcGVuZCBvbiBhIFBDSSBkZXZp
Y2UgLyB2ZW5kb3IgSUQsIGFuZCBJIHRoaW5rIHRoZSBQQ0llIHJlcGVhdGVyIGlzIHByb2JhYmx5
IG5vdCBldmVuIHZpc2libGUgdG8gdGhlIHBjaWVocCBvciB0aGUgUENJIHN1YnN5c3RlbSwgaG93
IGRvIEkgZGV0ZXJtaW5lIHRoYXQgdGhlIHF1aXJrIGhhcyB0byBiZSBhcHBsaWVkPw0KDQpJZiAo
aHdfaGFzX3BjaWVfcmVwZWF0ZXIpDQoJRG9uJ3QgdXNlIGxpbmstc3RhdGUgaG90cGx1ZyAoYW5k
IGRpc2FibGUgbGluayBwZXJtYW5lbnRseSBkdXJpbmcgY2FyZCByZW1vdmFsKQ0KRWxzZQ0KCVVz
ZSBsaW5rLXN0YXRlIGhvdHBsdWcgKGFuZCBkb24ndCBkaXNhYmxlIHRoZSBsaW5rIHBlcm1hbmVu
dGx5KQ0KCQ0KDQpZaW5naGFpOiBTaW5jZSBJIGRvIG5vdCBoYXZlIHRoYXQgaGFyZHdhcmUsIEkg
d2lsbCBuZWVkIHNvbWUgaGVscCBpbiB0ZXN0aW5nIHRoZSBwYXRjaCB3aXRoIHRoZSBxdWlyay4g
SSB3YXMgd29uZGVyaW5nIGlmIHlvdSdkIHN0aWxsIGhhdmUgdGhhdCBoYXJkd2FyZSBhcm91bmQg
YW5kIHdvdWxkIGJlIGFibGUgdG8gaGVscCBtZSB3aXRoIHRlc3Rpbmc/DQoNClRoYW5rcywNCg0K
UmFqYXQNCg==

RE: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Rajat Jain]

SGkgQmpvcm4sDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogbGludXgt
cGNpLW93bmVyQHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LXBjaS0NCj4gb3duZXJAdmdl
ci5rZXJuZWwub3JnXSBPbiBCZWhhbGYgT2YgUmFqYXQgSmFpbg0KPiBTZW50OiBUdWVzZGF5LCBK
YW51YXJ5IDA3LCAyMDE0IDEwOjIxIEFNDQo+IFRvOiBCam9ybiBIZWxnYWFzOyBSYWphdCBKYWlu
DQo+IENjOiBSYWphdCBKYWluOyBLZW5qaSBLYW5lc2hpZ2U7IEFsZXggV2lsbGlhbXNvbjsgWWlq
aW5nIFdhbmc7IGxpbnV4LQ0KPiBwY2lAdmdlci5rZXJuZWwub3JnOyBsaW51eC1rZXJuZWxAdmdl
ci5rZXJuZWwub3JnOyBZaW5naGFpIEx1OyBHdWVudGVyDQo+IFJvZWNrOyBZaW5naGFpIEx1DQo+
IFN1YmplY3Q6IFJFOiBbUEFUQ0ggdjMgNC84XSBwY2llaHA6IERvbid0IGRpc2FibGUgdGhlIGxp
bmsgcGVybWFuZW50bHksDQo+IGR1cmluZyByZW1vdmFsDQo+IA0KPiBIZWxsbyBCam9ybiAvIFlp
bmdoYWksDQo+IA0KPiA+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4gRnJvbTogQmpv
cm4gSGVsZ2FhcyBbbWFpbHRvOmJoZWxnYWFzQGdvb2dsZS5jb21dDQo+ID4gU2VudDogTW9uZGF5
LCBKYW51YXJ5IDA2LCAyMDE0IDQ6MDQgUE0NCj4gPiBUbzogUmFqYXQgSmFpbg0KPiA+IENjOiBS
YWphdCBKYWluOyBLZW5qaSBLYW5lc2hpZ2U7IEFsZXggV2lsbGlhbXNvbjsgWWlqaW5nIFdhbmc7
IGxpbnV4LQ0KPiA+IHBjaUB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5l
bC5vcmc7IFlpbmdoYWkgTHU7IEd1ZW50ZXINCj4gPiBSb2VjazsgUmFqYXQgSmFpbjsgWWluZ2hh
aSBMdQ0KPiA+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjMgNC84XSBwY2llaHA6IERvbid0IGRpc2Fi
bGUgdGhlIGxpbmsNCj4gPiBwZXJtYW5lbnRseSwgZHVyaW5nIHJlbW92YWwNCj4gPg0KPiA+IE9u
IFN1biwgSmFuIDUsIDIwMTQgYXQgMTA6NTMgQU0sIFJhamF0IEphaW4gPHJhamF0eGphaW5AZ21h
aWwuY29tPg0KPiA+IHdyb3RlOg0KPiA+ID4gSGVsbG8gQmpvcm4sDQo+ID4gPg0KPiA+ID4gSnVz
dCBjaGVja2luZyBvbiB0aGUgZmF0ZSBvZiB0aGlzIHBhdGNoIHNldC4uLg0KPiA+ID4NCj4gPiA+
IE9uIFR1ZSwgRGVjIDE3LCAyMDEzIGF0IDU6MDIgUE0sIEJqb3JuIEhlbGdhYXMgPGJoZWxnYWFz
QGdvb2dsZS5jb20+DQo+ID4gd3JvdGU6DQo+ID4gPj4gWytjYyB5aW5naGFpQGtlcm5lbC5vcmcg
KHNlZW1zIHRvIGJlIFlpbmdoYWkncyBwcmVmZXJyZWQgZW1haWxdDQo+ID4gPj4NCj4gPiA+PiBP
biBUdWUsIERlYyAxNywgMjAxMyBhdCAxMjowNjowNVBNIC0wODAwLCBSYWphdCBKYWluIHdyb3Rl
Og0KPiA+ID4+PiBXZSBuZWVkIGZ1dHVyZSBsaW5rIHVwIGV2ZW50cyBmb3IgaG90LWFkZCwgdGh1
cyBkb24ndCBkaXNhYmxlIHRoZQ0KPiA+ID4+PiBsaW5rIHBlcm1hbmVudGx5IGR1cmluZyBkZXZp
Y2UgcmVtb3ZhbC4gQWxzbywgcmVtb3ZlIHRoZSBzdGF0aWMNCj4gPiA+Pj4gZnVuY3Rpb25zIHRo
YXQgYXJlIG5vdyBsZWZ0IHVudXNlZC4NCj4gPiA+Pg0KPiA+ID4+IFRoZSBjaGFuZ2Vsb2cgc2hv
dWxkIG1lbnRpb24gdGhhdCB0aGlzIHJldmVydHMgcGFydCBvZiAyZGViZDkyODk5OTcNCj4gPiAo
IlBDSToNCj4gPiA+PiBwY2llaHA6IERpc2FibGUvZW5hYmxlIGxpbmsgZHVyaW5nIHNsb3QgcG93
ZXIgb2ZmL29uIikuDQo+ID4gPg0KPiA+ID4gU3VyZS4gRG8geW91IHdhbnQgbWUgdG8gc3VibWl0
IGFub3RoZXIgcGF0Y2ggc2V0IChidW1waW5nIHVwIHRoZQ0KPiA+ID4gdmVyc2lvbikgd2l0aCB0
aGlzIGNoYW5nZSBsb2csIG9yIHlvdSdkIHdhbnQgdG8gYWRkIHRoaXMgY2hhbmdlIGxvZw0KPiA+
ID4gd2hpbGUgbWVyZ2luZz8NCj4gPiA+DQo+ID4gPj4NCj4gPiA+PiBZaW5naGFpLCBjYW4geW91
IHRlbGwgdXMgd2hldGhlciB0aGlzIGlzIGFuIGlzc3VlIG9uIHlvdXIgc3lzdGVtcz8NCj4gPiA+
DQo+ID4gPiBBcyBZaW5naGFpIGNvbmZpcm1zIGZ1cnRoZXIgZG93biB0aGlzIHRocmVhZCwgaGlz
IGlzc3VlIHdhcw0KPiA+ID4gY29uZmlybWVkIGJ5IEludGVsIHRvIGJlIGEgYnVnIGluIHRoZSBy
ZXBlYXRlciBjaGlwLg0KPiA+ID4gLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0K
PiA+ID4gWWluZ2hhaSB3cml0ZXM6DQo+ID4gPj4gQWNjb3JkaW5nIHRvIEhXIGd1eXMgYW5kIElu
dGVsLCB0aGF0IHNob3VsZCBiZSBidWcgb2YgcmVwZWF0ZXIuDQo+ID4gPj4NCj4gPiA+IC0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiA+ID4gIEkgZG9uJ3Qga25vdyBhYm91dCB0
aGUgZGV0YWlscyBvZiBoaXMgc2NlbmFyaW8sIGV4Y2VwdCB0aGF0IHdoZW4NCj4gPiA+IHRoZSBh
ZGFwdGVyIHdhcyBkaXNhYmxlZCB0aGUgcmVwZWF0ZXIga2VwdCBvbiBmbGFwcGluZyB0aGUgbGlu
ayB1cCAmDQo+ID4gPiBkb3duIChhbmQgaGVuY2UgZGlzYWJsaW5nIHRoZSBsaW5rIHNvbHZlZCB0
aGUgcHJvYmxlbSB0aGVuKS4gWWluZ2hhaQ0KPiA+ID4gY291bGRuJ3QgdGVzdCwgYnV0IEkgYmVs
aWV2ZSB3aXRoIHRoaXMgcGF0Y2ggZXZlbiBpZiB3ZSBkaXNhYmxlDQo+ID4gPiBwcmVzZW5jZSBk
ZXRlY3QgaW50ZXJydXB0LCB0aGUgImFkYXB0ZXIgcHJlc2VudCAvIG5vIHByZXNlbnQiDQo+ID4g
PiBtZXNzYWdlcyB3b3VsZCAocmlnaHRseSkgY29udmVydCB0byAiTGluayBVcCAvIExpbmsgRG93
biIgbWVzc2FnZXMNCj4gPiA+IChzaW5jZSB0aGUgcmVwZWF0ZXIga2VlcHMgb24gZmxhcHBpbmcg
dGhlIGxpbmspLg0KPiA+ID4NCj4gPiA+IFNpbmNlIGl0IGlzIGEgcGxhdGZvcm0gc3BlY2lmaWMg
YnVnLCBJJ20gbm90IHN1cmUgd2hhdCBjYW4gYmUgZG9uZQ0KPiA+ID4gdG8gcmVtb3ZlIHRob3Nl
IG1lc3NhZ2VzIGV4Y2VwdCBtYXkgYmUgcmVkdWNlIHRoZSB2ZXJib3NpdHk/IElmDQo+ID4gPiB5
b3UnZCBsaWtlIEkgY291bGQgY2hhbmdlIGFsbCB0aGUgSU5GTyBtZXNzYWdlcyB0byBEQkcgbWVz
c2FnZXMuDQo+ID4NCj4gPiBFdmVuIGlmIGl0J3MgYSBkZWZlY3QgaW4gYSBwYXJ0aWN1bGFyIHBp
ZWNlIG9mIGhhcmR3YXJlLCBJIGRvbid0IHdhbnQNCj4gPiB0byByZWdyZXNzIG9uIHRoYXQgaGFy
ZHdhcmUsIGV2ZW4gaWYgdGhlIHJlZ3Jlc3Npb24gaXMganVzdCBleHRyYQ0KPiA+IG1lc3NhZ2Vz
IHRoYXQgd2UgZGlkbid0IHNlZSBiZWZvcmUuDQo+ID4NCj4gPiBJIHRoaW5rIGlkZWFsbHkgd2Ug
d291bGQgYWRkIHNvbWUgc29ydCBvZiBxdWlyayBmb3IgdGhhdCBoYXJkd2FyZSBzbw0KPiA+IGl0
IHdvcmtzIGp1c3QgYXMgd2VsbCBhcyBpdCBkb2VzIHRvZGF5LiAgSSB0aGluayBleHRyYSBtZXNz
YWdlcyB3aWxsDQo+ID4gbGVhZCB0byBhIGJ1ZyByZXBvcnRzIGZyb20gdXNlcnMuDQo+IA0KPiBT
dXJlLCBJIGNhbiBkbyB0aGF0LiBJIHRoaW5rIHdoYXQgdGhlIHF1aXJrIHdvdWxkIGhhdmUgdG8g
ZG8gaXMgdGhhdCBmb3INCj4gdGhhdCBwYXJ0aWN1bGFyIHBsYXRmb3JtLCBkb24ndCBlbmFibGUg
dGhlIGxpbmstc3RhdGUgYmFzZWQgaG90cGx1Zy4NCj4gKFNpbmNlIGxpbmstc3RhdGUgaG90cGx1
ZyB3aWxsIG5vdCB3b3JrIGlmIHdlIGRpc2FibGUgdGhlIGxpbmsNCj4gcGVybWFuZW50bHkgYXMg
d2UgZG8gdG9kYXkgb24gY2FyZCByZW1vdmFsKS4NCj4gDQo+IEJ1dCB0aGUgcXVlc3Rpb24gaXMg
aG93IHRvIGRldGVybWluZSB0aGF0IHRoZSBxdWlyayBoYXMgdG8gYmUgYXBwbGllZD8gSQ0KPiB0
aGluayB0aGUgb2JqZWN0aXZlIGlzIHRvIGFwcGx5IHRoZSBxdWlyayB0byB0aGUgcGxhdGZvcm1z
IHRoYXQgaGF2ZSBhDQo+ICJQQ0llIHJlcGVhdGVyIi4gU2luY2UgdGhpcyBkb2VzIG5vdCBkZXBl
bmQgb24gYSBQQ0kgZGV2aWNlIC8gdmVuZG9yIElELA0KPiBhbmQgSSB0aGluayB0aGUgUENJZSBy
ZXBlYXRlciBpcyBwcm9iYWJseSBub3QgZXZlbiB2aXNpYmxlIHRvIHRoZSBwY2llaHANCj4gb3Ig
dGhlIFBDSSBzdWJzeXN0ZW0sIGhvdyBkbyBJIGRldGVybWluZSB0aGF0IHRoZSBxdWlyayBoYXMg
dG8gYmUNCj4gYXBwbGllZD8NCg0KQW55IGlkZWFzIG9uIGhvdyBkbyBJIGlkZW50aWZ5IHRoZSBw
bGF0Zm9ybXMgdGhhdCBtYXkgaGF2ZSB0aGlzIHByb2JsZW0/IA0KDQpUaGFua3MsDQoNClJhamF0
DQoNCg0KPiANCj4gSWYgKGh3X2hhc19wY2llX3JlcGVhdGVyKQ0KPiAJRG9uJ3QgdXNlIGxpbmst
c3RhdGUgaG90cGx1ZyAoYW5kIGRpc2FibGUgbGluayBwZXJtYW5lbnRseSBkdXJpbmcNCj4gY2Fy
ZCByZW1vdmFsKSBFbHNlDQo+IAlVc2UgbGluay1zdGF0ZSBob3RwbHVnIChhbmQgZG9uJ3QgZGlz
YWJsZSB0aGUgbGluayBwZXJtYW5lbnRseSkNCj4gDQo+IA0KPiBZaW5naGFpOiBTaW5jZSBJIGRv
IG5vdCBoYXZlIHRoYXQgaGFyZHdhcmUsIEkgd2lsbCBuZWVkIHNvbWUgaGVscCBpbg0KPiB0ZXN0
aW5nIHRoZSBwYXRjaCB3aXRoIHRoZSBxdWlyay4gSSB3YXMgd29uZGVyaW5nIGlmIHlvdSdkIHN0
aWxsIGhhdmUNCj4gdGhhdCBoYXJkd2FyZSBhcm91bmQgYW5kIHdvdWxkIGJlIGFibGUgdG8gaGVs
cCBtZSB3aXRoIHRlc3Rpbmc/DQo+IA0KPiBUaGFua3MsDQo+IA0KPiBSYWphdA0KPiAEIHsubiAr
ICAgICAgICslICBsendtICBiIOunsiAgciAgelggIBpcICkgICB3KmpnICAgHiAgICAg3aJqLyAg
IHog3pYgIDIg3pkNCj4gJiAp36EgYSAgfyAgHiBHICAgaCAPIGo6K3YgICB3INmlDQo=

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Bjorn Helgaas]

On Thu, Jan 9, 2014 at 1:45 PM, Rajat Jain <rajatjain@juniper.net> wrote:
> Hi Bjorn,
>
>> -----Original Message-----
>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
>> owner@vger.kernel.org] On Behalf Of Rajat Jain
>> Sent: Tuesday, January 07, 2014 10:21 AM
>> To: Bjorn Helgaas; Rajat Jain
>> Cc: Rajat Jain; Kenji Kaneshige; Alex Williamson; Yijing Wang; linux-
>> pci@vger.kernel.org; linux-kernel@vger.kernel.org; Yinghai Lu; Guenter
>> Roeck; Yinghai Lu
>> Subject: RE: [PATCH v3 4/8] pciehp: Don't disable the link permanently,
>> during removal
>>
>> Hello Bjorn / Yinghai,
>>
>> > -----Original Message-----
>> > From: Bjorn Helgaas [mailto:bhelgaas@google.com]
>> > Sent: Monday, January 06, 2014 4:04 PM
>> > To: Rajat Jain
>> > Cc: Rajat Jain; Kenji Kaneshige; Alex Williamson; Yijing Wang; linux-
>> > pci@vger.kernel.org; linux-kernel@vger.kernel.org; Yinghai Lu; Guenter
>> > Roeck; Rajat Jain; Yinghai Lu
>> > Subject: Re: [PATCH v3 4/8] pciehp: Don't disable the link
>> > permanently, during removal
>> >
>> > On Sun, Jan 5, 2014 at 10:53 AM, Rajat Jain <rajatxjain@gmail.com>
>> > wrote:
>> > > Hello Bjorn,
>> > >
>> > > Just checking on the fate of this patch set...
>> > >
>> > > On Tue, Dec 17, 2013 at 5:02 PM, Bjorn Helgaas <bhelgaas@google.com>
>> > wrote:
>> > >> [+cc yinghai@kernel.org (seems to be Yinghai's preferred email]
>> > >>
>> > >> On Tue, Dec 17, 2013 at 12:06:05PM -0800, Rajat Jain wrote:
>> > >>> We need future link up events for hot-add, thus don't disable the
>> > >>> link permanently during device removal. Also, remove the static
>> > >>> functions that are now left unused.
>> > >>
>> > >> The changelog should mention that this reverts part of 2debd9289997
>> > ("PCI:
>> > >> pciehp: Disable/enable link during slot power off/on").
>> > >
>> > > Sure. Do you want me to submit another patch set (bumping up the
>> > > version) with this change log, or you'd want to add this change log
>> > > while merging?
>> > >
>> > >>
>> > >> Yinghai, can you tell us whether this is an issue on your systems?
>> > >
>> > > As Yinghai confirms further down this thread, his issue was
>> > > confirmed by Intel to be a bug in the repeater chip.
>> > > ----------------------------------
>> > > Yinghai writes:
>> > >> According to HW guys and Intel, that should be bug of repeater.
>> > >>
>> > > ---------------------------------
>> > >  I don't know about the details of his scenario, except that when
>> > > the adapter was disabled the repeater kept on flapping the link up &
>> > > down (and hence disabling the link solved the problem then). Yinghai
>> > > couldn't test, but I believe with this patch even if we disable
>> > > presence detect interrupt, the "adapter present / no present"
>> > > messages would (rightly) convert to "Link Up / Link Down" messages
>> > > (since the repeater keeps on flapping the link).
>> > >
>> > > Since it is a platform specific bug, I'm not sure what can be done
>> > > to remove those messages except may be reduce the verbosity? If
>> > > you'd like I could change all the INFO messages to DBG messages.
>> >
>> > Even if it's a defect in a particular piece of hardware, I don't want
>> > to regress on that hardware, even if the regression is just extra
>> > messages that we didn't see before.
>> >
>> > I think ideally we would add some sort of quirk for that hardware so
>> > it works just as well as it does today.  I think extra messages will
>> > lead to a bug reports from users.
>>
>> Sure, I can do that. I think what the quirk would have to do is that for
>> that particular platform, don't enable the link-state based hotplug.
>> (Since link-state hotplug will not work if we disable the link
>> permanently as we do today on card removal).
>>
>> But the question is how to determine that the quirk has to be applied? I
>> think the objective is to apply the quirk to the platforms that have a
>> "PCIe repeater". Since this does not depend on a PCI device / vendor ID,
>> and I think the PCIe repeater is probably not even visible to the pciehp
>> or the PCI subsystem, how do I determine that the quirk has to be
>> applied?
>
> Any ideas on how do I identify the platforms that may have this problem?

I sure don't know.  I suspect you're right that the PCIe repeater is
invisible to software, at least in terms of PCI config space.  Maybe
we could use DMI to identify platforms.  That's not a very good
solution because we have to come up with a list, but I can't think of
a better way.  Yinghai knows more about the platform and might have
better ideas.

Bjorn

>> If (hw_has_pcie_repeater)
>>       Don't use link-state hotplug (and disable link permanently during
>> card removal) Else
>>       Use link-state hotplug (and don't disable the link permanently)
>>
>>
>> Yinghai: Since I do not have that hardware, I will need some help in
>> testing the patch with the quirk. I was wondering if you'd still have
>> that hardware around and would be able to help me with testing?
>>
>> Thanks,
>>
>> Rajat
>>   {.n +       +%  lzwm  b 맲  r  zX   \ )   w*jg         ݢj/   z ޖ  2 ޙ
>> & )ߡ a       G   h   j:+v   w ٥

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Rajat Jain]

Hi Yinghai / Bjorn,


On Thu, Jan 9, 2014 at 12:58 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> >
>>> > On Sun, Jan 5, 2014 at 10:53 AM, Rajat Jain <rajatxjain@gmail.com>
>>> > wrote:
>>> > > Hello Bjorn,
>>> > >
>>> > > Just checking on the fate of this patch set...
>>> > >
>>> > > On Tue, Dec 17, 2013 at 5:02 PM, Bjorn Helgaas <bhelgaas@google.com>
>>> > wrote:
>>> > >> [+cc yinghai@kernel.org (seems to be Yinghai's preferred email]
>>> > >>
>>> > >> On Tue, Dec 17, 2013 at 12:06:05PM -0800, Rajat Jain wrote:
>>> > >>> We need future link up events for hot-add, thus don't disable the
>>> > >>> link permanently during device removal. Also, remove the static
>>> > >>> functions that are now left unused.
>>> > >>
>>> > >> The changelog should mention that this reverts part of 2debd9289997
>>> > ("PCI:
>>> > >> pciehp: Disable/enable link during slot power off/on").
>>> > >
>>> > > Sure. Do you want me to submit another patch set (bumping up the
>>> > > version) with this change log, or you'd want to add this change log
>>> > > while merging?
>>> > >
>>> > >>
>>> > >> Yinghai, can you tell us whether this is an issue on your systems?
>>> > >
>>> > > As Yinghai confirms further down this thread, his issue was
>>> > > confirmed by Intel to be a bug in the repeater chip.
>>> > > ----------------------------------
>>> > > Yinghai writes:
>>> > >> According to HW guys and Intel, that should be bug of repeater.
>>> > >>
>>> > > ---------------------------------
>>> > >  I don't know about the details of his scenario, except that when
>>> > > the adapter was disabled the repeater kept on flapping the link up &
>>> > > down (and hence disabling the link solved the problem then). Yinghai
>>> > > couldn't test, but I believe with this patch even if we disable
>>> > > presence detect interrupt, the "adapter present / no present"
>>> > > messages would (rightly) convert to "Link Up / Link Down" messages
>>> > > (since the repeater keeps on flapping the link).
>>> > >
>>> > > Since it is a platform specific bug, I'm not sure what can be done
>>> > > to remove those messages except may be reduce the verbosity? If
>>> > > you'd like I could change all the INFO messages to DBG messages.
>>> >
>>> > Even if it's a defect in a particular piece of hardware, I don't want
>>> > to regress on that hardware, even if the regression is just extra
>>> > messages that we didn't see before.
>>> >
>>> > I think ideally we would add some sort of quirk for that hardware so
>>> > it works just as well as it does today.  I think extra messages will
>>> > lead to a bug reports from users.
>>>
>>> Sure, I can do that. I think what the quirk would have to do is that for
>>> that particular platform, don't enable the link-state based hotplug.
>>> (Since link-state hotplug will not work if we disable the link
>>> permanently as we do today on card removal).
>>>
>>> But the question is how to determine that the quirk has to be applied? I
>>> think the objective is to apply the quirk to the platforms that have a
>>> "PCIe repeater". Since this does not depend on a PCI device / vendor ID,
>>> and I think the PCIe repeater is probably not even visible to the pciehp
>>> or the PCI subsystem, how do I determine that the quirk has to be
>>> applied?
>>
>> Any ideas on how do I identify the platforms that may have this problem?
>
> I sure don't know.  I suspect you're right that the PCIe repeater is
> invisible to software, at least in terms of PCI config space.  Maybe
> we could use DMI to identify platforms.  That's not a very good
> solution because we have to come up with a list, but I can't think of
> a better way.  Yinghai knows more about the platform and might have
> better ideas.

Yinghai: I am trying to understand what exactly is this platform bug
and how to add a quirk such that this platform remains unaffected. Can
you please help me by suggesting how to decide if this is _the_
platform that has the bug (the pcie repeater).

Bjorn: It seems to be that identification of this platform will be out
PCI code (since the bug seems to be in a pcie repeater chip which is
not a PCI device visible to SW). So even if we find a way to identify
this platform (e.g DMI) , I doubt if you'd want me to add that in the
pciehp code (which is platform independent so far). At best, the only
way out I can see is to provide a knob from the pciehp, that can be
use by the platform code to either enable or disable the link state
hotplug. It could go back towards using a module parameter like
pciehp_use_link_events. Please suggest.

The only other way I can think of, is that I can remove the debug
message altogether (Link up / Link down). (Or the user can change the
verbosity).

Humm, when I think of it, we're trying to address a bug of a chip
which is not a PCI device, into pciehp. I'm praying it doesn't bring
this patch set to a dead end :-)

Thanks,

Rajat

>
> Bjorn
>
>>> If (hw_has_pcie_repeater)
>>>       Don't use link-state hotplug (and disable link permanently during
>>> card removal) Else
>>>       Use link-state hotplug (and don't disable the link permanently)
>>>
>>>
>>> Yinghai: Since I do not have that hardware, I will need some help in
>>> testing the patch with the quirk. I was wondering if you'd still have
>>> that hardware around and would be able to help me with testing?
>>>
>>> Thanks,
>>>
>>> Rajat
>>>   {.n +       +%  lzwm  b 맲  r  zX   \ )   w*jg         ݢj/   z ޖ  2 ޙ
>>> & )ߡ a       G   h   j:+v   w ٥

Re: [PATCH v3 4/8] pciehp: Don't disable the link permanently, during removal

[Bjorn Helgaas]

On Mon, Jan 13, 2014 at 1:30 AM, Rajat Jain <rajatxjain@gmail.com> wrote:

> Yinghai: I am trying to understand what exactly is this platform bug
> and how to add a quirk such that this platform remains unaffected. Can
> you please help me by suggesting how to decide if this is _the_
> platform that has the bug (the pcie repeater).
>
> Bjorn: It seems to be that identification of this platform will be out
> PCI code (since the bug seems to be in a pcie repeater chip which is
> not a PCI device visible to SW). So even if we find a way to identify
> this platform (e.g DMI) , I doubt if you'd want me to add that in the
> pciehp code (which is platform independent so far). At best, the only
> way out I can see is to provide a knob from the pciehp, that can be
> use by the platform code to either enable or disable the link state
> hotplug. It could go back towards using a module parameter like
> pciehp_use_link_events. Please suggest.
>
> The only other way I can think of, is that I can remove the debug
> message altogether (Link up / Link down). (Or the user can change the
> verbosity).

I think it's perfectly fine to add a DMI-based quirk in pciehp.  Yes,
it's a bit ugly, but that's just the nature of working around hardware
defects.  Identify the platform, emit a diagnostic ("disabling link
state because platform may be buggy"), enable the workaround.  That
seems better than requiring the user to figure out what hardware he
has and whether it has a defect.

I would also be OK with adding a pciehp module parameter to explicitly
enable or disable the workaround if that seems necessary.  I just want
the common case of correctly working hardware to work without any
switches.

Removing or rate-limiting the link up/down debug message is also fine with me.

Bjorn