| Message ID | 20200927032829.11321-5-haifeng.zhao@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix DPC hotplug race and enhance error handling | expand |
On Sat, 2020-09-26 at 23:28 -0400, Ethan Zhao wrote: > simplify the pci_dev_set_io_state() function to only return true > when dev->error_state is changed. [] > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h [] > @@ -362,35 +362,11 @@ static inline bool pci_dev_set_io_state(struct pci_dev *dev, > bool changed = false; [] > + if (dev->error_state == new) > + return changed; > + > + dev->error_state = new; > + changed = true; > return changed; > } This would be simpler removing the unnecessary changed automatic ... if (dev->error_state == new) return false; dev->error_state = new; return true; }
😊 definitely simpler ! -----Original Message----- From: Joe Perches <joe@perches.com> Sent: Sunday, September 27, 2020 12:17 PM To: Zhao, Haifeng <haifeng.zhao@intel.com>; bhelgaas@google.com; oohall@gmail.com; ruscur@russell.cc; lukas@wunner.de; andriy.shevchenko@linux.intel.com; stuart.w.hayes@gmail.com; mr.nuke.me@gmail.com; mika.westerberg@linux.intel.com Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Jia, Pei P <pei.p.jia@intel.com>; ashok.raj@linux.intel.com; Kuppuswamy, Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com> Subject: Re: [PATCH 4/5 V2] PCI: only return true when dev io state is really changed On Sat, 2020-09-26 at 23:28 -0400, Ethan Zhao wrote: > simplify the pci_dev_set_io_state() function to only return true when > dev->error_state is changed. [] > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h [] > @@ -362,35 +362,11 @@ static inline bool pci_dev_set_io_state(struct pci_dev *dev, > bool changed = false; [] > + if (dev->error_state == new) > + return changed; > + > + dev->error_state = new; > + changed = true; > return changed; > } This would be simpler removing the unnecessary changed automatic ... if (dev->error_state == new) return false; dev->error_state = new; return true; }
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index fa12f7cbc1a0..387f891ce6a1 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -362,35 +362,11 @@ static inline bool pci_dev_set_io_state(struct pci_dev *dev, 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; + if (dev->error_state == new) + return changed; + + dev->error_state = new; + changed = true; return changed; }
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 changed. Signed-off-by: Ethan Zhao <haifeng.zhao@intel.com> Tested-by: Wen Jin <wen.jin@intel.com> Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> --- Changes: V2: revise doc and code flow according to Andy's suggestion. drivers/pci/pci.h | 34 +++++----------------------------- 1 file changed, 5 insertions(+), 29 deletions(-)