subject:"\"\\\\\\\[PATCH v2 2\\\\\\\/2\\\\\\\] PCI\\\\\\\/DPC\\\\\\\: Add Error Disconnect Recover \\\\\\\(EDR\\\\\\\) support\""

Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support

2019-04-10 Thread Bjorn Helgaas

On Wed, Apr 10, 2019 at 03:12:05PM -0700, sathyanarayanan kuppuswamy wrote:
> On 4/10/19 11:41 AM, Bjorn Helgaas wrote:
> > On Tue, Mar 19, 2019 at 01:47:29PM -0700, 
> > sathyanarayanan.kuppusw...@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan 
> > > 
> > > 
> > > As per PCI firmware specification v3.2 ECN
> > > (https://members.pcisig.com/wg/PCI-SIG/document/12614), when firmware
> > > owns Downstream Port Containment (DPC), its expected to use the "Error
> > > Disconnect Recover" (EDR) notification to alert OSPM of a DPC event and
> > > if OS supports EDR, its expected to handle the software state invalidation
> > > and port recovery in OS and let firmware know the recovery status via _OST
> > > ACPI call.
> > > 
> > > Since EDR is a hybrid service, DPC service shall be enabled in OS even
> > > if AER is not natively enabled in OS.

> > > + boolnative_dpc;
> > This is going to be way too confusing with a "native_dpc" in both the
> > struct pci_host_bridge and the struct dpc_dev.
> Any suggestions? what about native_mode ?

"native_mode" is different but doesn't contain any additional
information.  I haven't worked out what this new symbol actually does;
if it's related to EDR, maybe that could be incorporated somehow?

> > > + /* Register ACPI notifier for EDR event */
> > "Register handler for System events, one of which is the EDR event"?
> In our case, we only handle EDR event. So I just explicitly mentioned it.

Right.  As a courtesy to the reader, I think it's worth making
the comment match the code, i.e., you can't pass an event type to
acpi_install_notify_handler(), so obviously the handler will be called
for *all* System events.  By saying "one of which is the EDR event",
you give the reader a hint that the handler will have to filter out
the others.

The reason I noticed this is because I read "Register notifier for EDR
event" and wondered to myself "Hmmm, how does this work?  I don't see
anything passed to acpi_install_notify_handler() that would identify
an EDR event."

> > > + /*
> > > +  * If EDR support is enabled in OS, then even if AER is not handled in
> > > +  * OS, DPC service can be enabled.
> > > +  */
> > >   if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> > > - pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
> > > - (pcie_ports_native || host->native_dpc))
> > > + ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
> > > + (pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
> > > + (pcie_ports_native || host->native_dpc
> > Holy cow, I think I'll have to schedule an hour sometime to figure
> > out what's going on here :)
> Please check the previous patch in this series for details related to
> host->native_dpc.

Yeah, I'm just saying that conditional is starting to look pretty
gnarly.  Possibly there's nothing to do about it.

Bjorn

Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support

2019-04-10 Thread sathyanarayanan kuppuswamy


Hi ,

On 4/10/19 11:41 AM, Bjorn Helgaas wrote:

On Tue, Mar 19, 2019 at 01:47:29PM -0700, 
sathyanarayanan.kuppusw...@linux.intel.com wrote:

From: Kuppuswamy Sathyanarayanan 

As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), when firmware
owns Downstream Port Containment (DPC), its expected to use the "Error
Disconnect Recover" (EDR) notification to alert OSPM of a DPC event and
if OS supports EDR, its expected to handle the software state invalidation
and port recovery in OS and let firmware know the recovery status via _OST
ACPI call.

Since EDR is a hybrid service, DPC service shall be enabled in OS even
if AER is not natively enabled in OS.

Signed-off-by: Kuppuswamy Sathyanarayanan 

---
  drivers/pci/pcie/Kconfig|  10 +
  drivers/pci/pcie/dpc.c  | 326 ++--

Thanks for the review comments.

I'll be looking for Keith's reviewed-by for this eventually.

It looks like this could be split into some smaller patches:

   - Add dpc_dev.native_dpc (hopefully we can find a less confusing name)
   - Convert dpc_handler() to dpc_handler() + dpc_process_error()
   - Add EDR support

Ok. I will split them in next patch set.



  drivers/pci/pcie/portdrv_core.c |   9 +-
  3 files changed, 329 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 5cbdbca904ac..55f65d1cbc9e 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -142,3 +142,13 @@ config PCIE_PTM
  
  	  This is only useful if you have devices that support PTM, but it

  is safe to enable even if you don't.
+
+config PCIE_EDR
+   bool "PCI Express Error Disconnect Recover support"
+   default n
+   depends on PCIE_DPC && ACPI
+   help
+ This options adds Error Disconnect Recover support as specified
+ in PCI firmware specification v3.2 Downstream Port Containment
+ Related Enhancements ECN. Enable this if you want to support hybrid
+ DPC model which uses both firmware and OS to implement DPC.
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 7b77754a82de..bdf4ca8a0acb 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -11,6 +11,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "portdrv.h"

  #include "../pci.h"
@@ -20,8 +21,23 @@ struct dpc_dev {
u16 cap_pos;
boolrp_extensions;
u8  rp_log_size;
+   boolnative_dpc;

This is going to be way too confusing with a "native_dpc" in both the
struct pci_host_bridge and the struct dpc_dev.

Any suggestions? what about native_mode ?



+   pci_ers_result_terror_state;
+#ifdef CONFIG_ACPI
+   struct acpi_device  *adev;
+#endif
  };
  
+#ifdef CONFIG_ACPI

+
+#define EDR_PORT_ENABLE_DSM 0x0C
+#define EDR_PORT_LOCATE_DSM 0x0D
+
+static const guid_t pci_acpi_dsm_guid =
+   GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
+ 0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
+#endif
+
  static const char * const rp_pio_error_string[] = {
"Configuration Request received UR Completion",/* Bit Position 
0  */
"Configuration Request received CA Completion",/* Bit Position 
1  */
@@ -67,6 +83,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
if (!dpc)
return;
  
+	if (!dpc->native_dpc)

+   return;
+
save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
if (!save_state)
return;
@@ -88,6 +107,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
if (!dpc)
return;
  
+	if (!dpc->native_dpc)

+   return;
+
save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
if (!save_state)
return;
@@ -224,10 +246,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev 
*dev,
return 1;
  }
  
-static irqreturn_t dpc_handler(int irq, void *context)

+static void dpc_process_error(struct dpc_dev *dpc)
  {
struct aer_err_info info;
-   struct dpc_dev *dpc = context;
struct pci_dev *pdev = dpc->dev->port;
struct device *dev = &dpc->dev->device;
u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
@@ -261,6 +282,13 @@ static irqreturn_t dpc_handler(int irq, void *context)
  
  	/* We configure DPC so it only triggers on ERR_FATAL */

pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
+}
+
+static irqreturn_t dpc_handler(int irq, void *context)
+{
+   struct dpc_dev *dpc = context;
+
+   dpc_process_error(dpc);
  
  	return IRQ_HANDLED;

  }
@@ -283,6 +311,230 @@ static irqreturn_t dpc_irq(int irq, void *context)
return IRQ_HANDLED;
  }
  
+void dpc_error_resume(struct pci_dev *dev)

Looks like this should be static?

yes. I will fix it in next versi

Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support

2019-04-10 Thread Bjorn Helgaas

On Tue, Mar 19, 2019 at 01:47:29PM -0700, 
sathyanarayanan.kuppusw...@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan 
> 
> As per PCI firmware specification v3.2 ECN
> (https://members.pcisig.com/wg/PCI-SIG/document/12614), when firmware
> owns Downstream Port Containment (DPC), its expected to use the "Error
> Disconnect Recover" (EDR) notification to alert OSPM of a DPC event and
> if OS supports EDR, its expected to handle the software state invalidation
> and port recovery in OS and let firmware know the recovery status via _OST
> ACPI call.
> 
> Since EDR is a hybrid service, DPC service shall be enabled in OS even
> if AER is not natively enabled in OS.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan 
> 
> ---
>  drivers/pci/pcie/Kconfig|  10 +
>  drivers/pci/pcie/dpc.c  | 326 ++--

I'll be looking for Keith's reviewed-by for this eventually.

It looks like this could be split into some smaller patches:

  - Add dpc_dev.native_dpc (hopefully we can find a less confusing name)
  - Convert dpc_handler() to dpc_handler() + dpc_process_error()
  - Add EDR support

>  drivers/pci/pcie/portdrv_core.c |   9 +-
>  3 files changed, 329 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index 5cbdbca904ac..55f65d1cbc9e 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -142,3 +142,13 @@ config PCIE_PTM
>  
> This is only useful if you have devices that support PTM, but it
> is safe to enable even if you don't.
> +
> +config PCIE_EDR
> + bool "PCI Express Error Disconnect Recover support"
> + default n
> + depends on PCIE_DPC && ACPI
> + help
> +   This options adds Error Disconnect Recover support as specified
> +   in PCI firmware specification v3.2 Downstream Port Containment
> +   Related Enhancements ECN. Enable this if you want to support hybrid
> +   DPC model which uses both firmware and OS to implement DPC.
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 7b77754a82de..bdf4ca8a0acb 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -11,6 +11,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "portdrv.h"
>  #include "../pci.h"
> @@ -20,8 +21,23 @@ struct dpc_dev {
>   u16 cap_pos;
>   boolrp_extensions;
>   u8  rp_log_size;
> + boolnative_dpc;

This is going to be way too confusing with a "native_dpc" in both the
struct pci_host_bridge and the struct dpc_dev.

> + pci_ers_result_terror_state;
> +#ifdef CONFIG_ACPI
> + struct acpi_device  *adev;
> +#endif
>  };
>  
> +#ifdef CONFIG_ACPI
> +
> +#define EDR_PORT_ENABLE_DSM 0x0C
> +#define EDR_PORT_LOCATE_DSM 0x0D
> +
> +static const guid_t pci_acpi_dsm_guid =
> + GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
> +   0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
> +#endif
> +
>  static const char * const rp_pio_error_string[] = {
>   "Configuration Request received UR Completion",  /* Bit Position 0  */
>   "Configuration Request received CA Completion",  /* Bit Position 1  */
> @@ -67,6 +83,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
>   if (!dpc)
>   return;
>  
> + if (!dpc->native_dpc)
> + return;
> +
>   save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>   if (!save_state)
>   return;
> @@ -88,6 +107,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
>   if (!dpc)
>   return;
>  
> + if (!dpc->native_dpc)
> + return;
> +
>   save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
>   if (!save_state)
>   return;
> @@ -224,10 +246,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev 
> *dev,
>   return 1;
>  }
>  
> -static irqreturn_t dpc_handler(int irq, void *context)
> +static void dpc_process_error(struct dpc_dev *dpc)
>  {
>   struct aer_err_info info;
> - struct dpc_dev *dpc = context;
>   struct pci_dev *pdev = dpc->dev->port;
>   struct device *dev = &dpc->dev->device;
>   u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
> @@ -261,6 +282,13 @@ static irqreturn_t dpc_handler(int irq, void *context)
>  
>   /* We configure DPC so it only triggers on ERR_FATAL */
>   pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
> +}
> +
> +static irqreturn_t dpc_handler(int irq, void *context)
> +{
> + struct dpc_dev *dpc = context;
> +
> + dpc_process_error(dpc);
>  
>   return IRQ_HANDLED;
>  }
> @@ -283,6 +311,230 @@ static irqreturn_t dpc_irq(int irq, void *context)
>   return IRQ_HANDLED;
>  }
>  
> +void dpc_error_resume(struct pci_dev *dev)

Looks like this should be static?

> +{
> + struct dpc_dev *dpc;
> +
> + dpc = to_dpc_dev(de

[PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support

2019-03-19 Thread sathyanarayanan . kuppuswamy

From: Kuppuswamy Sathyanarayanan 

As per PCI firmware specification v3.2 ECN
(https://members.pcisig.com/wg/PCI-SIG/document/12614), when firmware
owns Downstream Port Containment (DPC), its expected to use the "Error
Disconnect Recover" (EDR) notification to alert OSPM of a DPC event and
if OS supports EDR, its expected to handle the software state invalidation
and port recovery in OS and let firmware know the recovery status via _OST
ACPI call.

Since EDR is a hybrid service, DPC service shall be enabled in OS even
if AER is not natively enabled in OS.

Signed-off-by: Kuppuswamy Sathyanarayanan 

---
 drivers/pci/pcie/Kconfig|  10 +
 drivers/pci/pcie/dpc.c  | 326 ++--
 drivers/pci/pcie/portdrv_core.c |   9 +-
 3 files changed, 329 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 5cbdbca904ac..55f65d1cbc9e 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -142,3 +142,13 @@ config PCIE_PTM
 
  This is only useful if you have devices that support PTM, but it
  is safe to enable even if you don't.
+
+config PCIE_EDR
+   bool "PCI Express Error Disconnect Recover support"
+   default n
+   depends on PCIE_DPC && ACPI
+   help
+ This options adds Error Disconnect Recover support as specified
+ in PCI firmware specification v3.2 Downstream Port Containment
+ Related Enhancements ECN. Enable this if you want to support hybrid
+ DPC model which uses both firmware and OS to implement DPC.
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 7b77754a82de..bdf4ca8a0acb 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "portdrv.h"
 #include "../pci.h"
@@ -20,8 +21,23 @@ struct dpc_dev {
u16 cap_pos;
boolrp_extensions;
u8  rp_log_size;
+   boolnative_dpc;
+   pci_ers_result_terror_state;
+#ifdef CONFIG_ACPI
+   struct acpi_device  *adev;
+#endif
 };
 
+#ifdef CONFIG_ACPI
+
+#define EDR_PORT_ENABLE_DSM 0x0C
+#define EDR_PORT_LOCATE_DSM 0x0D
+
+static const guid_t pci_acpi_dsm_guid =
+   GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
+ 0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
+#endif
+
 static const char * const rp_pio_error_string[] = {
"Configuration Request received UR Completion",  /* Bit Position 0  */
"Configuration Request received CA Completion",  /* Bit Position 1  */
@@ -67,6 +83,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
if (!dpc)
return;
 
+   if (!dpc->native_dpc)
+   return;
+
save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
if (!save_state)
return;
@@ -88,6 +107,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
if (!dpc)
return;
 
+   if (!dpc->native_dpc)
+   return;
+
save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
if (!save_state)
return;
@@ -224,10 +246,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev 
*dev,
return 1;
 }
 
-static irqreturn_t dpc_handler(int irq, void *context)
+static void dpc_process_error(struct dpc_dev *dpc)
 {
struct aer_err_info info;
-   struct dpc_dev *dpc = context;
struct pci_dev *pdev = dpc->dev->port;
struct device *dev = &dpc->dev->device;
u16 cap = dpc->cap_pos, status, source, reason, ext_reason;
@@ -261,6 +282,13 @@ static irqreturn_t dpc_handler(int irq, void *context)
 
/* We configure DPC so it only triggers on ERR_FATAL */
pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
+}
+
+static irqreturn_t dpc_handler(int irq, void *context)
+{
+   struct dpc_dev *dpc = context;
+
+   dpc_process_error(dpc);
 
return IRQ_HANDLED;
 }
@@ -283,6 +311,230 @@ static irqreturn_t dpc_irq(int irq, void *context)
return IRQ_HANDLED;
 }
 
+void dpc_error_resume(struct pci_dev *dev)
+{
+   struct dpc_dev *dpc;
+
+   dpc = to_dpc_dev(dev);
+   if (!dpc)
+   return;
+
+   dpc->error_state = PCI_ERS_RESULT_RECOVERED;
+}
+
+#ifdef CONFIG_ACPI
+
+/*
+ * _DSM wrapper function to enable/disable DPC port.
+ * @dpc   : DPC device structure
+ * @enable: status of DPC port (0 or 1).
+ *
+ * returns 0 on success or errno on failure.
+ */
+static int acpi_enable_dpc_port(struct dpc_dev *dpc, bool enable)
+{
+   union acpi_object *obj;
+   int status;
+   union acpi_object argv4;
+
+   /* Check whether EDR_PORT_ENABLE_DSM is supported in firmware */
+   status = acpi_check_dsm(dpc->adev->handle, &pci_acpi_dsm_guid, 1,
+   1 << EDR_PORT_ENABLE_DSM);
+   if (!statu

Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support

Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support

Re: [PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support

[PATCH v2 2/2] PCI/DPC: Add Error Disconnect Recover (EDR) support

4 matches

Site Navigation

Mail list logo

Footer information