linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] PCI/AER: Save and restore AER config state
@ 2019-08-07 16:03 Patel, Mayurkumar
  2019-08-07 18:01 ` sathyanarayanan kuppuswamy
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Patel, Mayurkumar @ 2019-08-07 16:03 UTC (permalink / raw)
  To: linux-pci
  Cc: Busch, Keith, 'Andy Shevchenko', Kuppuswamy Sathyanarayanan

This patch provides AER config save and restore capabilities. After system
resume AER config registers settings are lost. Not restoring AER root error
command register bits on root port if they were set, disables generation
of an AER interrupt reported by function as described in PCIe spec r4.0,
sec 7.8.4.9. Moreover, AER config mask, severity and ECRC registers are
also required to maintain same state prior to system suspend to maintain
AER interrupts behavior.

Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pci.c      |  2 ++
 drivers/pci/pcie/aer.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/aer.h    |  4 +++
 3 files changed, 76 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..fb067dc 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -448,6 +448,64 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	return 0;
 }
 
+static inline bool pcie_aer_cap_has_root_command(struct pci_dev *dev)
+{
+	return pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+		pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC;
+}
+
+void pci_save_aer_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u32 *cap;
+	int pos;
+
+	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_CAP, cap++);
+	if (pcie_aer_cap_has_root_command(dev))
+		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++);
+}
+
+void pci_restore_aer_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u32 *cap;
+	int pos;
+
+	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_CAP, *cap++);
+	if (pcie_aer_cap_has_root_command(dev))
+		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);
@@ -455,6 +513,18 @@ void pci_aer_init(struct pci_dev *dev)
 	if (dev->aer_cap)
 		dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
 
+	/*
+	 * Since PCI_ERR_ROOT_COMMAND is only valid for root port and root
+	 * complex event collector, as per PCIe 4.0 section 7.8.4, interpret
+	 * the device/port type to determine the availability of additional
+	 * root port and root complex event collector register.
+	 */
+	if (pcie_aer_cap_has_root_command(dev))
+		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
+					sizeof(u32) * 5);
+	else
+		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
+					sizeof(u32) * 4);
 	pci_cleanup_aer_error_status_regs(dev);
 }
 
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


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] 5+ messages in thread
* Re: [PATCH v2] PCI/AER: Save and restore AER config state
  2019-08-07 16:03 [PATCH v2] PCI/AER: Save and restore AER config state Patel, Mayurkumar
@ 2019-08-07 18:01 ` sathyanarayanan kuppuswamy
  2019-08-08 10:01 ` 'Andy Shevchenko'
  2019-10-04 20:36 ` Bjorn Helgaas
  2 siblings, 0 replies; 5+ messages in thread
From: sathyanarayanan kuppuswamy @ 2019-08-07 18:01 UTC (permalink / raw)
  To: Patel, Mayurkumar, linux-pci; +Cc: Busch, Keith, 'Andy Shevchenko'

Hi,

On 8/7/19 9:03 AM, Patel, Mayurkumar wrote:
> This patch provides AER config save and restore capabilities. After system
> resume AER config registers settings are lost. Not restoring AER root error
> command register bits on root port if they were set, disables generation
> of an AER interrupt reported by function as described in PCIe spec r4.0,
> sec 7.8.4.9. Moreover, AER config mask, severity and ECRC registers are
> also required to maintain same state prior to system suspend to maintain
> AER interrupts behavior.
Looks good to me
>
> Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>   drivers/pci/pci.c      |  2 ++
>   drivers/pci/pcie/aer.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/aer.h    |  4 +++
>   3 files changed, 76 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..fb067dc 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -448,6 +448,64 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>   	return 0;
>   }
>   
> +static inline bool pcie_aer_cap_has_root_command(struct pci_dev *dev)
> +{
> +	return pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +		pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC;
> +}
> +
> +void pci_save_aer_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *cap;
> +	int pos;
> +
> +	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_CAP, cap++);
> +	if (pcie_aer_cap_has_root_command(dev))
> +		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++);
> +}
> +
> +void pci_restore_aer_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *cap;
> +	int pos;
> +
> +	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_CAP, *cap++);
> +	if (pcie_aer_cap_has_root_command(dev))
> +		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);
> @@ -455,6 +513,18 @@ void pci_aer_init(struct pci_dev *dev)
>   	if (dev->aer_cap)
>   		dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
>   
> +	/*
> +	 * Since PCI_ERR_ROOT_COMMAND is only valid for root port and root
> +	 * complex event collector, as per PCIe 4.0 section 7.8.4, interpret
> +	 * the device/port type to determine the availability of additional
> +	 * root port and root complex event collector register.
> +	 */
> +	if (pcie_aer_cap_has_root_command(dev))
> +		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
> +					sizeof(u32) * 5);
> +	else
> +		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
> +					sizeof(u32) * 4);
>   	pci_cleanup_aer_error_status_regs(dev);
>   }
>   
> 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] 5+ messages in thread
* Re: [PATCH v2] PCI/AER: Save and restore AER config state
  2019-08-07 16:03 [PATCH v2] PCI/AER: Save and restore AER config state Patel, Mayurkumar
  2019-08-07 18:01 ` sathyanarayanan kuppuswamy
@ 2019-08-08 10:01 ` 'Andy Shevchenko'
  2019-09-09  8:52   ` Patel, Mayurkumar
  2019-10-04 20:36 ` Bjorn Helgaas
  2 siblings, 1 reply; 5+ messages in thread
From: 'Andy Shevchenko' @ 2019-08-08 10:01 UTC (permalink / raw)
  To: Patel, Mayurkumar; +Cc: linux-pci, Busch, Keith, Kuppuswamy Sathyanarayanan

On Wed, Aug 07, 2019 at 04:03:52PM +0000, Patel, Mayurkumar wrote:
> This patch provides AER config save and restore capabilities. After system
> resume AER config registers settings are lost. Not restoring AER root error
> command register bits on root port if they were set, disables generation
> of an AER interrupt reported by function as described in PCIe spec r4.0,
> sec 7.8.4.9. Moreover, AER config mask, severity and ECRC registers are
> also required to maintain same state prior to system suspend to maintain
> AER interrupts behavior.
> 

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/pci.c      |  2 ++
>  drivers/pci/pcie/aer.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/aer.h    |  4 +++
>  3 files changed, 76 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..fb067dc 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -448,6 +448,64 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>  	return 0;
>  }
>  
> +static inline bool pcie_aer_cap_has_root_command(struct pci_dev *dev)
> +{
> +	return pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +		pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC;
> +}
> +
> +void pci_save_aer_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *cap;
> +	int pos;
> +
> +	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_CAP, cap++);
> +	if (pcie_aer_cap_has_root_command(dev))
> +		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++);
> +}
> +
> +void pci_restore_aer_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *cap;
> +	int pos;
> +
> +	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_CAP, *cap++);
> +	if (pcie_aer_cap_has_root_command(dev))
> +		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);
> @@ -455,6 +513,18 @@ void pci_aer_init(struct pci_dev *dev)
>  	if (dev->aer_cap)
>  		dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
>  
> +	/*
> +	 * Since PCI_ERR_ROOT_COMMAND is only valid for root port and root
> +	 * complex event collector, as per PCIe 4.0 section 7.8.4, interpret
> +	 * the device/port type to determine the availability of additional
> +	 * root port and root complex event collector register.
> +	 */
> +	if (pcie_aer_cap_has_root_command(dev))
> +		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
> +					sizeof(u32) * 5);
> +	else
> +		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
> +					sizeof(u32) * 4);
>  	pci_cleanup_aer_error_status_regs(dev);
>  }
>  
> 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
> 
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [PATCH v2] PCI/AER: Save and restore AER config state
  2019-08-08 10:01 ` 'Andy Shevchenko'
@ 2019-09-09  8:52   ` Patel, Mayurkumar
  0 siblings, 0 replies; 5+ messages in thread
From: Patel, Mayurkumar @ 2019-09-09  8:52 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas
  Cc: Busch, Keith, Kuppuswamy Sathyanarayanan, 'Andy Shevchenko'

Hi Bjorn
May you please help to understand if the patch requires any further improvements?

Thanks
Mayur

> 
> On Wed, Aug 07, 2019 at 04:03:52PM +0000, Patel, Mayurkumar wrote:
> > This patch provides AER config save and restore capabilities. After system
> > resume AER config registers settings are lost. Not restoring AER root error
> > command register bits on root port if they were set, disables generation
> > of an AER interrupt reported by function as described in PCIe spec r4.0,
> > sec 7.8.4.9. Moreover, AER config mask, severity and ECRC registers are
> > also required to maintain same state prior to system suspend to maintain
> > AER interrupts behavior.
> >
> 
> FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> 
> > Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > ---
> >  drivers/pci/pci.c      |  2 ++
> >  drivers/pci/pcie/aer.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/aer.h    |  4 +++
> >  3 files changed, 76 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..fb067dc 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -448,6 +448,64 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> >  	return 0;
> >  }
> >
> > +static inline bool pcie_aer_cap_has_root_command(struct pci_dev *dev)
> > +{
> > +	return pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > +		pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC;
> > +}
> > +
> > +void pci_save_aer_state(struct pci_dev *dev)
> > +{
> > +	struct pci_cap_saved_state *save_state;
> > +	u32 *cap;
> > +	int pos;
> > +
> > +	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_CAP, cap++);
> > +	if (pcie_aer_cap_has_root_command(dev))
> > +		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++);
> > +}
> > +
> > +void pci_restore_aer_state(struct pci_dev *dev)
> > +{
> > +	struct pci_cap_saved_state *save_state;
> > +	u32 *cap;
> > +	int pos;
> > +
> > +	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_CAP, *cap++);
> > +	if (pcie_aer_cap_has_root_command(dev))
> > +		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);
> > @@ -455,6 +513,18 @@ void pci_aer_init(struct pci_dev *dev)
> >  	if (dev->aer_cap)
> >  		dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
> >
> > +	/*
> > +	 * Since PCI_ERR_ROOT_COMMAND is only valid for root port and root
> > +	 * complex event collector, as per PCIe 4.0 section 7.8.4, interpret
> > +	 * the device/port type to determine the availability of additional
> > +	 * root port and root complex event collector register.
> > +	 */
> > +	if (pcie_aer_cap_has_root_command(dev))
> > +		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
> > +					sizeof(u32) * 5);
> > +	else
> > +		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
> > +					sizeof(u32) * 4);
> >  	pci_cleanup_aer_error_status_regs(dev);
> >  }
> >
> > 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
> >
> >
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

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] 5+ messages in thread
* Re: [PATCH v2] PCI/AER: Save and restore AER config state
  2019-08-07 16:03 [PATCH v2] PCI/AER: Save and restore AER config state Patel, Mayurkumar
  2019-08-07 18:01 ` sathyanarayanan kuppuswamy
  2019-08-08 10:01 ` 'Andy Shevchenko'
@ 2019-10-04 20:36 ` Bjorn Helgaas
  2 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2019-10-04 20:36 UTC (permalink / raw)
  To: Patel, Mayurkumar
  Cc: linux-pci, Busch, Keith, 'Andy Shevchenko',
	Kuppuswamy Sathyanarayanan

On Wed, Aug 07, 2019 at 04:03:52PM +0000, Patel, Mayurkumar wrote:
> This patch provides AER config save and restore capabilities. After system
> resume AER config registers settings are lost. Not restoring AER root error
> command register bits on root port if they were set, disables generation
> of an AER interrupt reported by function as described in PCIe spec r4.0,
> sec 7.8.4.9. Moreover, AER config mask, severity and ECRC registers are
> also required to maintain same state prior to system suspend to maintain
> AER interrupts behavior.

Looks good, trivial comments below.

> Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/pci.c      |  2 ++
>  drivers/pci/pcie/aer.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/aer.h    |  4 +++
>  3 files changed, 76 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..fb067dc 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -448,6 +448,64 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>  	return 0;
>  }
>  
> +static inline bool pcie_aer_cap_has_root_command(struct pci_dev *dev)
> +{
> +	return pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +		pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC;
> +}

Can you use pcie_cap_has_rtctl() here instead of implementing it
again?  You'll have to make it non-static, similar to what we did for
pcie_cap_has_lnkctl() in 7a1562d4f2d0 ("PCI: Apply _HPX Link Control
settings to all devices with a link").

> +void pci_save_aer_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *cap;
> +	int pos;
> +
> +	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;

I think it would make more sense to reorder the above like this:

  pos = dev->aer_cap;
  if (!pos)
    return;

  save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ERR);
  ...

and you don't need the pci_is_pcie() test since only PCIe devices
implement the AER capability.  Same applies below, of course.

> +	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_CAP, cap++);
> +	if (pcie_aer_cap_has_root_command(dev))
> +		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, cap++);
> +}
> +
> +void pci_restore_aer_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *cap;
> +	int pos;
> +
> +	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_CAP, *cap++);
> +	if (pcie_aer_cap_has_root_command(dev))
> +		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);
> @@ -455,6 +513,18 @@ void pci_aer_init(struct pci_dev *dev)
>  	if (dev->aer_cap)
>  		dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
>  
> +	/*
> +	 * Since PCI_ERR_ROOT_COMMAND is only valid for root port and root
> +	 * complex event collector, as per PCIe 4.0 section 7.8.4, interpret
> +	 * the device/port type to determine the availability of additional
> +	 * root port and root complex event collector register.
> +	 */
> +	if (pcie_aer_cap_has_root_command(dev))
> +		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
> +					sizeof(u32) * 5);
> +	else
> +		pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR,
> +					sizeof(u32) * 4);
>  	pci_cleanup_aer_error_status_regs(dev);
>  }
>  
> 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
> 
> 
> 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] 5+ messages in thread
end of thread, other threads:[~2019-10-04 20:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-07 16:03 [PATCH v2] PCI/AER: Save and restore AER config state Patel, Mayurkumar
2019-08-07 18:01 ` sathyanarayanan kuppuswamy
2019-08-08 10:01 ` 'Andy Shevchenko'
2019-09-09  8:52   ` Patel, Mayurkumar
2019-10-04 20:36 ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).