pciehp: Acknowledge the spurious "cmd completed" event.

In case of a spurious "cmd completed", pcie_write_cmd() does not clear it, but yet expects more "cmd completed" events to be generated. This does not happen because the previous (spurious) event has not been acknowledged. Fix that.

Signed-off-by Rajat Jain <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
This is how I saw it in action: my controller does not implement any hot-plug elements (LED, power ctrl, EMI etc) but still supports Command completed bit.
- During initialization,

      pcie_disable_notification()
      -> pcie_write_cmd()
         -> writes to Slot control register
            -> which causes PCI_EXP_SLTSTA_CC to get set, which is not
               cleared, because IRQ is not generated (we just disabled
               notifications).
 - After some time,
      pcie_enable_notification()
      -> pcie_write_cmd()
          -> finds PCI_EXP_SLTSTA_CC is set, assumes it is spurious.
          -> Does not clear it, yet expects more command completed
             events to be generated (never happens).

 drivers/pci/hotplug/pciehp_hpc.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/hotplug/pciehp_hpc.c +1 -0

index 5b8d749..ba8e06f 100644
@@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
 	}
 
 	if (slot_status & PCI_EXP_SLTSTA_CC) {
+		pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
 		if (!ctrl->no_cmd_complete) {
 			/*
 			 * After 1 sec and CMD_COMPLETED still not set, just

Signature · 1 line

1.7.9.5

Quoted 12 lines · Rajat Jain

In case of a spurious "cmd completed", pcie_write_cmd() does not clear it, but yet expects more "cmd completed" events to be generated. This does not happen because the previous (spurious) event has not been acknowledged. Fix that.

Signed-off-by Rajat Jain <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
This is how I saw it in action: my controller does not implement any hot-plug elements (LED, power ctrl, EMI etc) but still supports Command completed bit.
- During initialization,

Quoted 12 lines

      pcie_disable_notification()
      -> pcie_write_cmd()
         -> writes to Slot control register
            -> which causes PCI_EXP_SLTSTA_CC to get set, which is not
               cleared, because IRQ is not generated (we just disabled
               notifications).
 - After some time,
      pcie_enable_notification()
      -> pcie_write_cmd()
          -> finds PCI_EXP_SLTSTA_CC is set, assumes it is spurious.
          -> Does not clear it, yet expects more command completed
             events to be generated (never happens).

I'm not sure this "cmd completed" is actually spurious. Spec section 7.8.10 is very clear that any write to Slot Control must cause a hot-plug command to be generated (if the port is hot-plug capable).

Can you collect "lspci -vv" output for your controller?

I assume
you're hitting this case in pcie_init() (added by 5808639bfa98 ("pciehp: fix slow probing")):

        /*
         * Controller doesn't notify of command completion if the "No
         * Command Completed Support" bit is set in Slot Capability
         * register or the controller supports none of power
         * controller, attention led, power led and EMI.
         */
        if (NO_CMD_CMPL(ctrl) ||
            !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
            ctrl->no_cmd_complete = 1;

and we're setting "no_cmd_complete = 1" for your controller, which keeps us from waiting for completion in pcie_write_cmd().

I'm dubious about the assertion that a controller without power control, attention LED, power LED, or EMI can't support command completion. I don't see anything in the spec to that effect.

Since you're seeing PCI_EXP_SLTSTA_CC=1, your controller *should* support Command Completion notification and PCI_EXP_SLTCAP_NCCS should be 0 (per Table 7-20), so I wonder what happens on your system if you change pcie_init() so it leaves "ctrl->no_cmd_complete = 0" instead? Does it work correctly then?

I know we can't just drop the "!(POWER_CTRL(ctrl) | ...)" tests because we don't want to reintroduce the problem fixed by 5808639bfa98, but I wonder if we can find a better fix that addresses both problems.

Bjorn

Quoted 19 lines

 drivers/pci/hotplug/pciehp_hpc.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/hotplug/pciehp_hpc.c +1 -1

index 5b8d749..ba8e06f 100644
@@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
      }
 
      if (slot_status & PCI_EXP_SLTSTA_CC) {
+             pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
              if (!ctrl->no_cmd_complete) {
                      /*
                       * After 1 sec and CMD_COMPLETED still not set, just
--

1.7.9.5

Quoted 11 lines

Quoted 11 lines

 drivers/pci/hotplug/pciehp_hpc.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/hotplug/pciehp_hpc.c +0 -0

index 5b8d749..ba8e06f 100644
@@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
      }
 

Quoted 8 lines

Quoted 8 lines

drivers/pci/hotplug/pciehp_hpc.c +1 -1

      if (slot_status & PCI_EXP_SLTSTA_CC) {
+             pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
              if (!ctrl->no_cmd_complete) {
                      /*
                       * After 1 sec and CMD_COMPLETED still not set, just
--

1.7.9.5

        if (NO_CMD_CMPL(ctrl) ||
            !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
            ctrl->no_cmd_complete = 1;

Hi Bjorn / list,

I think there are two independent problems that need to be addressed.

Problem #1: In any condition where (e.g. spurious interrupt) once the execution reaches inside the if condition below, the PCI_EXP_SLTSTA_CC bit is already set, and needs to be cleared. This does not happen today and hence this patch was aimed at solving that. Please note that this will not cause any problems to the systems that were fixed by 5808639bfa98, because this is equally applicable to any case.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

 drivers/pci/hotplug/pciehp_hpc.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/hotplug/pciehp_hpc.c +0 -0

index 5b8d749..ba8e06f 100644
@@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
      }
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/pci/hotplug/pciehp_hpc.c +1 -1

      if (slot_status & PCI_EXP_SLTSTA_CC) {
+             pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
              if (!ctrl->no_cmd_complete) {
                      /*
                       * After 1 sec and CMD_COMPLETED still not set, just
--

1.7.9.5

Problem #2: Looks like we have now 2 classes of systems:

a) The 5808639bfa98 was addressed towards systems that advertise the "Command completion notification" capability, but do not notify of command completion in case MRL, Power Ctrl, Attn LED and Pwr LED bits are not written in SLOT_CTRL register (not implemented in capabilities). Thus no command completion shall be generated for pcie_disable_notifications() for e.g.

b) I have a system that where none of the above described HP elements are present implemented, but the controller supports command completion, and sends it out for every write of the slot control register. Thus notification for command complete is generated for pcie_disable_notifiction().

I don't believe there is a way to differentiate between these 2 classes of systems at init time, unless we try to generate a notification and do or do not get one.

The PCIe specification section 7.8.10 seems to lean towards category
(b) of systems, but today this class of systems shall get penalized by delay of 1 second (per controller) during probe.

Quoted 10 lines · Rajat Jain

Hi Bjorn / list,

I think there are two independent problems that need to be addressed.

Problem #1: In any condition where (e.g. spurious interrupt) once the execution reaches inside the if condition below, the PCI_EXP_SLTSTA_CC bit is already set, and needs to be cleared. This does not happen today and hence this patch was aimed at solving that. Please note that this will not cause any problems to the systems that were fixed by 5808639bfa98, because this is equally applicable to any case.

Your patch might be the right thing to do, but something still niggles at me, and I can't quite put my finger on it yet.

Quoted 22 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

 drivers/pci/hotplug/pciehp_hpc.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/hotplug/pciehp_hpc.c +0 -0

index 5b8d749..ba8e06f 100644
@@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
      }
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/pci/hotplug/pciehp_hpc.c +1 -1

      if (slot_status & PCI_EXP_SLTSTA_CC) {
+             pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
              if (!ctrl->no_cmd_complete) {
                      /*
                       * After 1 sec and CMD_COMPLETED still not set, just
--

1.7.9.5

Problem #2: Looks like we have now 2 classes of systems:

Quoted 6 lines

a) The 5808639bfa98 was addressed towards systems that advertise the "Command completion notification" capability, but do not notify of command completion in case MRL, Power Ctrl, Attn LED and Pwr LED bits are not written in SLOT_CTRL register (not implemented in capabilities). Thus no command completion shall be generated for pcie_disable_notifications() for e.g.

This seems pretty clearly out of spec. I read the 82801H spec for the part used in the system in bz 10751, and the documentation seems to comply with the spec. Possibly the part doesn't work as advertised, but I think it's more likely there's some subtlety we're still missing.

I'm a little bit surprised that we'd be using pciehp on that Thinkpad R61 instead of acpiphp. The kernel in bz 10751 is so ancient that it might not be negotiating correctly for control of pciehp.

Quoted 6 lines

b) I have a system that where none of the above described HP elements are present implemented, but the controller supports command completion, and sends it out for every write of the slot control register. Thus notification for command complete is generated for pcie_disable_notifiction().

Quoted 7 lines

I don't believe there is a way to differentiate between these 2 classes of systems at init time, unless we try to generate a notification and do or do not get one.

The PCIe specification section 7.8.10 seems to lean towards category
(b) of systems, but today this class of systems shall get penalized by delay of 1 second (per controller) during probe.

Where does this delay happen on your system? I tried to work through the path but I don't see it yet. Here's what I expect to happen on your system:

pciehp_probe
pcie_init
readl(SLTCAP)
if (NO_CMD_CMPL || !(POWER_CTRL | ATTN_LED | PWR_LED | EMI)) # true
ctrl->no_cmd_complete = 1 # set (condition above is true)
writew(SLTSTA, 0x1f) # clear ABP PFD MRLSC PDC CC
pcie_disable_notification
pcie_write_cmd(0, PDCE | ABPE | MRLSCE | PFDE | HPIE | CCIE | DLLSCE)
readw(SLTSTA) # CC = 0 (was cleared above)
writew(SLTCTL)
...
# no waiting here because no_cmd_complete = 1
...
hardware sets SLTSTA.CC
...
pcie_init_notification
pcie_enable_notification
pcie_write_cmd
readw(SLTSTA) # CC = 1 (was set by HW above)
if (SLTSTA.CC) # true

if (!NO_CMD_CMPL)) # true
dbg("Unexpected CMD_COMPLETED. ...") ctrl->no_cmd_complete = 0
writew(SLTCTL)
...
# this time we wait because no_cmd_complete = 0
...
pcie_wait_cmd(..., poll = 1) # now no_cmd_complete = 0

pcie_poll_cmd
readw(SLTSTA) # CC = 1 on first read
if (SLTSTA.CC)
writew(SLTSTA, CC)
return 1

I would think you'd see the "Unexpected CMD_COMPLETED. Need to wait for command completed event" message (if enabled),
but I don't see where the
one second timeout would happen. It looks like we would call pcie_poll_cmd(), but it would return immediately because SLTSTA.CC is already set.

Quoted 24 lines · Bjorn Helgaas

Where does this delay happen on your system? I tried to work through the path but I don't see it yet. Here's what I expect to happen on your system:

pciehp_probe
pcie_init
readl(SLTCAP)
if (NO_CMD_CMPL || !(POWER_CTRL | ATTN_LED | PWR_LED | EMI)) # true
ctrl->no_cmd_complete = 1 # set (condition above is true)
writew(SLTSTA, 0x1f) # clear ABP PFD MRLSC PDC CC
pcie_disable_notification
pcie_write_cmd(0, PDCE | ABPE | MRLSCE | PFDE | HPIE | CCIE | DLLSCE)
readw(SLTSTA) # CC = 0 (was cleared above)
writew(SLTCTL)
...
# no waiting here because no_cmd_complete = 1
...
hardware sets SLTSTA.CC
...
pcie_init_notification
pcie_enable_notification
pcie_write_cmd
readw(SLTSTA) # CC = 1 (was set by HW above)
if (SLTSTA.CC) # true

Quoted 8 lines

if (!NO_CMD_CMPL)) # true
dbg("Unexpected CMD_COMPLETED. ...") ctrl->no_cmd_complete = 0
writew(SLTCTL)
...
# this time we wait because no_cmd_complete = 0
...
pcie_wait_cmd(..., poll = 1) # now no_cmd_complete = 0

Actually pcie_wait_cmd() shall be called with poll=0. That is because the following conditions are both false (we just enabled both the interrupts while enabling notifications):

                if (!(slot_ctrl & PCI_EXP_SLTCTL_HPIE) ||
                    !(slot_ctrl & PCI_EXP_SLTCTL_CCIE))
                        poll = 1;

Thus it does not poll, but waits for interrupt. Which does not happen because the CC bit in the slot status register was not cleared.

As a
result, we get the following messages on the console for each controller (and 1 second delay):
=============> Unexpected CMD_COMPLETED. Need to wait for command completed event. Command not completed in 1000 msec
=============> With my patch, we'd eliminate the 1 second delay and the second line of output above. If we also want to get rid of the first line "Unexpected CMD_COMPLETED..." also, that would probably have to be solved by changing the behavior to assume that CC shall be generated for every SLOTCTRL register write.

Quoted 5 lines

With my patch, we'd eliminate the 1 second delay and the second line of output above. If we also want to get rid of the first line "Unexpected CMD_COMPLETED..." also, that would probably have to be solved by changing the behavior to assume that CC shall be generated for every SLOTCTRL register write.

I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint. We shouldn't complain about hardware that is working perfectly fine. I don't know the best way to do that yet. I have found a box with the same hardware that was fixed by 5808639bfa98, so I hope to play with it and figure out something that will work nicely for both scenarios.

BTW, can you open a report at bugzilla.kernel.org and attach your "lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd like to have the details archived somewhere.

Hello,

Quoted 11 lines

Quoted 5 lines

With my patch, we'd eliminate the 1 second delay and the second line of output above. If we also want to get rid of the first line "Unexpected CMD_COMPLETED..." also, that would probably have to be solved by changing the behavior to assume that CC shall be generated for every SLOTCTRL register write.

I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint. We shouldn't complain about hardware that is working perfectly fine. I don't know the best way to do that yet. I have found a box with the same hardware that was fixed by 5808639bfa98, so I hope to play with it and figure out something that will work nicely for both scenarios.

Please keep posted :-)

Quoted 4 lines

BTW, can you open a report at bugzilla.kernel.org and attach your "lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd like to have the details archived somewhere.

Done:

https://bugzilla.kernel.org/show_bug.cgi?idd821

Please let me know if you need any help in trying something out - I'd be more than keen to help - write code or test.

Quoted 23 lines · Rajat Jain

Hello,

Quoted 11 lines

Quoted 5 lines

With my patch, we'd eliminate the 1 second delay and the second line of output above. If we also want to get rid of the first line "Unexpected CMD_COMPLETED..." also, that would probably have to be solved by changing the behavior to assume that CC shall be generated for every SLOTCTRL register write.

I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint. We shouldn't complain about hardware that is working perfectly fine. I don't know the best way to do that yet. I have found a box with the same hardware that was fixed by 5808639bfa98, so I hope to play with it and figure out something that will work nicely for both scenarios.

Please keep posted :-)

Quoted 4 lines

BTW, can you open a report at bugzilla.kernel.org and attach your "lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd like to have the details archived somewhere.

Done:

Quoted 4 lines

https://bugzilla.kernel.org/show_bug.cgi?idd821

Please let me know if you need any help in trying something out - I'd be more than keen to help - write code or test.

What do you think of the patch below? I'm afraid we'll trip over a few other old parts similar to the 82801H, but I'd rather do that than penalize the parts that work correctly.

PCI: pciehp: Support command completion even with no hotplug hardware

From: Bjorn Helgaas <[email protected]>

Commit 5808639bfa98 ("pciehp: fix slow probing") fixed a slow probing problem on hardware that doesn't conform to the spec, but caused a similar problem for hardware that *does* conform to the spec.

Per PCIe 3.0, sec 7.8.10 and 6.7.3.2, any write to Slot Control generates a hot-plug command. Ports that can accept new commands with no delay can set the "No Command Completed Support" bit. Otherwise the port must indicate its completion of the command and readiness to accept a new command with a "command completed event."

5808639bfa98 assumes ports that lack a power controller, power indicator, attention indicator, and interlock will not generate completion events, even if they neglect to set "No Command Completed Support." But on ports that lack those elements and *do* support command completion notification, it causes:

Unexpected CMD_COMPLETED. Need to wait for command completed event. Command not completed in 1000 msec

and forces us to wait for a 1 second timeout.

This patch makes the 5808639bfa98 workaround into a quirk that's applied only to devices known to be broken, currently just Intel 82801H ports. There are probably other similarly-broken devices that may now probe slowly, but I don't know how to catch them all without penalizing the ones that play by the rules.

Link https://bugzilla.kernel.org/show_bug.cgi?idd821

Reported-by Rajat Jain <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/hotplug/pciehp_hpc.c |   39 +++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

drivers/pci/hotplug/pciehp_hpc.c +30 -10

index 3eea3fdd4b0b..2fd2bd59e07f 100644
@@ -881,6 +881,34 @@ static inline void dbg_ctrl(struct controller *ctrl)
 	ctrl_info(ctrl, "Slot Control           : 0x%04x\n", reg16);
 }
 
+static int pciehp_no_command_complete(struct controller *ctrl)
+{
+	struct pcie_device *dev = ctrl->pcie;
+	struct pci_dev *pdev = dev->port;
+	u16 vendor, device;
+
+	if (NO_CMD_CMPL(ctrl))
+		return 1;
+
+	/*
+	 * Controller should notify on command completion unless the "No
+	 * Command Completed Support" bit is set.  But some hardware does
+	 * not.  See https://bugzilla.kernel.org/show_bug.cgi?id751
+	 */
+	if (!(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl))) {
+		vendor = pdev->vendor;
+		device = pdev->device;
+		if (vendor = PCI_VENDOR_ID_INTEL &&
+		    device >= 0x283f && device <= 0x2849) {
+			dev_info(&dev->device, "device [%04x:%04x] does not notify on hotplug command completion\n",
+				 vendor, device);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 struct controller *pcie_init(struct pcie_device *dev)
 {
 	struct controller *ctrl;

@@ -902,15 +930,8 @@ struct controller *pcie_init(struct pcie_device *dev)
 	mutex_init(&ctrl->ctrl_lock);
 	init_waitqueue_head(&ctrl->queue);
 	dbg_ctrl(ctrl);
-	/*
-	 * Controller doesn't notify of command completion if the "No
-	 * Command Completed Support" bit is set in Slot Capability
-	 * register or the controller supports none of power
-	 * controller, attention led, power led and EMI.
-	 */
-	if (NO_CMD_CMPL(ctrl) ||
-	    !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
-	    ctrl->no_cmd_complete = 1;
+
+	ctrl->no_cmd_complete = pciehp_no_command_complete(ctrl);
 
         /* Check if Data Link Layer Link Active Reporting is implemented */
         if (pciehp_readl(ctrl, PCI_EXP_LNKCAP, &link_cap)) {
--

To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

https://bugzilla.kernel.org/show_bug.cgi?idd821

Please let me know if you need any help in trying something out - I'd be more than keen to help - write code or test.

What do you think of the patch below? I'm afraid we'll trip over a few other old parts similar to the 82801H, but I'd rather do that than penalize the parts that work correctly.

PCI: pciehp: Support command completion even with no hotplug hardware

From: Bjorn Helgaas <[email protected]>

Commit 5808639bfa98 ("pciehp: fix slow probing") fixed a slow probing problem on hardware that doesn't conform to the spec, but caused a similar problem for hardware that *does* conform to the spec.

Per PCIe 3.0, sec 7.8.10 and 6.7.3.2, any write to Slot Control generates a hot-plug command. Ports that can accept new commands with no delay can set the "No Command Completed Support" bit. Otherwise the port must indicate its completion of the command and readiness to accept a new command with a "command completed event."

5808639bfa98 assumes ports that lack a power controller, power indicator, attention indicator, and interlock will not generate completion events, even if they neglect to set "No Command Completed Support." But on ports that lack those elements and *do* support command completion notification, it causes:

Unexpected CMD_COMPLETED. Need to wait for command completed event. Command not completed in 1000 msec

and forces us to wait for a 1 second timeout.

This patch makes the 5808639bfa98 workaround into a quirk that's applied only to devices known to be broken, currently just Intel 82801H ports. There are probably other similarly-broken devices that may now probe slowly, but I don't know how to catch them all without penalizing the ones that play by the rules.

Link https://bugzilla.kernel.org/show_bug.cgi?idd821

Reported-by Rajat Jain <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/hotplug/pciehp_hpc.c |   39 +++++++++++++++++++++++++++++--
-------
 1 file changed, 30 insertions(+), 9 deletions(-)

drivers/pci/hotplug/pciehp_hpc.c +29 -9

index 3eea3fdd4b0b..2fd2bd59e07f 100644
@@ -881,6 +881,34 @@ static inline void dbg_ctrl(struct controller
 *ctrl)
 	ctrl_info(ctrl, "Slot Control           : 0x%04x\n", reg16);
 }
 
+static int pciehp_no_command_complete(struct controller *ctrl) {
+	struct pcie_device *dev = ctrl->pcie;
+	struct pci_dev *pdev = dev->port;
+	u16 vendor, device;
+
+	if (NO_CMD_CMPL(ctrl))
+		return 1;
+
+	/*
+	 * Controller should notify on command completion unless the "No
+	 * Command Completed Support" bit is set.  But some hardware does
+	 * not.  See https://bugzilla.kernel.org/show_bug.cgi?id751
+	 */
+	if (!(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) |
 EMI(ctrl))) {
+		vendor = pdev->vendor;
+		device = pdev->device;
+		if (vendor = PCI_VENDOR_ID_INTEL &&
+		    device >= 0x283f && device <= 0x2849) {
+			dev_info(&dev->device, "device [%04x:%04x] does not
 notify on hotplug command completion\n",
+				 vendor, device);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 struct controller *pcie_init(struct pcie_device *dev)  {
 	struct controller *ctrl;

@@ -902,15 +930,8 @@ struct controller *pcie_init(struct pcie_device
 *dev)
 	mutex_init(&ctrl->ctrl_lock);
 	init_waitqueue_head(&ctrl->queue);
 	dbg_ctrl(ctrl);
-	/*
-	 * Controller doesn't notify of command completion if the "No
-	 * Command Completed Support" bit is set in Slot Capability
-	 * register or the controller supports none of power
-	 * controller, attention led, power led and EMI.
-	 */
-	if (NO_CMD_CMPL(ctrl) ||
-	    !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) |
 EMI(ctrl)))
-	    ctrl->no_cmd_complete = 1;
+
+	ctrl->no_cmd_complete = pciehp_no_command_complete(ctrl);
 

         /* Check if Data Link Layer Link Active Reporting is
implemented */
         if (pciehp_readl(ctrl, PCI_EXP_LNKCAP, &link_cap)) {

Hello Bjorn,

Quoted 3 lines

Quoted 3 lines

https://bugzilla.kernel.org/show_bug.cgi?idd821

Quoted 6 lines

Quoted 2 lines

Please let me know if you need any help in trying something out - I'd be more than keen to help - write code or test.

What do you think of the patch below? I'm afraid we'll trip over a few other old parts similar to the 82801H, but I'd rather do that than penalize the parts that work correctly.

Tested and works fine. Looks good (on a side note, we are introducing device specific check in a common code, but I do not think there is any other cleaner way). I think this patch should be applied as it solves the problem of slow probing.

On a different note, I feel there is still a need to apply my original patch. There is still an open problem in case of spurious interrupts (or in any case where the condition "if (slot_status & PCI_EXP_SLTSTA_CC)" becomes true in pcie_write_cmd()). That is because once that happens, we never clear that interrupt, and no further hotplug interrupts shall be received unless we do that.

Quoted 1 line

On a different note, I feel there is still a need to apply my original

patch. There is still an open problem in case of spurious interrupts (or in any case where the condition "if (slot_status & PCI_EXP_SLTSTA_CC)" becomes true in pcie_write_cmd()). That is because once that happens, we never clear that interrupt, and no further hotplug interrupts shall be received unless we do that.

I agree this is an issue and we should address it somehow. My hesitation is just that I'd prefer to do some more aggressive restructuring rather than apply a point fix. For example:

- We currently look at PCI_EXP_SLTSTA_CC in pcie_isr(), pcie_poll_cmd(), and pcie_write_cmd(). I think it would be better to look at it only in pcie_isr().

- I don't think pcie_poll_cmd() should exist at all; we should poll by calling pcie_isr() instead.

- We need pcie_write_cmd(), but I think the way it waits is backwards. Currently we issue the command, then wait for it to complete. I think
we should issue the command, note the current time, and return without waiting. The *next* time we need to issue a command, we can wait for completion of the previous one (or timeout) if necessary.

But maybe we need the point fix in the interim, especially if anybody can actually produce the scenario you mention.

Hello,

Quoted 10 lines

Quoted 1 line

On a different note, I feel there is still a need to apply my original

patch. There is still an open problem in case of spurious interrupts (or in any case where the condition "if (slot_status & PCI_EXP_SLTSTA_CC)" becomes true in pcie_write_cmd()). That is because once that happens, we never clear that interrupt, and no further hotplug interrupts shall be received unless we do that.

I agree this is an issue and we should address it somehow. My hesitation is just that I'd prefer to do some more aggressive restructuring rather than apply a point fix. For example:

OK, I'll attempt to fix it that way when I get time.

Quoted 14 lines

- We currently look at PCI_EXP_SLTSTA_CC in pcie_isr(), pcie_poll_cmd(), and pcie_write_cmd(). I think it would be better to look at it only in pcie_isr().

- I don't think pcie_poll_cmd() should exist at all; we should poll by calling pcie_isr() instead.

- We need pcie_write_cmd(), but I think the way it waits is backwards. Currently we issue the command, then wait for it to complete. I think
we should issue the command, note the current time, and return without waiting. The *next* time we need to issue a command, we can wait for completion of the previous one (or timeout) if necessary.

Quoted 2 lines

But maybe we need the point fix in the interim, especially if anybody can actually produce the scenario you mention.

Ok.

Quoted 34 lines · Rajat Jain

Quoted 6 lines

Hello,

Quoted 4 lines

Quoted 2 lines

On a different note, I feel there is still a need to apply my original

patch. There is still an open problem in case of spurious interrupts (or in any case where the condition "if (slot_status &

PCI_EXP_SLTSTA_CC)"

Quoted 4 lines

Quoted 4 lines

becomes true in pcie_write_cmd()). That is because once that happens, we never clear that interrupt, and no further hotplug interrupts shall be received unless we do that.

Quoted 15 lines

Quoted 3 lines

I agree this is an issue and we should address it somehow. My hesitation is just that I'd prefer to do some more aggressive restructuring rather than apply a point fix. For example:

OK, I'll attempt to fix it that way when I get time.

Quoted 9 lines

- We currently look at PCI_EXP_SLTSTA_CC in pcie_isr(), pcie_poll_cmd(), and pcie_write_cmd(). I think it would be better to look at it only in pcie_isr().

- I don't think pcie_poll_cmd() should exist at all; we should poll by calling pcie_isr() instead.

- We need pcie_write_cmd(), but I think the way it waits is

backwards.

Quoted 5 lines

Quoted 5 lines

Currently we issue the command, then wait for it to complete. I
think we should issue the command, note the current time, and return without waiting. The *next* time we need to issue a command, we can wait for completion of the previous one (or timeout) if necessary.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

But maybe we need the point fix in the interim, especially if anybody can actually produce the scenario you mention.

Ok.

This patch is still in patchwork, but I've lost track of where we are. Did you resolve this in the series that I just applied, or is it still an outstanding issue?

No, I did not solve it. It is still an outstanding issue. So far I am using your patch to overcome this:

http://www.spinics.net/lists/hotplug/msg05830.html

I'll just attempt to conclude the status on this issue so that you can make the decision on the course of action. IMHO there are 2 independent issues that we discussed in this thread:

1) PCIe compliant HW (that generates cmd completed interrupts at every write of Slot_ctrl register) being penalized with 1 second delay during the boot up. Your patch solves this.

2) If there is a genuine spurious interrupt, it does not get acknowledged. I had originally posted a patch for THIS problem.
http://www.spinics.net/lists/hotplug/msg05815.html

You had indicated that you would rather want a bigger restructuring of the driver to solve (2).

My observation: MY problem (in my setup) is not seen if I use either of the patches (yours or mine).

My opinion: I think my patch solves (2) but might not solve (1) for all corner cases. Also your patch solves (1) but may not solve (2) for all corner cases -Thus we should probably solve both of these problems individually.

Hello Bjorn,

Quoted 56 lines

Quoted 34 lines · Rajat Jain

Quoted 6 lines

Hello,

Quoted 4 lines

Quoted 2 lines

On a different note, I feel there is still a need to apply my original

patch. There is still an open problem in case of spurious interrupts (or in any case where the condition "if (slot_status &

PCI_EXP_SLTSTA_CC)"

Quoted 4 lines

Quoted 4 lines

becomes true in pcie_write_cmd()). That is because once that happens, we never clear that interrupt, and no further hotplug interrupts shall be received unless we do that.

Quoted 15 lines

Quoted 3 lines

I agree this is an issue and we should address it somehow. My hesitation is just that I'd prefer to do some more aggressive restructuring rather than apply a point fix. For example:

OK, I'll attempt to fix it that way when I get time.

Quoted 9 lines

- We currently look at PCI_EXP_SLTSTA_CC in pcie_isr(), pcie_poll_cmd(), and pcie_write_cmd(). I think it would be better to look at it only in pcie_isr().

- I don't think pcie_poll_cmd() should exist at all; we should poll by calling pcie_isr() instead.

- We need pcie_write_cmd(), but I think the way it waits is

backwards.

Quoted 5 lines

Quoted 5 lines

Currently we issue the command, then wait for it to complete. I
think we should issue the command, note the current time, and return without waiting. The *next* time we need to issue a command, we can wait for completion of the previous one (or timeout) if necessary.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

But maybe we need the point fix in the interim, especially if anybody can actually produce the scenario you mention.

Ok.

This patch is still in patchwork, but I've lost track of where we are. Did you resolve this in the series that I just applied, or is it still an outstanding issue?

No, I did not solve it. It is still an outstanding issue. So far I am using your patch to overcome this:

http://www.spinics.net/lists/hotplug/msg05830.html

I'll just attempt to conclude the status on this issue so that you can make the decision on the course of action. IMHO there are 2 independent issues that we discussed in this thread:

1) PCIe compliant HW (that generates cmd completed interrupts at every write of Slot_ctrl register) being penalized with 1 second delay during the boot up. Your patch solves this.

2) If there is a genuine spurious interrupt, it does not get acknowledged. I had originally posted a patch for THIS problem.
http://www.spinics.net/lists/hotplug/msg05815.html

You had indicated that you would rather want a bigger restructuring of the driver to solve (2).

Quoted 4 lines

My observation: MY problem (in my setup) is not seen if I use either of the patches (yours or mine).

My opinion: I think my patch solves (2) but might not solve (1) for all corner cases. Also your patch solves (1) but may not solve (2) for all corner cases -Thus we should probably solve both of these problems individually.

Just wondering if you decided on how to solve this problem?

Are you planning this for 3.15?

Quoted 63 lines · Rajat Jain

Hello Bjorn,

Quoted 60 lines

Quoted 34 lines · Rajat Jain

Quoted 6 lines

Hello,

Quoted 4 lines

Quoted 2 lines

On a different note, I feel there is still a need to apply my original

patch. There is still an open problem in case of spurious interrupts (or in any case where the condition "if (slot_status &

PCI_EXP_SLTSTA_CC)"

Quoted 4 lines

Quoted 4 lines

becomes true in pcie_write_cmd()). That is because once that happens, we never clear that interrupt, and no further hotplug interrupts shall be received unless we do that.

Quoted 15 lines

Quoted 3 lines

I agree this is an issue and we should address it somehow. My hesitation is just that I'd prefer to do some more aggressive restructuring rather than apply a point fix. For example:

OK, I'll attempt to fix it that way when I get time.

Quoted 9 lines

- We currently look at PCI_EXP_SLTSTA_CC in pcie_isr(), pcie_poll_cmd(), and pcie_write_cmd(). I think it would be better to look at it only in pcie_isr().

- I don't think pcie_poll_cmd() should exist at all; we should poll by calling pcie_isr() instead.

- We need pcie_write_cmd(), but I think the way it waits is

backwards.

Quoted 5 lines

Quoted 5 lines

Currently we issue the command, then wait for it to complete. I
think we should issue the command, note the current time, and return without waiting. The *next* time we need to issue a command, we can wait for completion of the previous one (or timeout) if necessary.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

But maybe we need the point fix in the interim, especially if anybody can actually produce the scenario you mention.

Ok.

This patch is still in patchwork, but I've lost track of where we are. Did you resolve this in the series that I just applied, or is it still an outstanding issue?

No, I did not solve it. It is still an outstanding issue. So far I am using your patch to overcome this:

http://www.spinics.net/lists/hotplug/msg05830.html

I'll just attempt to conclude the status on this issue so that you can make the decision on the course of action. IMHO there are 2 independent issues that we discussed in this thread:

1) PCIe compliant HW (that generates cmd completed interrupts at every write of Slot_ctrl register) being penalized with 1 second delay during the boot up. Your patch solves this.

2) If there is a genuine spurious interrupt, it does not get acknowledged. I had originally posted a patch for THIS problem.
http://www.spinics.net/lists/hotplug/msg05815.html

You had indicated that you would rather want a bigger restructuring of the driver to solve (2).

My observation: MY problem (in my setup) is not seen if I use either of the patches (yours or mine).

My opinion: I think my patch solves (2) but might not solve (1) for all corner cases. Also your patch solves (1) but may not solve (2) for all corner cases -Thus we should probably solve both of these problems individually.

Quoted 3 lines

Just wondering if you decided on how to solve this problem?

Are you planning this for 3.15?

Sorry, I haven't had a chance to work on this, so I don't think *I* will get anything done for v3.15. To make forward progress, maybe we should merge your original patch? Would you mind posting it again so it gets into patchwork again?