[PATCH] PCI: add back missing MEM_64 mask check for hotplug path

Discussion:

Yinghai Lu

2014-08-23 01:15:07 UTC

We missed that in
| commit 5b28541552ef5eeffc41d6936105f38c2508e566
| PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources
for pci hotplug path.

We have MEM_64 in type_mask from pci_assign_unassigned_root_bus_resources()
for boot path.

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

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

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -1652,7 +1652,7 @@ void pci_assign_unassigned_bridge_resour
struct pci_dev_resource *fail_res;
int retval;
unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
- IORESOURCE_PREFETCH;
+ IORESOURCE_PREFETCH | IORESOURCE_MEM_64;

again:
__pci_bus_size_bridges(parent, &add_list);

Yinghai Lu

2014-08-23 01:15:08 UTC

Permalink

pcie_poll_cmd() take msecs instead of jiffies, need to convert
timeout to msecs.

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

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

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
@@ -160,7 +160,7 @@ static void pcie_wait_cmd(struct control
ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE)
rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout);
else
- rc = pcie_poll_cmd(ctrl, timeout);
+ rc = pcie_poll_cmd(ctrl, jiffies_to_msecs(timeout));

/*
* Controllers with errata like Intel CF118 don't generate

Bjorn Helgaas

2014-09-23 02:29:45 UTC

Permalink

Post by Yinghai Lu
pcie_poll_cmd() take msecs instead of jiffies, need to convert
timeout to msecs.

Thanks for the fix. I applied this to for-linus for v3.17, because it
fixes a change (40b960831cfa) that we merged for v3.17, and the bug may
cause us to wait less than we should before timing out and issuing a new
hotplug command.

Post by Yinghai Lu
---
drivers/pci/hotplug/pciehp_hpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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
@@ -160,7 +160,7 @@ static void pcie_wait_cmd(struct control
ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE)
rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout);
else
- rc = pcie_poll_cmd(ctrl, timeout);
+ rc = pcie_poll_cmd(ctrl, jiffies_to_msecs(timeout));
/*
* Controllers with errata like Intel CF118 don't generate

Yinghai Lu

2014-08-23 01:15:09 UTC

Permalink

debug print out should add back timeout that pass during wait event or
polling.

That now is cached vaule before wait.

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

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

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
@@ -173,7 +173,7 @@ static void pcie_wait_cmd(struct control
if (!rc)
ctrl_info(ctrl, "Timeout on hotplug command %#010x (issued %u msec ago)\n",
ctrl->slot_ctrl,
- jiffies_to_msecs(now - ctrl->cmd_started));
+ jiffies_to_msecs(now - ctrl->cmd_started + timeout));
}

/**

Bjorn Helgaas

2014-09-23 02:35:01 UTC

Permalink

Post by Yinghai Lu
debug print out should add back timeout that pass during wait event or
polling.
That now is cached vaule before wait.
---
drivers/pci/hotplug/pciehp_hpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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
@@ -173,7 +173,7 @@ static void pcie_wait_cmd(struct control
if (!rc)
ctrl_info(ctrl, "Timeout on hotplug command %#010x (issued %u msec ago)\n",
ctrl->slot_ctrl,
- jiffies_to_msecs(now - ctrl->cmd_started));
+ jiffies_to_msecs(now - ctrl->cmd_started + timeout));
}
/**

I propose the modification below because I think it's more direct. Does it
make sense to you, or am I still missing something?

commit 83d752536aa9dfb1fddb04d6ef8c6bdc75492d4f
Author: Yinghai Lu <***@kernel.org>
Date: Mon Sep 22 20:07:35 2014 -0600

PCI: pciehp: Fix wait time in timeout message

When we warned about a timeout on a hotplug command, we previously printed
the time between calls to pcie_write_cmd(), without accounting for any time
spent actually waiting. Consider this sequence:

pcie_write_cmd
write SLTCTL
cmd_started = jiffies # T1

pcie_write_cmd
pcie_wait_cmd
now = jiffies # T2
wait_event_timeout # we may wait here
if (timeout)
ctrl_info("Timeout on command issued %u msec ago",
jiffies_to_msecs(now - cmd_started))

We previously printed (T2 - T1), but that doesn't include the time spent in
wait_event_timeout().

Fix this by using the current jiffies value, not the one cached before
calling wait_event_timeout().

[bhelgaas: changelog, use current jiffies instead of adding timeout]
Fixes: 40b960831cfa ("PCI: pciehp: Compute timeout from hotplug command start time")
Signed-off-by: Yinghai Lu <***@kernel.org>
Signed-off-by: Bjorn Helgaas <***@google.com>

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 9e0f4aec5f0c..3673a913379f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -173,7 +173,7 @@ static void pcie_wait_cmd(struct controller *ctrl)
if (!rc)
ctrl_info(ctrl, "Timeout on hotplug command %#06x (issued %u msec ago)\n",
ctrl->slot_ctrl,
- jiffies_to_msecs(now - ctrl->cmd_started));
+ jiffies_to_msecs(jiffies - ctrl->cmd_started));
}

/**

Yinghai Lu

2014-09-24 01:53:30 UTC

Permalink

Post by Bjorn Helgaas
[bhelgaas: changelog, use current jiffies instead of adding timeout]
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 9e0f4aec5f0c..3673a913379f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -173,7 +173,7 @@ static void pcie_wait_cmd(struct controller *ctrl)
if (!rc)
ctrl_info(ctrl, "Timeout on hotplug command %#06x (issued %u msec ago)\n",
ctrl->slot_ctrl,
- jiffies_to_msecs(now - ctrl->cmd_started));
+ jiffies_to_msecs(jiffies - ctrl->cmd_started));
}
/**

Yeah, that is more simple.

Thanks

Yinghai

Yinghai Lu

2014-08-23 01:15:10 UTC

Permalink

On hotplug path, we can not touch sibling bridges that is out
side of the slot.

That could happen when BIOS does not assign some bridge BARs and
later can not assign resource to them in first try.

Check if fail dev is the parent bridge, then just use subordinate
bus instead use parent bus.

Reported-by: Andreas Noever <***@gmail.com>
Signed-off-by: Yinghai Lu <***@kernel.org>

---
drivers/pci/setup-bus.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -1676,10 +1676,16 @@ again:
* Try to release leaf bridge's resources that doesn't fit resource of
* child device under that bridge
*/
- list_for_each_entry(fail_res, &fail_head, list)
- pci_bus_release_bridge_resources(fail_res->dev->bus,
+ list_for_each_entry(fail_res, &fail_head, list) {
+ struct pci_bus *bus = fail_res->dev->bus;
+
+ if (fail_res->dev == bridge)
+ bus = bridge->subordinate;
+
+ pci_bus_release_bridge_resources(bus,
fail_res->flags & type_mask,
whole_subtree);
+ }

/* restore size and flags */
list_for_each_entry(fail_res, &fail_head, list) {

Bjorn Helgaas

2014-09-03 23:35:53 UTC

Permalink

Post by Yinghai Lu
On hotplug path, we can not touch sibling bridges that is out
side of the slot.
That could happen when BIOS does not assign some bridge BARs and
later can not assign resource to them in first try.
Check if fail dev is the parent bridge, then just use subordinate
bus instead use parent bus.

This needs a link to the problem report so we can try to match up the
fix with the problem and see how it works.

Post by Yinghai Lu
---
drivers/pci/setup-bus.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
* Try to release leaf bridge's resources that doesn't fit resource of
* child device under that bridge
*/
- list_for_each_entry(fail_res, &fail_head, list)
- pci_bus_release_bridge_resources(fail_res->dev->bus,
+ list_for_each_entry(fail_res, &fail_head, list) {
+ struct pci_bus *bus = fail_res->dev->bus;
+
+ if (fail_res->dev == bridge)
+ bus = bridge->subordinate;
+
+ pci_bus_release_bridge_resources(bus,
fail_res->flags & type_mask,
whole_subtree);
+ }
/* restore size and flags */
list_for_each_entry(fail_res, &fail_head, list) {

Yinghai Lu

2014-08-23 01:15:11 UTC

Permalink

Also move down one printout after pcie_write_cmd to be consistent with
other debug print out.

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

---
drivers/pci/hotplug/pciehp_hpc.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

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
@@ -422,9 +422,9 @@ void pciehp_set_attention_status(struct
default:
return;
}
+ pcie_write_cmd(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC);
ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
- pcie_write_cmd(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC);
}

void pciehp_green_led_on(struct slot *slot)
@@ -602,6 +602,8 @@ void pcie_enable_notification(struct con
PCI_EXP_SLTCTL_DLLSCE);

pcie_write_cmd(ctrl, cmd, mask);
+ ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+ pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
}

static void pcie_disable_notification(struct controller *ctrl)
@@ -613,6 +615,8 @@ static void pcie_disable_notification(st
PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE |
PCI_EXP_SLTCTL_DLLSCE);
pcie_write_cmd(ctrl, 0, mask);
+ ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+ pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);
}

/*
@@ -640,6 +644,8 @@ int pciehp_reset_slot(struct slot *slot,
stat_mask |= PCI_EXP_SLTSTA_DLLSC;

pcie_write_cmd(ctrl, 0, ctrl_mask);
+ ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+ pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);
if (pciehp_poll_mode)
del_timer_sync(&ctrl->poll_timer);

@@ -647,6 +653,8 @@ int pciehp_reset_slot(struct slot *slot,

pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, stat_mask);
pcie_write_cmd(ctrl, ctrl_mask, ctrl_mask);
+ ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+ pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, ctrl_mask);
if (pciehp_poll_mode)
int_poll_timeout(ctrl->poll_timer.data);

Bjorn Helgaas

2014-09-23 02:51:59 UTC

Permalink

Post by Yinghai Lu
Also move down one printout after pcie_write_cmd to be consistent with
other debug print out.

Applied to pci/hotplug for v3.18, thanks!

Post by Yinghai Lu
---
drivers/pci/hotplug/pciehp_hpc.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
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
@@ -422,9 +422,9 @@ void pciehp_set_attention_status(struct
return;
}
+ pcie_write_cmd(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC);
ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_cmd);
- pcie_write_cmd(ctrl, slot_cmd, PCI_EXP_SLTCTL_AIC);
}
void pciehp_green_led_on(struct slot *slot)
@@ -602,6 +602,8 @@ void pcie_enable_notification(struct con
PCI_EXP_SLTCTL_DLLSCE);
pcie_write_cmd(ctrl, cmd, mask);
+ ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+ pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
}
static void pcie_disable_notification(struct controller *ctrl)
@@ -613,6 +615,8 @@ static void pcie_disable_notification(st
PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE |
PCI_EXP_SLTCTL_DLLSCE);
pcie_write_cmd(ctrl, 0, mask);
+ ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+ pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);
}
/*
@@ -640,6 +644,8 @@ int pciehp_reset_slot(struct slot *slot,
stat_mask |= PCI_EXP_SLTSTA_DLLSC;
pcie_write_cmd(ctrl, 0, ctrl_mask);
+ ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+ pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);
if (pciehp_poll_mode)
del_timer_sync(&ctrl->poll_timer);
@@ -647,6 +653,8 @@ int pciehp_reset_slot(struct slot *slot,
pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, stat_mask);
pcie_write_cmd(ctrl, ctrl_mask, ctrl_mask);
+ ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
+ pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, ctrl_mask);
if (pciehp_poll_mode)
int_poll_timeout(ctrl->poll_timer.data);

Yinghai Lu

2014-08-23 01:15:12 UTC

Permalink

disable and enable notification in a row will cause 1 second delay
per hotplug slot on system with Intel chip that only have command
complete for real hotplut operations.

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

---
drivers/pci/hotplug/pciehp_hpc.c | 3 ---
1 file changed, 3 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
@@ -785,9 +785,6 @@ struct controller *pcie_init(struct pcie
PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);

- /* Disable software notification */
- pcie_disable_notification(ctrl);
-
ctrl_info(ctrl, "Slot #%d AttnBtn%c AttnInd%c PwrInd%c PwrCtrl%c MRL%c Interlock%c NoCompl%c LLActRep%c\n",
(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),

Bjorn Helgaas

2014-09-23 02:53:26 UTC

Permalink

Post by Yinghai Lu
disable and enable notification in a row will cause 1 second delay
per hotplug slot on system with Intel chip that only have command
complete for real hotplut operations.

Applied to pci/hotplug for v3.18, thanks!

Post by Yinghai Lu
---
drivers/pci/hotplug/pciehp_hpc.c | 3 ---
1 file changed, 3 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
@@ -785,9 +785,6 @@ struct controller *pcie_init(struct pcie
PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
- /* Disable software notification */
- pcie_disable_notification(ctrl);
-
ctrl_info(ctrl, "Slot #%d AttnBtn%c AttnInd%c PwrInd%c PwrCtrl%c MRL%c Interlock%c NoCompl%c LLActRep%c\n",
(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),

Bjorn Helgaas

2014-09-30 20:35:18 UTC

Permalink

Post by Yinghai Lu
We missed that in
| commit 5b28541552ef5eeffc41d6936105f38c2508e566
| PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources
for pci hotplug path.
We have MEM_64 in type_mask from pci_assign_unassigned_root_bus_resources()
for boot path.

I think we probably need this patch, but I can't figure out what problem it
fixes. If we don't have this, what problem will happen?

Post by Yinghai Lu
---
drivers/pci/setup-bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -1652,7 +1652,7 @@ void pci_assign_unassigned_bridge_resour
struct pci_dev_resource *fail_res;
int retval;
unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
- IORESOURCE_PREFETCH;
+ IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
__pci_bus_size_bridges(parent, &add_list);

Yinghai Lu

2014-10-01 05:06:37 UTC

Permalink

Post by Bjorn Helgaas

I think we probably need this patch, but I can't figure out what problem it
fixes. If we don't have this, what problem will happen?

We can not do exact type checking. Will not use bridge pref that
support 64bit for 64bit pref.

Thanks

Yinghai

Bjorn Helgaas

2014-10-01 20:07:14 UTC

Permalink

Applied to pci/resource for v3.18, thanks.

I added a stable tag for v3.16+, since 5b28541552ef appeared in v3.16.