Hi,folks,
This simple patch set fixed some serious security issues found when DPC
error injection and NVMe SSD hotplug brute force test were doing -- race
condition between DPC handler and pciehp, AER interrupt handlers, caused
system hang and system with DPC feature couldn't recover to normal
working state as expected (NVMe instance lost, mount operation hang,
race PCIe access caused uncorrectable errors reported alternatively etc).
With this patch set applied, stable 5.9-rc6 on ICS (Ice Lake SP platform,
see
https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))
could pass the PCIe Gen4 NVMe SSD brute force hotplug test with any time
interval between hot-remove and plug-in operation tens of times without
any errors occur and system works normal.
With this patch set applied, system with DPC feature could recover from
NON-FATAL and FATAL errors injection test and works as expected.
System works smoothly when errors happen while hotplug is doing, no
uncorrectable errors found.
Brute DPC error injection script:
for i in {0..100}
do
setpci -s 64:02.0 0x196.w=000a
setpci -s 65:00.0 0x04.w=0544
mount /dev/nvme0n1p1 /root/nvme
sleep 1
done
Other details see every commits description part.
This patch set could be applied to stable 5.9-rc6/rc7 directly.
Help to review and test.
v2: changed according to review by Andy Shevchenko.
v3: changed patch 4/5 to simpler coding.
v4: move function pci_wait_port_outdpc() to DPC driver and its
declaration to pci.h. (tip from Christoph Hellwig <[email protected]>).
v5: fix building issue reported by [email protected] with some config.
v6: move patch[3/5] as the first patch according to Lukas's suggestion.
and rewrite the comment part of patch[3/5].
v7: change the patch[4/5], based on Bjorn's code and truth table.
change the patch[5/5] about the debug output information.
Thanks,
Ethan
Ethan Zhao (5):
PCI/ERR: get device before call device driver to avoid NULL pointer
dereference
PCI/DPC: define a function to check and wait till port finish DPC
handling
PCI: pciehp: check and wait port status out of DPC before handling
DLLSC and PDC
PCI: only return true when dev io state is really changed
PCI/ERR: don't mix io state not changed and no driver together
drivers/pci/hotplug/pciehp_hpc.c | 4 ++-
drivers/pci/pci.h | 55 +++++++++++++-------------------
drivers/pci/pcie/dpc.c | 27 ++++++++++++++++
drivers/pci/pcie/err.c | 18 +++++++++--
4 files changed, 68 insertions(+), 36 deletions(-)
base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
--
2.18.4
When uncorrectable error happens, AER driver and DPC driver interrupt
handlers likely call
pcie_do_recovery()
->pci_walk_bus()
->report_frozen_detected()
with pci_channel_io_frozen the same time.
If pci_dev_set_io_state() return true even if the original state is
pci_channel_io_frozen, that will cause AER or DPC handler re-enter
the error detecting and recovery procedure one after another.
The result is the recovery flow mixed between AER and DPC.
So simplify the pci_dev_set_io_state() function to only return true
when dev->error_state is really changed.
Signed-off-by: Ethan Zhao <[email protected]>
Tested-by: Wen Jin <[email protected]>
Tested-by: Shanshan Zhang <[email protected]>
Reviewed-by: Alexandru Gagniuc <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changnes:
v2: revise description and code according to suggestion from Andy.
v3: change code to simpler.
v4: no change.
v5: no change.
v6: no change.
v7: changed based on Bjorn's code and truth table.
drivers/pci/pci.h | 53 ++++++++++++++++++-----------------------------
1 file changed, 20 insertions(+), 33 deletions(-)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 455b32187abd..47af1ff2a286 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -354,44 +354,31 @@ struct pci_sriov {
*
* Must be called with device_lock held.
*
- * Returns true if state has been changed to the requested state.
+ * Returns true if state has been really changed to the requested state.
*/
static inline bool pci_dev_set_io_state(struct pci_dev *dev,
pci_channel_state_t new)
{
- bool changed = false;
-
device_lock_assert(&dev->dev);
- switch (new) {
- case pci_channel_io_perm_failure:
- switch (dev->error_state) {
- case pci_channel_io_frozen:
- case pci_channel_io_normal:
- case pci_channel_io_perm_failure:
- changed = true;
- break;
- }
- break;
- case pci_channel_io_frozen:
- switch (dev->error_state) {
- case pci_channel_io_frozen:
- case pci_channel_io_normal:
- changed = true;
- break;
- }
- break;
- case pci_channel_io_normal:
- switch (dev->error_state) {
- case pci_channel_io_frozen:
- case pci_channel_io_normal:
- changed = true;
- break;
- }
- break;
- }
- if (changed)
- dev->error_state = new;
- return changed;
+
+/*
+ * Truth table:
+ * requested new state
+ * current ------------------------------------------
+ * state normal frozen perm_failure
+ * ------------ + ------------- ------------- ------------
+ * normal | normal frozen perm_failure
+ * frozen | normal frozen perm_failure
+ * perm_failure | perm_failure* perm_failure* perm_failure
+ */
+
+ if (dev->error_state == pci_channel_io_perm_failure)
+ return false;
+ else if (dev->error_state == new)
+ return false;
+
+ dev->error_state = new;
+ return true;
}
static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
--
2.18.4
When root port has DPC capability and it is enabled, then triggered by
errors, DPC DLLSC and PDC etc interrupts will be sent to DPC driver, pciehp
drivers almost at the same time.
That will cause following messed and confused errors handling/recovery/removal
/plugin procedure.
1. Port and device are in error recovery reseting initiated by DPC hardware,
pciehp driver treats them as device is doing hot-remove or hot-plugin the
same time.
2. While DPC handler calling device driver->err_handler callback(
error_detected/resume etc), but the slot may be powered off by
pciehp
-> remove_board()
-> pciehp_power_off_slot().
3. While DPC handler -> pci_do_recovery is doing different action to detect
error and recover based on device->error_state, pciehp driver could change
it on the fly by:
pciehp_unconfigure_device()
->pci_walk_bus()
-> pci_dev_set_disconnected()
4. While DPC handler is calling device driver err_handler callback to detect
error and recover, pciehp driver could is doing device unbind and release
its driver.
...
While NON-FATAL/FATAL errors happen while hotplug is(is not)doing, result
is not determinate.
So we need some kind of synchronization between pciehp DLLSC/PDC handling
and DPC driver error recover handling. we need a determinate result
of DPC error containment, link is recovered, link isn't recovered, device
is still there, device is removed, then do pciehp hot-remove and hot-plugin
procudure, don't mix them together.
Per our test on ICS platform, DPC error containment and software handler will
take 10ms up to 50ms till clean the DPC triggered status. it is quick enough
for pciehp compared with its 1000ms waiting to ignore DLLSC/PDC after doing
power off.
With this patch, the handling flow of DPC containment and hotplug is
partly ordered and serialized, let hardware DPC do the controller reset
etc recovery action first, then DPC driver handling the call-back from
device drivers, clear the DPC status, at the end, pciehp handle the DLLSC
and PDC etc.
After tens of PCIe Gen4 NVMe SSD brute force hot-remove and hot-plugin with
any time internval between the two actions, also stressed with the DPC
injection test. system recovered to normal working state from NON-FATAL/FATAL
errors as expected. hotplug works well without any random undeterminate errors
or malfunction.
Brute DPC error injection script:
for i in {0..100}
do
setpci -s 64:02.0 0x196.w=000a
setpci -s 65:00.0 0x04.w=0544
mount /dev/nvme0n1p1 /root/nvme
sleep 1
done
Signed-off-by: Ethan Zhao <[email protected]>
Tested-by: Wen Jin <[email protected]>
Tested-by: Shanshan Zhang <[email protected]>
---
Changes:
v2: revise doc according to Andy's suggestion.
v3: no change.
v4: no change.
v5: no change.
v6: moved to [3/5] from [2/5] and re-wrote description.
v7: no change.
drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 53433b37e181..6f271160f18d 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -710,8 +710,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
down_read(&ctrl->reset_lock);
if (events & DISABLE_SLOT)
pciehp_handle_disable_request(ctrl);
- else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
+ else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC)) {
+ pci_wait_port_outdpc(pdev);
pciehp_handle_presence_or_link_change(ctrl, events);
+ }
up_read(&ctrl->reset_lock);
ret = IRQ_HANDLED;
--
2.18.4
When we see 'can't recover (no error_detected callback)' on console,
Maybe the reason is io state is not changed by calling
pci_dev_set_io_state(), that is confused. fix it.
Signed-off-by: Ethan Zhao <[email protected]>
Tested-by: Wen Jin <[email protected]>
Tested-by: Shanshan Zhang <[email protected]>
---
Chagnes:
v2: no change.
v3: no change.
v4: no change.
v5: no change.
v6: no change.
v7: change debug output information.
drivers/pci/pcie/err.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index e35c4480c86b..2ca2723f3b34 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -55,8 +55,10 @@ static int report_error_detected(struct pci_dev *dev,
if (!pci_dev_get(dev))
return 0;
device_lock(&dev->dev);
- if (!pci_dev_set_io_state(dev, state) ||
- !dev->driver ||
+ if (!pci_dev_set_io_state(dev, state)) {
+ pci_dbg(dev, "Device was in that state or not allowed setting.\n");
+ vote = PCI_ERS_RESULT_NONE;
+ } else if (!dev->driver ||
!dev->driver->err_handler ||
!dev->driver->err_handler->error_detected) {
/*
--
2.18.4
Once root port DPC capability is enabled and triggered, at the beginning
of DPC is triggered, the DPC status bits are set by hardware and then
sends DPC/DLLSC/PDC interrupts to OS DPC and pciehp drivers, it will
take the port and software DPC interrupt handler 10ms to 50ms (test data
on ICS(Ice Lake SP platform, see
https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server)
& stable 5.9-rc6) to complete the DPC containment procedure
till the DPC status is cleared at the end of the DPC interrupt handler.
We use this function to check if the root port is in DPC handling status
and wait till the hardware and software completed the procedure.
Signed-off-by: Ethan Zhao <[email protected]>
Tested-by: Wen Jin <[email protected]>
Tested-by: Shanshan Zhang <[email protected]>
---
changes:
v2：align ICS code name to public doc.
v3: no change.
v4: response to Christoph's (Christoph Hellwig <[email protected]>)
tip, move pci_wait_port_outdpc() to DPC driver and its declaration
to pci.h.
v5: fix building issue reported by [email protected] with some config.
v6: move from [1/5] to [2/5].
v7: no change.
drivers/pci/pci.h | 2 ++
drivers/pci/pcie/dpc.c | 27 +++++++++++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7cbc1a0..455b32187abd 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -455,10 +455,12 @@ void pci_restore_dpc_state(struct pci_dev *dev);
void pci_dpc_init(struct pci_dev *pdev);
void dpc_process_error(struct pci_dev *pdev);
pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
+bool pci_wait_port_outdpc(struct pci_dev *pdev);
#else
static inline void pci_save_dpc_state(struct pci_dev *dev) {}
static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
static inline void pci_dpc_init(struct pci_dev *pdev) {}
+static inline bool pci_wait_port_outdpc(struct pci_dev *pdev) { return false; }
#endif
#ifdef CONFIG_PCI_ATS
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index daa9a4153776..2e0e091ce923 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -71,6 +71,33 @@ void pci_restore_dpc_state(struct pci_dev *dev)
pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
}
+bool pci_wait_port_outdpc(struct pci_dev *pdev)
+{
+ u16 cap = pdev->dpc_cap, status;
+ u16 loop = 0;
+
+ if (!cap) {
+ pci_WARN_ONCE(pdev, !cap, "No DPC capability initiated\n");
+ return false;
+ }
+ pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
+ pci_dbg(pdev, "DPC status %x, cap %x\n", status, cap);
+
+ while (status & PCI_EXP_DPC_STATUS_TRIGGER && loop < 100) {
+ msleep(10);
+ loop++;
+ pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
+ }
+
+ if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
+ pci_dbg(pdev, "Out of DPC %x, cost %d ms\n", status, loop*10);
+ return true;
+ }
+
+ pci_dbg(pdev, "Timeout to wait port out of DPC status\n");
+ return false;
+}
+
static int dpc_wait_rp_inactive(struct pci_dev *pdev)
{
unsigned long timeout = jiffies + HZ;
--
2.18.4
On Sat, Oct 03, 2020 at 03:55:13AM -0400, Ethan Zhao wrote:
> When uncorrectable error happens, AER driver and DPC driver interrupt
> handlers likely call
>
> pcie_do_recovery()
> ->pci_walk_bus()
> ->report_frozen_detected()
>
> with pci_channel_io_frozen the same time.
> If pci_dev_set_io_state() return true even if the original state is
> pci_channel_io_frozen, that will cause AER or DPC handler re-enter
> the error detecting and recovery procedure one after another.
> The result is the recovery flow mixed between AER and DPC.
> So simplify the pci_dev_set_io_state() function to only return true
> when dev->error_state is really changed.
>
> Signed-off-by: Ethan Zhao <[email protected]>
> Tested-by: Wen Jin <[email protected]>
> Tested-by: Shanshan Zhang <[email protected]>
> Reviewed-by: Alexandru Gagniuc <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> Changnes:
> v2: revise description and code according to suggestion from Andy.
> v3: change code to simpler.
> v4: no change.
> v5: no change.
> v6: no change.
> v7: changed based on Bjorn's code and truth table.
>
> drivers/pci/pci.h | 53 ++++++++++++++++++-----------------------------
> 1 file changed, 20 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 455b32187abd..47af1ff2a286 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -354,44 +354,31 @@ struct pci_sriov {
> *
> * Must be called with device_lock held.
> *
> - * Returns true if state has been changed to the requested state.
> + * Returns true if state has been really changed to the requested state.
> */
> static inline bool pci_dev_set_io_state(struct pci_dev *dev,
> pci_channel_state_t new)
> {
> - bool changed = false;
> -
> device_lock_assert(&dev->dev);
> - switch (new) {
> - case pci_channel_io_perm_failure:
> - switch (dev->error_state) {
> - case pci_channel_io_frozen:
> - case pci_channel_io_normal:
> - case pci_channel_io_perm_failure:
> - changed = true;
> - break;
> - }
> - break;
> - case pci_channel_io_frozen:
> - switch (dev->error_state) {
> - case pci_channel_io_frozen:
> - case pci_channel_io_normal:
> - changed = true;
> - break;
> - }
> - break;
> - case pci_channel_io_normal:
> - switch (dev->error_state) {
> - case pci_channel_io_frozen:
> - case pci_channel_io_normal:
> - changed = true;
> - break;
> - }
> - break;
> - }
> - if (changed)
> - dev->error_state = new;
> - return changed;
> +
> +/*
> + * Truth table:
> + * requested new state
> + * current ------------------------------------------
> + * state normal frozen perm_failure
> + * ------------ + ------------- ------------- ------------
> + * normal | normal frozen perm_failure
> + * frozen | normal frozen perm_failure
> + * perm_failure | perm_failure* perm_failure* perm_failure
> + */
> +
> + if (dev->error_state == pci_channel_io_perm_failure)
> + return false;
> + else if (dev->error_state == new)
> + return false;
> +
> + dev->error_state = new;
> + return true;
No, you missed the point. I want
1) One patch that converts the "switch" to the shorter "if"
statements. This one will be big and ugly, but should not change
the functionality at all, and it should be pretty easy to verify
that since there aren't very many states involved.
Since this one is pure code simplification, the commit log won't
say anything at all about AER or DPC or their requirements
because it's not changing any behavior.
2) A separate patch that's tiny and makes whatever functional change
you need.
> }
>
> static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
> --
> 2.18.4
>
Hi Ethan
On Sat, Oct 03, 2020 at 03:55:09AM -0400, Ethan Zhao wrote:
> Hi,folks,
>
> This simple patch set fixed some serious security issues found when DPC
> error injection and NVMe SSD hotplug brute force test were doing -- race
> condition between DPC handler and pciehp, AER interrupt handlers, caused
> system hang and system with DPC feature couldn't recover to normal
> working state as expected (NVMe instance lost, mount operation hang,
> race PCIe access caused uncorrectable errors reported alternatively etc).
I think maybe picking from other commit messages to make this description in
cover letter bit clear. The fundamental premise is that when due to error
conditions when events are processed by both DPC handler and hotplug handling of
DLLSC both operating on the same device object ends up with crashes.
>
> With this patch set applied, stable 5.9-rc6 on ICS (Ice Lake SP platform,
> see
> https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))
>
> could pass the PCIe Gen4 NVMe SSD brute force hotplug test with any time
> interval between hot-remove and plug-in operation tens of times without
> any errors occur and system works normal.
>
> With this patch set applied, system with DPC feature could recover from
> NON-FATAL and FATAL errors injection test and works as expected.
>
> System works smoothly when errors happen while hotplug is doing, no
> uncorrectable errors found.
>
> Brute DPC error injection script:
>
> for i in {0..100}
> do
> setpci -s 64:02.0 0x196.w=000a
> setpci -s 65:00.0 0x04.w=0544
> mount /dev/nvme0n1p1 /root/nvme
> sleep 1
> done
>
> Other details see every commits description part.
>
> This patch set could be applied to stable 5.9-rc6/rc7 directly.
>
> Help to review and test.
>
> v2: changed according to review by Andy Shevchenko.
> v3: changed patch 4/5 to simpler coding.
> v4: move function pci_wait_port_outdpc() to DPC driver and its
> declaration to pci.h. (tip from Christoph Hellwig <[email protected]>).
> v5: fix building issue reported by [email protected] with some config.
> v6: move patch[3/5] as the first patch according to Lukas's suggestion.
> and rewrite the comment part of patch[3/5].
> v7: change the patch[4/5], based on Bjorn's code and truth table.
> change the patch[5/5] about the debug output information.
>
> Thanks,
> Ethan
>
>
> Ethan Zhao (5):
> PCI/ERR: get device before call device driver to avoid NULL pointer
> dereference
> PCI/DPC: define a function to check and wait till port finish DPC
> handling
> PCI: pciehp: check and wait port status out of DPC before handling
> DLLSC and PDC
> PCI: only return true when dev io state is really changed
> PCI/ERR: don't mix io state not changed and no driver together
>
> drivers/pci/hotplug/pciehp_hpc.c | 4 ++-
> drivers/pci/pci.h | 55 +++++++++++++-------------------
> drivers/pci/pcie/dpc.c | 27 ++++++++++++++++
> drivers/pci/pcie/err.c | 18 +++++++++--
> 4 files changed, 68 insertions(+), 36 deletions(-)
>
>
> base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
> --
> 2.18.4
>
On Sat, Oct 03, 2020 at 03:55:12AM -0400, Ethan Zhao wrote:
> When root port has DPC capability and it is enabled, then triggered by
> errors, DPC DLLSC and PDC etc interrupts will be sent to DPC driver, pciehp
> drivers almost at the same time.
Do the DLLSC and PDC events occur as a result of handling the error
or do they occur independently?
If the latter, I don't see how we can tell whether the card in the
slot is still the same.
If the former, holding the hotplug slot's reset_lock and doing something
along the lines of pciehp_reset_slot() (or calling it directly) might
solve the race.
Thanks,
Lukas
Raj,
On Sun, Oct 4, 2020 at 12:57 PM Raj, Ashok <[email protected]> wrote:
>
> Hi Ethan
>
> On Sat, Oct 03, 2020 at 03:55:09AM -0400, Ethan Zhao wrote:
> > Hi,folks,
> >
> > This simple patch set fixed some serious security issues found when DPC
> > error injection and NVMe SSD hotplug brute force test were doing -- race
> > condition between DPC handler and pciehp, AER interrupt handlers, caused
> > system hang and system with DPC feature couldn't recover to normal
> > working state as expected (NVMe instance lost, mount operation hang,
> > race PCIe access caused uncorrectable errors reported alternatively etc).
>
> I think maybe picking from other commit messages to make this description in
> cover letter bit clear. The fundamental premise is that when due to error
> conditions when events are processed by both DPC handler and hotplug handling of
> DLLSC both operating on the same device object ends up with crashes.
Yep, that's right.
Thanks,
Ethan
>
>
> >
> > With this patch set applied, stable 5.9-rc6 on ICS (Ice Lake SP platform,
> > see
> > https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))
> >
> > could pass the PCIe Gen4 NVMe SSD brute force hotplug test with any time
> > interval between hot-remove and plug-in operation tens of times without
> > any errors occur and system works normal.
>
> >
> > With this patch set applied, system with DPC feature could recover from
> > NON-FATAL and FATAL errors injection test and works as expected.
> >
> > System works smoothly when errors happen while hotplug is doing, no
> > uncorrectable errors found.
> >
> > Brute DPC error injection script:
> >
> > for i in {0..100}
> > do
> > setpci -s 64:02.0 0x196.w=000a
> > setpci -s 65:00.0 0x04.w=0544
> > mount /dev/nvme0n1p1 /root/nvme
> > sleep 1
> > done
> >
> > Other details see every commits description part.
> >
> > This patch set could be applied to stable 5.9-rc6/rc7 directly.
> >
> > Help to review and test.
> >
> > v2: changed according to review by Andy Shevchenko.
> > v3: changed patch 4/5 to simpler coding.
> > v4: move function pci_wait_port_outdpc() to DPC driver and its
> > declaration to pci.h. (tip from Christoph Hellwig <[email protected]>).
> > v5: fix building issue reported by [email protected] with some config.
> > v6: move patch[3/5] as the first patch according to Lukas's suggestion.
> > and rewrite the comment part of patch[3/5].
> > v7: change the patch[4/5], based on Bjorn's code and truth table.
> > change the patch[5/5] about the debug output information.
> >
> > Thanks,
> > Ethan
> >
> >
> > Ethan Zhao (5):
> > PCI/ERR: get device before call device driver to avoid NULL pointer
> > dereference
> > PCI/DPC: define a function to check and wait till port finish DPC
> > handling
> > PCI: pciehp: check and wait port status out of DPC before handling
> > DLLSC and PDC
> > PCI: only return true when dev io state is really changed
> > PCI/ERR: don't mix io state not changed and no driver together
> >
> > drivers/pci/hotplug/pciehp_hpc.c | 4 ++-
> > drivers/pci/pci.h | 55 +++++++++++++-------------------
> > drivers/pci/pcie/dpc.c | 27 ++++++++++++++++
> > drivers/pci/pcie/err.c | 18 +++++++++--
> > 4 files changed, 68 insertions(+), 36 deletions(-)
> >
> >
> > base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
> > --
> > 2.18.4
> >
Lukas,
On Mon, Oct 5, 2020 at 3:13 AM Lukas Wunner <[email protected]> wrote:
>
> On Sat, Oct 03, 2020 at 03:55:12AM -0400, Ethan Zhao wrote:
> > When root port has DPC capability and it is enabled, then triggered by
> > errors, DPC DLLSC and PDC etc interrupts will be sent to DPC driver, pciehp
> > drivers almost at the same time.
>
> Do the DLLSC and PDC events occur as a result of handling the error
> or do they occur independently?
They could happen independently if links were recovered then the card
was removed.
They could happen as a result of handling the errors the same time.
So don't assume DLLSC and PDC all occur at the same time.
>
> If the latter, I don't see how we can tell whether the card in the
> slot is still the same.
If PDC happens, the card in the slot might not be the same. so
hot-removal /hot -plugin handling follows the PDC event.
>
> If the former, holding the hotplug slot's reset_lock and doing something
> along the lines of pciehp_reset_slot() (or calling it directly) might
> solve the race.
DPC reset is done by hardware, only AER calls pciehp_reset_slot() as recovery
handling initiated by software.
Thanks,
Ethan
>
> Thanks,
>
> Lukas
Bjorn,
On Sun, Oct 4, 2020 at 12:44 AM Bjorn Helgaas <[email protected]> wrote:
>
> On Sat, Oct 03, 2020 at 03:55:13AM -0400, Ethan Zhao wrote:
> > When uncorrectable error happens, AER driver and DPC driver interrupt
> > handlers likely call
> >
> > pcie_do_recovery()
> > ->pci_walk_bus()
> > ->report_frozen_detected()
> >
> > with pci_channel_io_frozen the same time.
> > If pci_dev_set_io_state() return true even if the original state is
> > pci_channel_io_frozen, that will cause AER or DPC handler re-enter
> > the error detecting and recovery procedure one after another.
> > The result is the recovery flow mixed between AER and DPC.
> > So simplify the pci_dev_set_io_state() function to only return true
> > when dev->error_state is really changed.
> >
> > Signed-off-by: Ethan Zhao <[email protected]>
> > Tested-by: Wen Jin <[email protected]>
> > Tested-by: Shanshan Zhang <[email protected]>
> > Reviewed-by: Alexandru Gagniuc <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > ---
> > Changnes:
> > v2: revise description and code according to suggestion from Andy.
> > v3: change code to simpler.
> > v4: no change.
> > v5: no change.
> > v6: no change.
> > v7: changed based on Bjorn's code and truth table.
> >
> > drivers/pci/pci.h | 53 ++++++++++++++++++-----------------------------
> > 1 file changed, 20 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 455b32187abd..47af1ff2a286 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -354,44 +354,31 @@ struct pci_sriov {
> > *
> > * Must be called with device_lock held.
> > *
> > - * Returns true if state has been changed to the requested state.
> > + * Returns true if state has been really changed to the requested state.
> > */
> > static inline bool pci_dev_set_io_state(struct pci_dev *dev,
> > pci_channel_state_t new)
> > {
> > - bool changed = false;
> > -
> > device_lock_assert(&dev->dev);
> > - switch (new) {
> > - case pci_channel_io_perm_failure:
> > - switch (dev->error_state) {
> > - case pci_channel_io_frozen:
> > - case pci_channel_io_normal:
> > - case pci_channel_io_perm_failure:
> > - changed = true;
> > - break;
> > - }
> > - break;
> > - case pci_channel_io_frozen:
> > - switch (dev->error_state) {
> > - case pci_channel_io_frozen:
> > - case pci_channel_io_normal:
> > - changed = true;
> > - break;
> > - }
> > - break;
> > - case pci_channel_io_normal:
> > - switch (dev->error_state) {
> > - case pci_channel_io_frozen:
> > - case pci_channel_io_normal:
> > - changed = true;
> > - break;
> > - }
> > - break;
> > - }
> > - if (changed)
> > - dev->error_state = new;
> > - return changed;
> > +
> > +/*
> > + * Truth table:
> > + * requested new state
> > + * current ------------------------------------------
> > + * state normal frozen perm_failure
> > + * ------------ + ------------- ------------- ------------
> > + * normal | normal frozen perm_failure
> > + * frozen | normal frozen perm_failure
> > + * perm_failure | perm_failure* perm_failure* perm_failure
> > + */
> > +
> > + if (dev->error_state == pci_channel_io_perm_failure)
> > + return false;
> > + else if (dev->error_state == new)
> > + return false;
> > +
> > + dev->error_state = new;
> > + return true;
>
> No, you missed the point. I want
>
> 1) One patch that converts the "switch" to the shorter "if"
> statements. This one will be big and ugly, but should not change
> the functionality at all, and it should be pretty easy to verify
> that since there aren't very many states involved.
>
> Since this one is pure code simplification, the commit log won't
> say anything at all about AER or DPC or their requirements
> because it's not changing any behavior.
>
> 2) A separate patch that's tiny and makes whatever functional change
> you need.
Make sense, clear, this time.
Thanks,
Ethan
>
> > }
> >
> > static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
> > --
> > 2.18.4
> >