* [PATCH] PCI/AER: save/restore AER registers during suspend/resume @ 2019-07-09 8:00 Patel, Mayurkumar 2019-07-09 18:31 ` sathyanarayanan kuppuswamy 0 siblings, 1 reply; 10+ messages in thread From: Patel, Mayurkumar @ 2019-07-09 8:00 UTC (permalink / raw) To: linux-pci, 'Bjorn Helgaas' Cc: Patel, Mayurkumar, mika.westerberg, 'Andy Shevchenko' After system suspend/resume cycle AER registers settings are lost. Not restoring Root Error Command Register bits if it were set, keeps AER interrupts disabled after system resume. Moreover, AER mask and severity registers are also required to be restored back to AER settings prior to system suspend. Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> --- drivers/pci/pci.c | 2 ++ drivers/pci/pcie/aer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/aer.h | 4 ++++ 3 files changed, 55 insertions(+) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 8abc843..40d5507 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1340,6 +1340,7 @@ int pci_save_state(struct pci_dev *dev) pci_save_ltr_state(dev); pci_save_dpc_state(dev); + pci_save_aer_state(dev); return pci_save_vc_state(dev); } EXPORT_SYMBOL(pci_save_state); @@ -1453,6 +1454,7 @@ void pci_restore_state(struct pci_dev *dev) pci_restore_dpc_state(dev); pci_cleanup_aer_error_status_regs(dev); + pci_restore_aer_state(dev); pci_restore_config_space(dev); diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index b45bc47..1acc641 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -448,6 +448,54 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) return 0; } +void pci_save_aer_state(struct pci_dev *dev) +{ + int pos = 0; + struct pci_cap_saved_state *save_state; + u32 *cap; + + if (!pci_is_pcie(dev)) + return; + + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); + if (!save_state) + return; + + pos = dev->aer_cap; + if (!pos) + return; + + cap = &save_state->cap.data[0]; + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, cap++); + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, cap++); + pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, cap++); + pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++); +} + +void pci_restore_aer_state(struct pci_dev *dev) +{ + int pos = 0; + struct pci_cap_saved_state *save_state; + u32 *cap; + + if (!pci_is_pcie(dev)) + return; + + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); + if (!save_state) + return; + + pos = dev->aer_cap; + if (!pos) + return; + + cap = &save_state->cap.data[0]; + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, *cap++); + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, *cap++); + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, *cap++); + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, *cap++); +} + void pci_aer_init(struct pci_dev *dev) { dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); @@ -1396,6 +1444,7 @@ static int aer_probe(struct pcie_device *dev) return status; } + pci_add_ext_cap_save_buffer(port, PCI_EXT_CAP_ID_ERR, sizeof(u32) * 4); aer_enable_rootport(rpc); pci_info(port, "enabled with IRQ %d\n", dev->irq); return 0; diff --git a/include/linux/aer.h b/include/linux/aer.h index 514bffa..fa19e01 100644 --- a/include/linux/aer.h +++ b/include/linux/aer.h @@ -46,6 +46,8 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev); int pci_disable_pcie_error_reporting(struct pci_dev *dev); int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev); int pci_cleanup_aer_error_status_regs(struct pci_dev *dev); +void pci_save_aer_state(struct pci_dev *dev); +void pci_restore_aer_state(struct pci_dev *dev); #else static inline int pci_enable_pcie_error_reporting(struct pci_dev *dev) { @@ -63,6 +65,8 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) { return -EINVAL; } +static inline void pci_save_aer_state(struct pci_dev *dev) {} +static inline void pci_restore_aer_state(struct pci_dev *dev) {} #endif void cper_print_aer(struct pci_dev *dev, int aer_severity, -- 2.7.4 Mayurkumar Patel Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Gary Kershaw Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-09 8:00 [PATCH] PCI/AER: save/restore AER registers during suspend/resume Patel, Mayurkumar @ 2019-07-09 18:31 ` sathyanarayanan kuppuswamy 2019-07-10 17:22 ` Patel, Mayurkumar 0 siblings, 1 reply; 10+ messages in thread From: sathyanarayanan kuppuswamy @ 2019-07-09 18:31 UTC (permalink / raw) To: Patel, Mayurkumar, linux-pci, 'Bjorn Helgaas' Cc: mika.westerberg, 'Andy Shevchenko' Hi, On 7/9/19 1:00 AM, Patel, Mayurkumar wrote: > After system suspend/resume cycle AER registers settings are > lost. Not restoring Root Error Command Register bits if it were > set, keeps AER interrupts disabled after system resume. > Moreover, AER mask and severity registers are also required > to be restored back to AER settings prior to system suspend. > > Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> > --- > drivers/pci/pci.c | 2 ++ > drivers/pci/pcie/aer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/aer.h | 4 ++++ > 3 files changed, 55 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 8abc843..40d5507 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1340,6 +1340,7 @@ int pci_save_state(struct pci_dev *dev) > > pci_save_ltr_state(dev); > pci_save_dpc_state(dev); > + pci_save_aer_state(dev); > return pci_save_vc_state(dev); > } > EXPORT_SYMBOL(pci_save_state); > @@ -1453,6 +1454,7 @@ void pci_restore_state(struct pci_dev *dev) > pci_restore_dpc_state(dev); > > pci_cleanup_aer_error_status_regs(dev); > + pci_restore_aer_state(dev); > > pci_restore_config_space(dev); > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index b45bc47..1acc641 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -448,6 +448,54 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) > return 0; > } > > +void pci_save_aer_state(struct pci_dev *dev) > +{ > + int pos = 0; > + struct pci_cap_saved_state *save_state; > + u32 *cap; > + > + if (!pci_is_pcie(dev)) > + return; > + > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > + if (!save_state) > + return; > + > + pos = dev->aer_cap; > + if (!pos) > + return; > + > + cap = &save_state->cap.data[0]; > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, cap++); > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, cap++); > + pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, cap++); > + pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++); I don't see AER driver modifying UNCOR_MASK/SEVER/COR_MASK register. If all it has is default value then why do you want to preserve it ? Also, any reason for not preserving ECRC settings ? > +} > + > +void pci_restore_aer_state(struct pci_dev *dev) > +{ > + int pos = 0; > + struct pci_cap_saved_state *save_state; > + u32 *cap; > + > + if (!pci_is_pcie(dev)) > + return; > + > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > + if (!save_state) > + return; > + > + pos = dev->aer_cap; > + if (!pos) > + return; > + > + cap = &save_state->cap.data[0]; > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, *cap++); > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, *cap++); > + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, *cap++); > + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, *cap++); > +} > + > void pci_aer_init(struct pci_dev *dev) > { > dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > @@ -1396,6 +1444,7 @@ static int aer_probe(struct pcie_device *dev) > return status; > } > > + pci_add_ext_cap_save_buffer(port, PCI_EXT_CAP_ID_ERR, sizeof(u32) * 4); > aer_enable_rootport(rpc); > pci_info(port, "enabled with IRQ %d\n", dev->irq); > return 0; > diff --git a/include/linux/aer.h b/include/linux/aer.h > index 514bffa..fa19e01 100644 > --- a/include/linux/aer.h > +++ b/include/linux/aer.h > @@ -46,6 +46,8 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev); > int pci_disable_pcie_error_reporting(struct pci_dev *dev); > int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev); > int pci_cleanup_aer_error_status_regs(struct pci_dev *dev); > +void pci_save_aer_state(struct pci_dev *dev); > +void pci_restore_aer_state(struct pci_dev *dev); > #else > static inline int pci_enable_pcie_error_reporting(struct pci_dev *dev) > { > @@ -63,6 +65,8 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) > { > return -EINVAL; > } > +static inline void pci_save_aer_state(struct pci_dev *dev) {} > +static inline void pci_restore_aer_state(struct pci_dev *dev) {} > #endif > > void cper_print_aer(struct pci_dev *dev, int aer_severity, -- Sathyanarayanan Kuppuswamy Linux kernel developer ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-09 18:31 ` sathyanarayanan kuppuswamy @ 2019-07-10 17:22 ` Patel, Mayurkumar 2019-07-10 17:36 ` sathyanarayanan kuppuswamy 0 siblings, 1 reply; 10+ messages in thread From: Patel, Mayurkumar @ 2019-07-10 17:22 UTC (permalink / raw) To: 'sathyanarayanan.kuppuswamy@linux.intel.com', linux-pci, 'Bjorn Helgaas' Cc: mika.westerberg, 'Andy Shevchenko' Hi, > Hi, > > On 7/9/19 1:00 AM, Patel, Mayurkumar wrote: > > After system suspend/resume cycle AER registers settings are > > lost. Not restoring Root Error Command Register bits if it were > > set, keeps AER interrupts disabled after system resume. > > Moreover, AER mask and severity registers are also required > > to be restored back to AER settings prior to system suspend. > > > > Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> > > --- > > drivers/pci/pci.c | 2 ++ > > drivers/pci/pcie/aer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/aer.h | 4 ++++ > > 3 files changed, 55 insertions(+) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 8abc843..40d5507 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -1340,6 +1340,7 @@ int pci_save_state(struct pci_dev *dev) > > > > pci_save_ltr_state(dev); > > pci_save_dpc_state(dev); > > + pci_save_aer_state(dev); > > return pci_save_vc_state(dev); > > } > > EXPORT_SYMBOL(pci_save_state); > > @@ -1453,6 +1454,7 @@ void pci_restore_state(struct pci_dev *dev) > > pci_restore_dpc_state(dev); > > > > pci_cleanup_aer_error_status_regs(dev); > > + pci_restore_aer_state(dev); > > > > pci_restore_config_space(dev); > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index b45bc47..1acc641 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -448,6 +448,54 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) > > return 0; > > } > > > > +void pci_save_aer_state(struct pci_dev *dev) > > +{ > > + int pos = 0; > > + struct pci_cap_saved_state *save_state; > > + u32 *cap; > > + > > + if (!pci_is_pcie(dev)) > > + return; > > + > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > > + if (!save_state) > > + return; > > + > > + pos = dev->aer_cap; > > + if (!pos) > > + return; > > + > > + cap = &save_state->cap.data[0]; > > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, cap++); > > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, cap++); > > + pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, cap++); > > + pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++); > > I don't see AER driver modifying UNCOR_MASK/SEVER/COR_MASK register. If > all it has is default value then why do you want to preserve it ? > Thanks for reply. You are right about UNCOR_MASK/SEVER/COR_MASK mask AER driver leaves untouched, But IMHO users for example can use "setpci" to unmask specific errors and its severity based on their debugging requirement on Root port and/or Endpoint. So during resume, if PCIe endpoint fails due to any error which by default stays masked then it can't be catched and/or debugged. Moreover, Endpoint driver may also unmask during "driver probe" certain specific errors for endpoint, which needs to be restored while resume. @Bjorn/Anybody else has any opinion to cache/restore UNCOR_MASK/SEVER/COR_MASK registers? Please help to comment. > Also, any reason for not preserving ECRC settings ? No specific reason. I can incorporte that with v2 of this patchset but I don’t have HW on which I can validate that. > > > +} > > + > > +void pci_restore_aer_state(struct pci_dev *dev) > > +{ > > + int pos = 0; > > + struct pci_cap_saved_state *save_state; > > + u32 *cap; > > + > > + if (!pci_is_pcie(dev)) > > + return; > > + > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > > + if (!save_state) > > + return; > > + > > + pos = dev->aer_cap; > > + if (!pos) > > + return; > > + > > + cap = &save_state->cap.data[0]; > > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, *cap++); > > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, *cap++); > > + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, *cap++); > > + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, *cap++); > > +} > > + > > void pci_aer_init(struct pci_dev *dev) > > { > > dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > > @@ -1396,6 +1444,7 @@ static int aer_probe(struct pcie_device *dev) > > return status; > > } > > > > + pci_add_ext_cap_save_buffer(port, PCI_EXT_CAP_ID_ERR, sizeof(u32) * 4); > > aer_enable_rootport(rpc); > > pci_info(port, "enabled with IRQ %d\n", dev->irq); > > return 0; > > diff --git a/include/linux/aer.h b/include/linux/aer.h > > index 514bffa..fa19e01 100644 > > --- a/include/linux/aer.h > > +++ b/include/linux/aer.h > > @@ -46,6 +46,8 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev); > > int pci_disable_pcie_error_reporting(struct pci_dev *dev); > > int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev); > > int pci_cleanup_aer_error_status_regs(struct pci_dev *dev); > > +void pci_save_aer_state(struct pci_dev *dev); > > +void pci_restore_aer_state(struct pci_dev *dev); > > #else > > static inline int pci_enable_pcie_error_reporting(struct pci_dev *dev) > > { > > @@ -63,6 +65,8 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) > > { > > return -EINVAL; > > } > > +static inline void pci_save_aer_state(struct pci_dev *dev) {} > > +static inline void pci_restore_aer_state(struct pci_dev *dev) {} > > #endif > > > > void cper_print_aer(struct pci_dev *dev, int aer_severity, > > -- > Sathyanarayanan Kuppuswamy > Linux kernel developer Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Gary Kershaw Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-10 17:22 ` Patel, Mayurkumar @ 2019-07-10 17:36 ` sathyanarayanan kuppuswamy 2019-07-24 18:45 ` Bjorn Helgaas 0 siblings, 1 reply; 10+ messages in thread From: sathyanarayanan kuppuswamy @ 2019-07-10 17:36 UTC (permalink / raw) To: Patel, Mayurkumar, linux-pci, 'Bjorn Helgaas' Cc: mika.westerberg, 'Andy Shevchenko' Hi, On 7/10/19 10:22 AM, Patel, Mayurkumar wrote: > Hi, > > >> Hi, >> >> On 7/9/19 1:00 AM, Patel, Mayurkumar wrote: >>> After system suspend/resume cycle AER registers settings are >>> lost. Not restoring Root Error Command Register bits if it were >>> set, keeps AER interrupts disabled after system resume. >>> Moreover, AER mask and severity registers are also required >>> to be restored back to AER settings prior to system suspend. >>> >>> Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> >>> --- >>> drivers/pci/pci.c | 2 ++ >>> drivers/pci/pcie/aer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/aer.h | 4 ++++ >>> 3 files changed, 55 insertions(+) >>> >>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>> index 8abc843..40d5507 100644 >>> --- a/drivers/pci/pci.c >>> +++ b/drivers/pci/pci.c >>> @@ -1340,6 +1340,7 @@ int pci_save_state(struct pci_dev *dev) >>> >>> pci_save_ltr_state(dev); >>> pci_save_dpc_state(dev); >>> + pci_save_aer_state(dev); >>> return pci_save_vc_state(dev); >>> } >>> EXPORT_SYMBOL(pci_save_state); >>> @@ -1453,6 +1454,7 @@ void pci_restore_state(struct pci_dev *dev) >>> pci_restore_dpc_state(dev); >>> >>> pci_cleanup_aer_error_status_regs(dev); >>> + pci_restore_aer_state(dev); >>> >>> pci_restore_config_space(dev); >>> >>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >>> index b45bc47..1acc641 100644 >>> --- a/drivers/pci/pcie/aer.c >>> +++ b/drivers/pci/pcie/aer.c >>> @@ -448,6 +448,54 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) >>> return 0; >>> } >>> >>> +void pci_save_aer_state(struct pci_dev *dev) >>> +{ >>> + int pos = 0; >>> + struct pci_cap_saved_state *save_state; >>> + u32 *cap; >>> + >>> + if (!pci_is_pcie(dev)) >>> + return; >>> + >>> + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); >>> + if (!save_state) >>> + return; >>> + >>> + pos = dev->aer_cap; >>> + if (!pos) >>> + return; >>> + >>> + cap = &save_state->cap.data[0]; >>> + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, cap++); >>> + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, cap++); >>> + pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, cap++); >>> + pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++); >> I don't see AER driver modifying UNCOR_MASK/SEVER/COR_MASK register. If >> all it has is default value then why do you want to preserve it ? >> > Thanks for reply. > You are right about UNCOR_MASK/SEVER/COR_MASK mask AER driver leaves untouched, > But IMHO users for example can use "setpci" to unmask specific errors and its severity based on > their debugging requirement on Root port and/or Endpoint. So during resume, if PCIe endpoint > fails due to any error which by default stays masked then it can't be catched and/or debugged. > Moreover, Endpoint driver may also unmask during "driver probe" certain specific > errors for endpoint, which needs to be restored while resume. Isn't these registers configuration usually done by firmware ? I think user application rarely touch them. Also, IMO, Caching/Restoring registers under the assumption that it might be useful for user if they modified it is not a convincing argument. But I will let Bjorn and others decide whether its alright to cache these registers. > > @Bjorn/Anybody else has any opinion to cache/restore UNCOR_MASK/SEVER/COR_MASK registers? > Please help to comment. > >> Also, any reason for not preserving ECRC settings ? > No specific reason. I can incorporte that with v2 of this patchset but I don’t have HW on which I can validate that. > > >>> +} >>> + >>> +void pci_restore_aer_state(struct pci_dev *dev) >>> +{ >>> + int pos = 0; >>> + struct pci_cap_saved_state *save_state; >>> + u32 *cap; >>> + >>> + if (!pci_is_pcie(dev)) >>> + return; >>> + >>> + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); >>> + if (!save_state) >>> + return; >>> + >>> + pos = dev->aer_cap; >>> + if (!pos) >>> + return; >>> + >>> + cap = &save_state->cap.data[0]; >>> + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, *cap++); >>> + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, *cap++); >>> + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, *cap++); >>> + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, *cap++); >>> +} >>> + >>> void pci_aer_init(struct pci_dev *dev) >>> { >>> dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); >>> @@ -1396,6 +1444,7 @@ static int aer_probe(struct pcie_device *dev) >>> return status; >>> } >>> >>> + pci_add_ext_cap_save_buffer(port, PCI_EXT_CAP_ID_ERR, sizeof(u32) * 4); >>> aer_enable_rootport(rpc); >>> pci_info(port, "enabled with IRQ %d\n", dev->irq); >>> return 0; >>> diff --git a/include/linux/aer.h b/include/linux/aer.h >>> index 514bffa..fa19e01 100644 >>> --- a/include/linux/aer.h >>> +++ b/include/linux/aer.h >>> @@ -46,6 +46,8 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev); >>> int pci_disable_pcie_error_reporting(struct pci_dev *dev); >>> int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev); >>> int pci_cleanup_aer_error_status_regs(struct pci_dev *dev); >>> +void pci_save_aer_state(struct pci_dev *dev); >>> +void pci_restore_aer_state(struct pci_dev *dev); >>> #else >>> static inline int pci_enable_pcie_error_reporting(struct pci_dev *dev) >>> { >>> @@ -63,6 +65,8 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) >>> { >>> return -EINVAL; >>> } >>> +static inline void pci_save_aer_state(struct pci_dev *dev) {} >>> +static inline void pci_restore_aer_state(struct pci_dev *dev) {} >>> #endif >>> >>> void cper_print_aer(struct pci_dev *dev, int aer_severity, >> -- >> Sathyanarayanan Kuppuswamy >> Linux kernel developer > Intel Deutschland GmbH > Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Christin Eisenschmid, Gary Kershaw > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928 -- Sathyanarayanan Kuppuswamy Linux kernel developer ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-10 17:36 ` sathyanarayanan kuppuswamy @ 2019-07-24 18:45 ` Bjorn Helgaas 2019-07-25 16:08 ` Keith Busch 0 siblings, 1 reply; 10+ messages in thread From: Bjorn Helgaas @ 2019-07-24 18:45 UTC (permalink / raw) To: sathyanarayanan kuppuswamy Cc: Patel, Mayurkumar, linux-pci, mika.westerberg, 'Andy Shevchenko', Keith Busch, Oza Pawandeep [+cc Keith, Oza, who did quite a bit of recent AER work] Please see https://lkml.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com for a few incidental subject line/commit log hints. On Wed, Jul 10, 2019 at 10:36:16AM -0700, sathyanarayanan kuppuswamy wrote: > On 7/10/19 10:22 AM, Patel, Mayurkumar wrote: > > > On 7/9/19 1:00 AM, Patel, Mayurkumar wrote: > > > > After system suspend/resume cycle AER registers settings are > > > > lost. Not restoring Root Error Command Register bits if it were > > > > set, keeps AER interrupts disabled after system resume. > > > > Moreover, AER mask and severity registers are also required > > > > to be restored back to AER settings prior to system suspend. > > > > > > > > Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> > > > > --- > > > > drivers/pci/pci.c | 2 ++ > > > > drivers/pci/pcie/aer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/aer.h | 4 ++++ > > > > 3 files changed, 55 insertions(+) > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > index 8abc843..40d5507 100644 > > > > --- a/drivers/pci/pci.c > > > > +++ b/drivers/pci/pci.c > > > > @@ -1340,6 +1340,7 @@ int pci_save_state(struct pci_dev *dev) > > > > > > > > pci_save_ltr_state(dev); > > > > pci_save_dpc_state(dev); > > > > + pci_save_aer_state(dev); > > > > return pci_save_vc_state(dev); > > > > } > > > > EXPORT_SYMBOL(pci_save_state); > > > > @@ -1453,6 +1454,7 @@ void pci_restore_state(struct pci_dev *dev) > > > > pci_restore_dpc_state(dev); > > > > > > > > pci_cleanup_aer_error_status_regs(dev); > > > > + pci_restore_aer_state(dev); > > > > > > > > pci_restore_config_space(dev); > > > > > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > > > index b45bc47..1acc641 100644 > > > > --- a/drivers/pci/pcie/aer.c > > > > +++ b/drivers/pci/pcie/aer.c > > > > @@ -448,6 +448,54 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) > > > > return 0; > > > > } > > > > > > > > +void pci_save_aer_state(struct pci_dev *dev) > > > > +{ > > > > + int pos = 0; > > > > + struct pci_cap_saved_state *save_state; > > > > + u32 *cap; > > > > + > > > > + if (!pci_is_pcie(dev)) > > > > + return; > > > > + > > > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > > > > + if (!save_state) > > > > + return; > > > > + > > > > + pos = dev->aer_cap; > > > > + if (!pos) > > > > + return; > > > > + > > > > + cap = &save_state->cap.data[0]; > > > > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, cap++); > > > > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, cap++); > > > > + pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, cap++); > > > > + pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++); > > > I don't see AER driver modifying UNCOR_MASK/SEVER/COR_MASK register. If > > > all it has is default value then why do you want to preserve it ? > > > > > Thanks for reply. > > You are right about UNCOR_MASK/SEVER/COR_MASK mask AER driver > > leaves untouched, But IMHO users for example can use "setpci" to > > unmask specific errors and its severity based on their debugging > > requirement on Root port and/or Endpoint. So during resume, if > > PCIe endpoint fails due to any error which by default stays masked > > then it can't be catched and/or debugged. Moreover, Endpoint > > driver may also unmask during "driver probe" certain specific > > errors for endpoint, which needs to be restored while resume. > > Isn't these registers configuration usually done by firmware ? I > think user application rarely touch them. Also, IMO, > Caching/Restoring registers under the assumption that it might be > useful for user if they modified it is not a convincing argument. > But I will let Bjorn and others decide whether its alright to cache > these registers. I think the ideal user experience would be "suspend/resume has no effect on any configuration". That would argue for saving/restoring registers even if we think it's unlikely the user would change them. > > @Bjorn/Anybody else has any opinion to cache/restore > > UNCOR_MASK/SEVER/COR_MASK registers? Please help to comment. > > > > > Also, any reason for not preserving ECRC settings ? > > No specific reason. I can incorporte that with v2 of this patchset > > but I don’t have HW on which I can validate that. I think we should preserve ECRC settings as well for the same reason. > > > > +} > > > > + > > > > +void pci_restore_aer_state(struct pci_dev *dev) > > > > +{ > > > > + int pos = 0; > > > > + struct pci_cap_saved_state *save_state; > > > > + u32 *cap; > > > > + > > > > + if (!pci_is_pcie(dev)) > > > > + return; > > > > + > > > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > > > > + if (!save_state) > > > > + return; > > > > + > > > > + pos = dev->aer_cap; > > > > + if (!pos) > > > > + return; > > > > + > > > > + cap = &save_state->cap.data[0]; > > > > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, *cap++); > > > > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, *cap++); > > > > + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, *cap++); > > > > + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, *cap++); > > > > +} > > > > + > > > > void pci_aer_init(struct pci_dev *dev) > > > > { > > > > dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > > > > @@ -1396,6 +1444,7 @@ static int aer_probe(struct pcie_device *dev) > > > > return status; > > > > } > > > > > > > > + pci_add_ext_cap_save_buffer(port, PCI_EXT_CAP_ID_ERR, sizeof(u32) * 4); > > > > aer_enable_rootport(rpc); > > > > pci_info(port, "enabled with IRQ %d\n", dev->irq); > > > > return 0; > > > > diff --git a/include/linux/aer.h b/include/linux/aer.h > > > > index 514bffa..fa19e01 100644 > > > > --- a/include/linux/aer.h > > > > +++ b/include/linux/aer.h > > > > @@ -46,6 +46,8 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev); > > > > int pci_disable_pcie_error_reporting(struct pci_dev *dev); > > > > int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev); > > > > int pci_cleanup_aer_error_status_regs(struct pci_dev *dev); > > > > +void pci_save_aer_state(struct pci_dev *dev); > > > > +void pci_restore_aer_state(struct pci_dev *dev); > > > > #else > > > > static inline int pci_enable_pcie_error_reporting(struct pci_dev *dev) > > > > { > > > > @@ -63,6 +65,8 @@ static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) > > > > { > > > > return -EINVAL; > > > > } > > > > +static inline void pci_save_aer_state(struct pci_dev *dev) {} > > > > +static inline void pci_restore_aer_state(struct pci_dev *dev) {} > > > > #endif > > > > > > > > void cper_print_aer(struct pci_dev *dev, int aer_severity, > > > -- > > > Sathyanarayanan Kuppuswamy > > > Linux kernel developer > > Intel Deutschland GmbH > > Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany > > Tel: +49 89 99 8853-0, www.intel.de > > Managing Directors: Christin Eisenschmid, Gary Kershaw > > Chairperson of the Supervisory Board: Nicole Lau > > Registered Office: Munich > > Commercial Register: Amtsgericht Muenchen HRB 186928 > > -- > Sathyanarayanan Kuppuswamy > Linux kernel developer > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-24 18:45 ` Bjorn Helgaas @ 2019-07-25 16:08 ` Keith Busch 2019-07-26 9:00 ` Patel, Mayurkumar 0 siblings, 1 reply; 10+ messages in thread From: Keith Busch @ 2019-07-25 16:08 UTC (permalink / raw) To: Bjorn Helgaas Cc: sathyanarayanan kuppuswamy, Patel, Mayurkumar, linux-pci, mika.westerberg, 'Andy Shevchenko', Keith Busch, Oza Pawandeep On Wed, Jul 24, 2019 at 01:45:48PM -0500, Bjorn Helgaas wrote: > On Wed, Jul 10, 2019 at 10:36:16AM -0700, sathyanarayanan kuppuswamy wrote: > > On 7/10/19 10:22 AM, Patel, Mayurkumar wrote: > > > > On 7/9/19 1:00 AM, Patel, Mayurkumar wrote: > > > > > After system suspend/resume cycle AER registers settings are > > > > > lost. Not restoring Root Error Command Register bits if it were > > > > > set, keeps AER interrupts disabled after system resume. > > > > > Moreover, AER mask and severity registers are also required > > > > > to be restored back to AER settings prior to system suspend. > > > > > > > > > > Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> > > > > > --- > > > > > drivers/pci/pci.c | 2 ++ > > > > > drivers/pci/pcie/aer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > include/linux/aer.h | 4 ++++ > > > > > 3 files changed, 55 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > index 8abc843..40d5507 100644 > > > > > --- a/drivers/pci/pci.c > > > > > +++ b/drivers/pci/pci.c > > > > > @@ -1340,6 +1340,7 @@ int pci_save_state(struct pci_dev *dev) > > > > > > > > > > pci_save_ltr_state(dev); > > > > > pci_save_dpc_state(dev); > > > > > + pci_save_aer_state(dev); > > > > > return pci_save_vc_state(dev); > > > > > } > > > > > EXPORT_SYMBOL(pci_save_state); > > > > > @@ -1453,6 +1454,7 @@ void pci_restore_state(struct pci_dev *dev) > > > > > pci_restore_dpc_state(dev); > > > > > > > > > > pci_cleanup_aer_error_status_regs(dev); > > > > > + pci_restore_aer_state(dev); > > > > > > > > > > pci_restore_config_space(dev); > > > > > > > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > > > > index b45bc47..1acc641 100644 > > > > > --- a/drivers/pci/pcie/aer.c > > > > > +++ b/drivers/pci/pcie/aer.c > > > > > @@ -448,6 +448,54 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) > > > > > return 0; > > > > > } > > > > > > > > > > +void pci_save_aer_state(struct pci_dev *dev) > > > > > +{ > > > > > + int pos = 0; > > > > > + struct pci_cap_saved_state *save_state; > > > > > + u32 *cap; > > > > > + > > > > > + if (!pci_is_pcie(dev)) > > > > > + return; > > > > > + > > > > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > > > > > + if (!save_state) > > > > > + return; > > > > > + > > > > > + pos = dev->aer_cap; > > > > > + if (!pos) > > > > > + return; > > > > > + > > > > > + cap = &save_state->cap.data[0]; > > > > > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, cap++); > > > > > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, cap++); > > > > > + pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, cap++); > > > > > + pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++); > > > > > I don't see AER driver modifying UNCOR_MASK/SEVER/COR_MASK register. If > > > > all it has is default value then why do you want to preserve it ? > > > > > > > Thanks for reply. > > > You are right about UNCOR_MASK/SEVER/COR_MASK mask AER driver > > > leaves untouched, But IMHO users for example can use "setpci" to > > > unmask specific errors and its severity based on their debugging > > > requirement on Root port and/or Endpoint. So during resume, if > > > PCIe endpoint fails due to any error which by default stays masked > > > then it can't be catched and/or debugged. Moreover, Endpoint > > > driver may also unmask during "driver probe" certain specific > > > errors for endpoint, which needs to be restored while resume. > > > > Isn't these registers configuration usually done by firmware ? I > > think user application rarely touch them. Also, IMO, > > Caching/Restoring registers under the assumption that it might be > > useful for user if they modified it is not a convincing argument. > > But I will let Bjorn and others decide whether its alright to cache > > these registers. > > I think the ideal user experience would be "suspend/resume has no > effect on any configuration". That would argue for saving/restoring > registers even if we think it's unlikely the user would change them. The call to pci_save_state most likely occurs long before a user has an opportunity to alter these regsiters, though. Won't this just restore what was previously there, and not the state you changed it to? > > > @Bjorn/Anybody else has any opinion to cache/restore > > > UNCOR_MASK/SEVER/COR_MASK registers? Please help to comment. > > > > > > > Also, any reason for not preserving ECRC settings ? > > > No specific reason. I can incorporte that with v2 of this patchset > > > but I don’t have HW on which I can validate that. > > I think we should preserve ECRC settings as well for the same reason. > > > > > > +} > > > > > + > > > > > +void pci_restore_aer_state(struct pci_dev *dev) > > > > > +{ > > > > > + int pos = 0; > > > > > + struct pci_cap_saved_state *save_state; > > > > > + u32 *cap; > > > > > + > > > > > + if (!pci_is_pcie(dev)) > > > > > + return; > > > > > + > > > > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > > > > > + if (!save_state) > > > > > + return; > > > > > + > > > > > + pos = dev->aer_cap; > > > > > + if (!pos) > > > > > + return; > > > > > + > > > > > + cap = &save_state->cap.data[0]; > > > > > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, *cap++); > > > > > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, *cap++); > > > > > + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, *cap++); > > > > > + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, *cap++); > > > > > +} > > > > > + > > > > > void pci_aer_init(struct pci_dev *dev) > > > > > { > > > > > dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > > > > > @@ -1396,6 +1444,7 @@ static int aer_probe(struct pcie_device *dev) > > > > > return status; > > > > > } > > > > > > > > > > + pci_add_ext_cap_save_buffer(port, PCI_EXT_CAP_ID_ERR, sizeof(u32) * 4); > > > > > aer_enable_rootport(rpc); > > > > > pci_info(port, "enabled with IRQ %d\n", dev->irq); > > > > > return 0; You are allocating the capability save buffer in aer_probe(), so this save/restore applies only to root ports. But you mention above that you want to restore end devices, right? ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-25 16:08 ` Keith Busch @ 2019-07-26 9:00 ` Patel, Mayurkumar 2019-07-26 16:15 ` Keith Busch 0 siblings, 1 reply; 10+ messages in thread From: Patel, Mayurkumar @ 2019-07-26 9:00 UTC (permalink / raw) To: Keith Busch, Bjorn Helgaas Cc: sathyanarayanan kuppuswamy, linux-pci, mika.westerberg, 'Andy Shevchenko', Busch, Keith, Oza Pawandeep Thanks Bjorn and Keith for the response. > > On Wed, Jul 24, 2019 at 01:45:48PM -0500, Bjorn Helgaas wrote: > > On Wed, Jul 10, 2019 at 10:36:16AM -0700, sathyanarayanan kuppuswamy wrote: > > > On 7/10/19 10:22 AM, Patel, Mayurkumar wrote: > > > > > On 7/9/19 1:00 AM, Patel, Mayurkumar wrote: > > > > > > After system suspend/resume cycle AER registers settings are > > > > > > lost. Not restoring Root Error Command Register bits if it were > > > > > > set, keeps AER interrupts disabled after system resume. > > > > > > Moreover, AER mask and severity registers are also required > > > > > > to be restored back to AER settings prior to system suspend. > > > > > > > > > > > > Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> > > > > > > --- > > > > > > drivers/pci/pci.c | 2 ++ > > > > > > drivers/pci/pcie/aer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > include/linux/aer.h | 4 ++++ > > > > > > 3 files changed, 55 insertions(+) > > > > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > > index 8abc843..40d5507 100644 > > > > > > --- a/drivers/pci/pci.c > > > > > > +++ b/drivers/pci/pci.c > > > > > > @@ -1340,6 +1340,7 @@ int pci_save_state(struct pci_dev *dev) > > > > > > > > > > > > pci_save_ltr_state(dev); > > > > > > pci_save_dpc_state(dev); > > > > > > + pci_save_aer_state(dev); > > > > > > return pci_save_vc_state(dev); > > > > > > } > > > > > > EXPORT_SYMBOL(pci_save_state); > > > > > > @@ -1453,6 +1454,7 @@ void pci_restore_state(struct pci_dev *dev) > > > > > > pci_restore_dpc_state(dev); > > > > > > > > > > > > pci_cleanup_aer_error_status_regs(dev); > > > > > > + pci_restore_aer_state(dev); > > > > > > > > > > > > pci_restore_config_space(dev); > > > > > > > > > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > > > > > index b45bc47..1acc641 100644 > > > > > > --- a/drivers/pci/pcie/aer.c > > > > > > +++ b/drivers/pci/pcie/aer.c > > > > > > @@ -448,6 +448,54 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +void pci_save_aer_state(struct pci_dev *dev) > > > > > > +{ > > > > > > + int pos = 0; > > > > > > + struct pci_cap_saved_state *save_state; > > > > > > + u32 *cap; > > > > > > + > > > > > > + if (!pci_is_pcie(dev)) > > > > > > + return; > > > > > > + > > > > > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > > > > > > + if (!save_state) > > > > > > + return; > > > > > > + > > > > > > + pos = dev->aer_cap; > > > > > > + if (!pos) > > > > > > + return; > > > > > > + > > > > > > + cap = &save_state->cap.data[0]; > > > > > > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, cap++); > > > > > > + pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, cap++); > > > > > > + pci_read_config_dword(dev, pos + PCI_ERR_COR_MASK, cap++); > > > > > > + pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++); > > > > > > > I don't see AER driver modifying UNCOR_MASK/SEVER/COR_MASK register. If > > > > > all it has is default value then why do you want to preserve it ? > > > > > > > > > Thanks for reply. > > > > You are right about UNCOR_MASK/SEVER/COR_MASK mask AER driver > > > > leaves untouched, But IMHO users for example can use "setpci" to > > > > unmask specific errors and its severity based on their debugging > > > > requirement on Root port and/or Endpoint. So during resume, if > > > > PCIe endpoint fails due to any error which by default stays masked > > > > then it can't be catched and/or debugged. Moreover, Endpoint > > > > driver may also unmask during "driver probe" certain specific > > > > errors for endpoint, which needs to be restored while resume. > > > > > > Isn't these registers configuration usually done by firmware ? I > > > think user application rarely touch them. Also, IMO, > > > Caching/Restoring registers under the assumption that it might be > > > useful for user if they modified it is not a convincing argument. > > > But I will let Bjorn and others decide whether its alright to cache > > > these registers. > > > > I think the ideal user experience would be "suspend/resume has no > > effect on any configuration". That would argue for saving/restoring > > registers even if we think it's unlikely the user would change them. > > The call to pci_save_state most likely occurs long before a user has an > opportunity to alter these regsiters, though. Won't this just restore > what was previously there, and not the state you changed it to? There were two things (not sure to call them issues), 1. PCI_ERR_ROOT_COMMAND resets to 0 during S3 entry/exit, which disables AER interrupt trigger if AER happens on Endpoint after resume. Also specified in spec. NCB-PCI_Express_Base_4.0r1.0_September-27-2017-c.pdf in chapter 7.8.4.9 Root Error Command Register (Offset 2Ch) - in bitfields descriptions. i.e. Correctable Error Reporting Enable – When Set, this bit enables the generation of an interrupt when a correctable error is reported by any of the Functions in the Hierarchy Domain associated with this Root Port. 2. Root port resets to its default configuration of UNCOR_MASK/SEVER/COR_MASK register bits after system resume. This influences user configurations, how errors shall be treated if AER happens on root port itself due to Device (for example Endpoint not answering which results in completion timeouts on root ports). Following is one example scenario which can handled with this patch. - user configures AER registers using setpci certain masks and severity based on debug requirements. This can be applied on Root port of EP. - triggers system test which includes S3 entry/exit cycles. - system enters s3 -> AER registers settings are saved which has been configured by users. - system exits s3 -> AER registers settings are restored which has been configured by users. > > > > > @Bjorn/Anybody else has any opinion to cache/restore > > > > UNCOR_MASK/SEVER/COR_MASK registers? Please help to comment. > > > > > > > > > Also, any reason for not preserving ECRC settings ? > > > > No specific reason. I can incorporte that with v2 of this patchset > > > > but I don’t have HW on which I can validate that. > > > > I think we should preserve ECRC settings as well for the same reason. > > > > > > > > +} > > > > > > + > > > > > > +void pci_restore_aer_state(struct pci_dev *dev) > > > > > > +{ > > > > > > + int pos = 0; > > > > > > + struct pci_cap_saved_state *save_state; > > > > > > + u32 *cap; > > > > > > + > > > > > > + if (!pci_is_pcie(dev)) > > > > > > + return; > > > > > > + > > > > > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR); > > > > > > + if (!save_state) > > > > > > + return; > > > > > > + > > > > > > + pos = dev->aer_cap; > > > > > > + if (!pos) > > > > > > + return; > > > > > > + > > > > > > + cap = &save_state->cap.data[0]; > > > > > > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, *cap++); > > > > > > + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, *cap++); > > > > > > + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, *cap++); > > > > > > + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, *cap++); > > > > > > +} > > > > > > + > > > > > > void pci_aer_init(struct pci_dev *dev) > > > > > > { > > > > > > dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > > > > > > @@ -1396,6 +1444,7 @@ static int aer_probe(struct pcie_device *dev) > > > > > > return status; > > > > > > } > > > > > > > > > > > > + pci_add_ext_cap_save_buffer(port, PCI_EXT_CAP_ID_ERR, sizeof(u32) * 4); > > > > > > aer_enable_rootport(rpc); > > > > > > pci_info(port, "enabled with IRQ %d\n", dev->irq); > > > > > > return 0; > > You are allocating the capability save buffer in aer_probe(), so this > save/restore applies only to root ports. But you mention above that you > want to restore end devices, right? That’s correct. I agree that my commit message was not so explicit. But Since I included PCI_ERR_ROOT_COMMAND register for save/restore which is specific to Root ports only & I thought endpoint drivers can handle to save/restore (UNCOR_MASK/SEVER/COR_MASK) themselves with its suspend/resume functions. However I am fine to move pci_add_ext_cap_save_buffer() to some other function so that it also save/restores UNCOR_MASK/SEVER/COR_MASK registers for endpoints and if it's not useful to save/restore MASK & SEVER registers then also fine to remove them and just restore PCI_ERR_ROOT_COMMAND & ECRC settings. Please let me know. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Gary Kershaw Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-26 9:00 ` Patel, Mayurkumar @ 2019-07-26 16:15 ` Keith Busch 2019-07-30 13:57 ` Bjorn Helgaas 0 siblings, 1 reply; 10+ messages in thread From: Keith Busch @ 2019-07-26 16:15 UTC (permalink / raw) To: Patel, Mayurkumar Cc: Bjorn Helgaas, sathyanarayanan kuppuswamy, linux-pci, mika.westerberg, 'Andy Shevchenko', Busch, Keith, Oza Pawandeep On Fri, Jul 26, 2019 at 02:00:05AM -0700, Patel, Mayurkumar wrote: > > The call to pci_save_state most likely occurs long before a user has an > > opportunity to alter these regsiters, though. Won't this just restore > > what was previously there, and not the state you changed it to? > > > There were two things (not sure to call them issues), > 1. PCI_ERR_ROOT_COMMAND resets to 0 during S3 entry/exit, which disables AER interrupt trigger > if AER happens on Endpoint after resume. > > Also specified in spec. NCB-PCI_Express_Base_4.0r1.0_September-27-2017-c.pdf in > chapter 7.8.4.9 Root Error Command Register (Offset 2Ch) - in bitfields descriptions. > i.e. Correctable Error Reporting Enable – When Set, this bit enables the generation of an interrupt when > a correctable error is reported by any of the Functions in the Hierarchy Domain associated with this Root Port. > > 2. Root port resets to its default configuration of UNCOR_MASK/SEVER/COR_MASK register bits after system resume. > This influences user configurations, how errors shall be treated if AER happens on root port itself due to Device (for example > Endpoint not answering which results in completion timeouts on root ports). > > Following is one example scenario which can handled with this patch. > - user configures AER registers using setpci certain masks and severity based on debug requirements. This can be applied on Root port of EP. > - triggers system test which includes S3 entry/exit cycles. > - system enters s3 -> AER registers settings are saved which has been configured by users. > - system exits s3 -> AER registers settings are restored which has been configured by users. Right, I was just more curious *where* the aer state was being saved during suspend since pci port driver only saves state during probe. This patch must be relying on the generic pci-driver's power management. I think that works, but I just didn't realize initially that we're relying on that path. > > You are allocating the capability save buffer in aer_probe(), so this > > save/restore applies only to root ports. But you mention above that you > > want to restore end devices, right? > > That’s correct. I agree that my commit message was not so explicit. > But Since I included PCI_ERR_ROOT_COMMAND register for save/restore which is specific to Root ports only & I thought > endpoint drivers can handle to save/restore (UNCOR_MASK/SEVER/COR_MASK) themselves with its suspend/resume functions. > > However I am fine to move pci_add_ext_cap_save_buffer() to some other function so > that it also save/restores UNCOR_MASK/SEVER/COR_MASK registers for endpoints and > if it's not useful to save/restore MASK & SEVER registers then also fine to remove them > and just restore PCI_ERR_ROOT_COMMAND & ECRC settings. Please let me know. If users are changing default settings that you want to preserve, that reason should apply to bridges and endpoints too, so I think you ought to allocate the save buffer for this capability for all devices that support it in in pci_aer_init(). Just make sure to check the device type in save/restore so you're not accessing root port specific registers. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-26 16:15 ` Keith Busch @ 2019-07-30 13:57 ` Bjorn Helgaas 2019-08-01 13:09 ` Patel, Mayurkumar 0 siblings, 1 reply; 10+ messages in thread From: Bjorn Helgaas @ 2019-07-30 13:57 UTC (permalink / raw) To: Keith Busch Cc: Patel, Mayurkumar, sathyanarayanan kuppuswamy, linux-pci, mika.westerberg, 'Andy Shevchenko', Busch, Keith, Oza Pawandeep On Fri, Jul 26, 2019 at 10:15:24AM -0600, Keith Busch wrote: > On Fri, Jul 26, 2019 at 02:00:05AM -0700, Patel, Mayurkumar wrote: > > > The call to pci_save_state most likely occurs long before a user has an > > > opportunity to alter these regsiters, though. Won't this just restore > > > what was previously there, and not the state you changed it to? > > > > There were two things (not sure to call them issues), > > 1. PCI_ERR_ROOT_COMMAND resets to 0 during S3 entry/exit, which disables AER interrupt trigger > > if AER happens on Endpoint after resume. > > > > Also specified in spec. NCB-PCI_Express_Base_4.0r1.0_September-27-2017-c.pdf in > > chapter 7.8.4.9 Root Error Command Register (Offset 2Ch) - in bitfields descriptions. > > i.e. Correctable Error Reporting Enable – When Set, this bit enables the generation of an interrupt when > > a correctable error is reported by any of the Functions in the Hierarchy Domain associated with this Root Port. > > > > 2. Root port resets to its default configuration of UNCOR_MASK/SEVER/COR_MASK register bits after system resume. > > This influences user configurations, how errors shall be treated if AER happens on root port itself due to Device (for example > > Endpoint not answering which results in completion timeouts on root ports). > > > > Following is one example scenario which can handled with this patch. > > - user configures AER registers using setpci certain masks and severity based on debug requirements. This can be applied on Root port of EP. > > - triggers system test which includes S3 entry/exit cycles. > > - system enters s3 -> AER registers settings are saved which has been configured by users. > > - system exits s3 -> AER registers settings are restored which has been configured by users. > > Right, I was just more curious *where* the aer state was being saved > during suspend since pci port driver only saves state during probe. This > patch must be relying on the generic pci-driver's power management. I > think that works, but I just didn't realize initially that we're relying > on that path. I agree that we should save the AER state at suspend-time, not just at probe-time. I *think* this should happen already via the pci_save_state() calls in pci_pm_suspend_noirq(), etc. > > > You are allocating the capability save buffer in aer_probe(), so this > > > save/restore applies only to root ports. But you mention above that you > > > want to restore end devices, right? > > > > That’s correct. I agree that my commit message was not so explicit. > > But Since I included PCI_ERR_ROOT_COMMAND register for save/restore which is specific to Root ports only & I thought > > endpoint drivers can handle to save/restore (UNCOR_MASK/SEVER/COR_MASK) themselves with its suspend/resume functions. > > > > However I am fine to move pci_add_ext_cap_save_buffer() to some other function so > > that it also save/restores UNCOR_MASK/SEVER/COR_MASK registers for endpoints and > > if it's not useful to save/restore MASK & SEVER registers then also fine to remove them > > and just restore PCI_ERR_ROOT_COMMAND & ECRC settings. Please let me know. > > If users are changing default settings that you want to preserve, that > reason should apply to bridges and endpoints too, so I think you ought > to allocate the save buffer for this capability for all devices that > support it in in pci_aer_init(). Just make sure to check the device type > in save/restore so you're not accessing root port specific registers. I agree, I think we need to save/restore AER settings for all devices, not just bridges. Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] PCI/AER: save/restore AER registers during suspend/resume 2019-07-30 13:57 ` Bjorn Helgaas @ 2019-08-01 13:09 ` Patel, Mayurkumar 0 siblings, 0 replies; 10+ messages in thread From: Patel, Mayurkumar @ 2019-08-01 13:09 UTC (permalink / raw) To: Bjorn Helgaas, Keith Busch Cc: sathyanarayanan kuppuswamy, linux-pci, mika.westerberg, 'Andy Shevchenko', Busch, Keith, Oza Pawandeep > > On Fri, Jul 26, 2019 at 10:15:24AM -0600, Keith Busch wrote: > > On Fri, Jul 26, 2019 at 02:00:05AM -0700, Patel, Mayurkumar wrote: > > > > The call to pci_save_state most likely occurs long before a user has an > > > > opportunity to alter these regsiters, though. Won't this just restore > > > > what was previously there, and not the state you changed it to? > > > > > > There were two things (not sure to call them issues), > > > 1. PCI_ERR_ROOT_COMMAND resets to 0 during S3 entry/exit, which disables AER interrupt trigger > > > if AER happens on Endpoint after resume. > > > > > > Also specified in spec. NCB-PCI_Express_Base_4.0r1.0_September-27-2017-c.pdf in > > > chapter 7.8.4.9 Root Error Command Register (Offset 2Ch) - in bitfields descriptions. > > > i.e. Correctable Error Reporting Enable – When Set, this bit enables the generation of an interrupt when > > > a correctable error is reported by any of the Functions in the Hierarchy Domain associated with this Root Port. > > > > > > 2. Root port resets to its default configuration of UNCOR_MASK/SEVER/COR_MASK register bits after system resume. > > > This influences user configurations, how errors shall be treated if AER happens on root port itself due to Device (for example > > > Endpoint not answering which results in completion timeouts on root ports). > > > > > > Following is one example scenario which can handled with this patch. > > > - user configures AER registers using setpci certain masks and severity based on debug requirements. This can be applied on Root port of > EP. > > > - triggers system test which includes S3 entry/exit cycles. > > > - system enters s3 -> AER registers settings are saved which has been configured by users. > > > - system exits s3 -> AER registers settings are restored which has been configured by users. > > > > Right, I was just more curious *where* the aer state was being saved > > during suspend since pci port driver only saves state during probe. This > > patch must be relying on the generic pci-driver's power management. I > > think that works, but I just didn't realize initially that we're relying > > on that path. > > I agree that we should save the AER state at suspend-time, not just at > probe-time. I *think* this should happen already via the > pci_save_state() calls in pci_pm_suspend_noirq(), etc. > > > > > You are allocating the capability save buffer in aer_probe(), so this > > > > save/restore applies only to root ports. But you mention above that you > > > > want to restore end devices, right? > > > > > > That’s correct. I agree that my commit message was not so explicit. > > > But Since I included PCI_ERR_ROOT_COMMAND register for save/restore which is specific to Root ports only & I thought > > > endpoint drivers can handle to save/restore (UNCOR_MASK/SEVER/COR_MASK) themselves with its suspend/resume functions. > > > > > > However I am fine to move pci_add_ext_cap_save_buffer() to some other function so > > > that it also save/restores UNCOR_MASK/SEVER/COR_MASK registers for endpoints and > > > if it's not useful to save/restore MASK & SEVER registers then also fine to remove them > > > and just restore PCI_ERR_ROOT_COMMAND & ECRC settings. Please let me know. > > > > If users are changing default settings that you want to preserve, that > > reason should apply to bridges and endpoints too, so I think you ought > > to allocate the save buffer for this capability for all devices that > > support it in in pci_aer_init(). Just make sure to check the device type > > in save/restore so you're not accessing root port specific registers. > > I agree, I think we need to save/restore AER settings for all devices, > not just bridges. > Thanks for replies. I will update my patch accordingly and post a revised one. > Bjorn Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Gary Kershaw Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-08-01 13:09 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-09 8:00 [PATCH] PCI/AER: save/restore AER registers during suspend/resume Patel, Mayurkumar 2019-07-09 18:31 ` sathyanarayanan kuppuswamy 2019-07-10 17:22 ` Patel, Mayurkumar 2019-07-10 17:36 ` sathyanarayanan kuppuswamy 2019-07-24 18:45 ` Bjorn Helgaas 2019-07-25 16:08 ` Keith Busch 2019-07-26 9:00 ` Patel, Mayurkumar 2019-07-26 16:15 ` Keith Busch 2019-07-30 13:57 ` Bjorn Helgaas 2019-08-01 13:09 ` Patel, Mayurkumar
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.