[PATCH v2 0/4] PCI: query active service list

[PATCH v2 0/4] PCI: query active service list

[Oza Pawandeep]

The list of patches provides querying facility to all services drivers.
They can query any other service, to see if it is active/registered.

This is useful when multiple service drivers do not want to race for
actions based on service interrupt trigger.

for e.g. if both AER and DPC are triggering, AER should not race since
DPC is going to bring down the link and all the devices beneath, and
going to recover HW.

Another example;
If hot plug service is not active DPC can do enumeration.

Changes Since v1:
Sinan's comment implemented.

Oza Pawandeep (4):
  PCI: Add port service list node for pci_dev.
  PCI/portdrv: Add/Remove port services to the list
  PCI/portdrv: Implement interface to query the registered service
  PCI/AER: Dont do recovery when DPC is enabled

 drivers/pci/pcie/aer/aerdrv_core.c | 33 ++++++++++++++++++++++++++++++++
 drivers/pci/pcie/portdrv_core.c    | 39 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c                |  1 +
 include/linux/pci.h                |  2 ++
 include/linux/pcieport_if.h        |  4 +++-
 5 files changed, 78 insertions(+), 1 deletion(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 0/4] PCI: query active service list

[Oza Pawandeep]

The list of patches provides querying facility to all services drivers.
They can query any other service, to see if it is active/registered.

This is useful when multiple service drivers do not want to race for
actions based on service interrupt trigger.

for e.g. if both AER and DPC are triggering, AER should not race since
DPC is going to bring down the link and all the devices beneath, and
going to recover HW.

Another example;
If hot plug service is not active DPC can do enumeration.

Changes Since v1:
Sinan's comment implemented.

Oza Pawandeep (4):
  PCI: Add port service list node for pci_dev.
  PCI/portdrv: Add/Remove port services to the list
  PCI/portdrv: Implement interface to query the registered service
  PCI/AER: Dont do recovery when DPC is enabled

 drivers/pci/pcie/aer/aerdrv_core.c | 33 ++++++++++++++++++++++++++++++++
 drivers/pci/pcie/portdrv_core.c    | 39 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c                |  1 +
 include/linux/pci.h                |  2 ++
 include/linux/pcieport_if.h        |  4 +++-
 5 files changed, 78 insertions(+), 1 deletion(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 0/4] PCI: query active service list

[Oza Pawandeep]

The list of patches provides querying facility to all services drivers.
They can query any other service, to see if it is active/registered.

This is useful when multiple service drivers do not want to race for
actions based on service interrupt trigger.

for e.g. if both AER and DPC are triggering, AER should not race since
DPC is going to bring down the link and all the devices beneath, and
going to recover HW.

Another example;
If hot plug service is not active DPC can do enumeration.

Changes Since v1:
Sinan's comment implemented.

Oza Pawandeep (4):
  PCI: Add port service list node for pci_dev.
  PCI/portdrv: Add/Remove port services to the list
  PCI/portdrv: Implement interface to query the registered service
  PCI/AER: Dont do recovery when DPC is enabled

 drivers/pci/pcie/aer/aerdrv_core.c | 33 ++++++++++++++++++++++++++++++++
 drivers/pci/pcie/portdrv_core.c    | 39 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c                |  1 +
 include/linux/pci.h                |  2 ++
 include/linux/pcieport_if.h        |  4 +++-
 5 files changed, 78 insertions(+), 1 deletion(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] PCI: Add port service list node for pci_dev.

[Oza Pawandeep]

This patch adds the list node to keep track of services registered to
pci port driver.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 14e0ea1..f772979 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1931,6 +1931,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 		return NULL;
 
 	INIT_LIST_HEAD(&dev->bus_list);
+	INIT_LIST_HEAD(&dev->service_list);
 	dev->dev.type = &pci_dev_type;
 	dev->bus = pci_bus_get(bus);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 96c9498..eb86a4b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -408,6 +408,8 @@ struct pci_dev {
 	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
 	struct bin_attribute *res_attr_wc[DEVICE_COUNT_RESOURCE]; /* sysfs file for WC mapping of resources */
 
+	struct list_head service_list;	/* node in per-service list */
+
 #ifdef CONFIG_PCIE_PTM
 	unsigned int	ptm_root:1;
 	unsigned int	ptm_enabled:1;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 1/4] PCI: Add port service list node for pci_dev.

[Oza Pawandeep]

This patch adds the list node to keep track of services registered to
pci port driver.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 14e0ea1..f772979 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1931,6 +1931,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 		return NULL;
 
 	INIT_LIST_HEAD(&dev->bus_list);
+	INIT_LIST_HEAD(&dev->service_list);
 	dev->dev.type = &pci_dev_type;
 	dev->bus = pci_bus_get(bus);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 96c9498..eb86a4b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -408,6 +408,8 @@ struct pci_dev {
 	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
 	struct bin_attribute *res_attr_wc[DEVICE_COUNT_RESOURCE]; /* sysfs file for WC mapping of resources */
 
+	struct list_head service_list;	/* node in per-service list */
+
 #ifdef CONFIG_PCIE_PTM
 	unsigned int	ptm_root:1;
 	unsigned int	ptm_enabled:1;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 1/4] PCI: Add port service list node for pci_dev.

[Oza Pawandeep]

This patch adds the list node to keep track of services registered to
pci port driver.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 14e0ea1..f772979 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1931,6 +1931,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 		return NULL;
 
 	INIT_LIST_HEAD(&dev->bus_list);
+	INIT_LIST_HEAD(&dev->service_list);
 	dev->dev.type = &pci_dev_type;
 	dev->bus = pci_bus_get(bus);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 96c9498..eb86a4b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -408,6 +408,8 @@ struct pci_dev {
 	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
 	struct bin_attribute *res_attr_wc[DEVICE_COUNT_RESOURCE]; /* sysfs file for WC mapping of resources */
 
+	struct list_head service_list;	/* node in per-service list */
+
 #ifdef CONFIG_PCIE_PTM
 	unsigned int	ptm_root:1;
 	unsigned int	ptm_enabled:1;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] PCI/portdrv: Add/Remove port services to the list

[Oza Pawandeep]

This patch adds and removes the port service to the service list.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index a592103..6bfe986 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -454,6 +454,8 @@ static int pcie_port_probe_service(struct device *dev)
 	if (status)
 		return status;
 
+	list_add_tail(&pciedev->slist, &pciedev->port->service_list);
+
 	get_device(dev);
 	return 0;
 }
@@ -477,6 +479,9 @@ static int pcie_port_remove_service(struct device *dev)
 
 	pciedev = to_pcie_device(dev);
 	driver = to_service_driver(dev->driver);
+
+	list_del(&pciedev->slist);
+
 	if (driver && driver->remove) {
 		driver->remove(pciedev);
 		put_device(dev);
diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h
index b69769d..9d05621 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -31,6 +31,7 @@ struct pcie_device {
 	u32		service;    /* Port service this device represents */
 	void		*priv_data; /* Service Private Data */
 	struct device	device;     /* Generic Device Interface */
+	struct list_head slist;     /* List of services */
 };
 #define to_pcie_device(d) container_of(d, struct pcie_device, device)
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 2/4] PCI/portdrv: Add/Remove port services to the list

[Oza Pawandeep]

This patch adds and removes the port service to the service list.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index a592103..6bfe986 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -454,6 +454,8 @@ static int pcie_port_probe_service(struct device *dev)
 	if (status)
 		return status;
 
+	list_add_tail(&pciedev->slist, &pciedev->port->service_list);
+
 	get_device(dev);
 	return 0;
 }
@@ -477,6 +479,9 @@ static int pcie_port_remove_service(struct device *dev)
 
 	pciedev = to_pcie_device(dev);
 	driver = to_service_driver(dev->driver);
+
+	list_del(&pciedev->slist);
+
 	if (driver && driver->remove) {
 		driver->remove(pciedev);
 		put_device(dev);
diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h
index b69769d..9d05621 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -31,6 +31,7 @@ struct pcie_device {
 	u32		service;    /* Port service this device represents */
 	void		*priv_data; /* Service Private Data */
 	struct device	device;     /* Generic Device Interface */
+	struct list_head slist;     /* List of services */
 };
 #define to_pcie_device(d) container_of(d, struct pcie_device, device)
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 2/4] PCI/portdrv: Add/Remove port services to the list

[Oza Pawandeep]

This patch adds and removes the port service to the service list.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index a592103..6bfe986 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -454,6 +454,8 @@ static int pcie_port_probe_service(struct device *dev)
 	if (status)
 		return status;
 
+	list_add_tail(&pciedev->slist, &pciedev->port->service_list);
+
 	get_device(dev);
 	return 0;
 }
@@ -477,6 +479,9 @@ static int pcie_port_remove_service(struct device *dev)
 
 	pciedev = to_pcie_device(dev);
 	driver = to_service_driver(dev->driver);
+
+	list_del(&pciedev->slist);
+
 	if (driver && driver->remove) {
 		driver->remove(pciedev);
 		put_device(dev);
diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h
index b69769d..9d05621 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -31,6 +31,7 @@ struct pcie_device {
 	u32		service;    /* Port service this device represents */
 	void		*priv_data; /* Service Private Data */
 	struct device	device;     /* Generic Device Interface */
+	struct list_head slist;     /* List of services */
 };
 #define to_pcie_device(d) container_of(d, struct pcie_device, device)
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] PCI/portdrv: Implement interface to query the registered service

[Oza Pawandeep]

This patch implements query service interface. So that, any port service
driver can query to know, if the service is active or not.

When multiple service drivers try to take actions, these service drivers
could race or could have conflict of interest.

For e.g. when DPC is enabled, AER should not attempt recovery.

The other interface walks up the parent till root bridge, so that on each
pci_dev service can be queried.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 6bfe986..c7681d9 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -490,6 +490,40 @@ static int pcie_port_remove_service(struct device *dev)
 }
 
 /**
+ * pcie_port_query_service - query the associated port service.
+ * dev: pcie device
+ * @port service: PCI express port service
+ */
+bool pcie_port_query_service(struct pci_dev *dev, u32 port_service)
+{
+	struct pcie_device *pdev;
+
+	list_for_each_entry(pdev, &dev->service_list, slist) {
+		if (pdev->service == port_service)
+			return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(pcie_port_query_service);
+
+/**
+ * pcie_port_upstream_bridge - returns immediate upstream bridge.
+ * dev: pcie device
+ */
+struct pci_dev *pcie_port_upstream_bridge(struct pci_dev *dev)
+{
+	struct pci_dev *parent;
+
+	parent = pci_upstream_bridge(dev);
+
+	if (parent && pci_is_pcie(parent))
+		return parent;
+
+	return NULL;
+}
+EXPORT_SYMBOL(pcie_port_upstream_bridge);
+
+/**
  * pcie_port_shutdown_service - shut down given PCI Express port service
  * @dev: PCI Express port service device to handle
  *
diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h
index 9d05621..8322443 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -68,5 +68,6 @@ struct pcie_port_service_driver {
 
 int pcie_port_service_register(struct pcie_port_service_driver *new);
 void pcie_port_service_unregister(struct pcie_port_service_driver *new);
-
+bool pcie_port_query_service(struct pci_dev *dev, u32 port_service);
+struct pci_dev *pcie_port_upstream_bridge(struct pci_dev *dev);
 #endif /* _PCIEPORT_IF_H_ */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 3/4] PCI/portdrv: Implement interface to query the registered service

[Oza Pawandeep]

This patch implements query service interface. So that, any port service
driver can query to know, if the service is active or not.

When multiple service drivers try to take actions, these service drivers
could race or could have conflict of interest.

For e.g. when DPC is enabled, AER should not attempt recovery.

The other interface walks up the parent till root bridge, so that on each
pci_dev service can be queried.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 6bfe986..c7681d9 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -490,6 +490,40 @@ static int pcie_port_remove_service(struct device *dev)
 }
 
 /**
+ * pcie_port_query_service - query the associated port service.
+ * dev: pcie device
+ * @port service: PCI express port service
+ */
+bool pcie_port_query_service(struct pci_dev *dev, u32 port_service)
+{
+	struct pcie_device *pdev;
+
+	list_for_each_entry(pdev, &dev->service_list, slist) {
+		if (pdev->service == port_service)
+			return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(pcie_port_query_service);
+
+/**
+ * pcie_port_upstream_bridge - returns immediate upstream bridge.
+ * dev: pcie device
+ */
+struct pci_dev *pcie_port_upstream_bridge(struct pci_dev *dev)
+{
+	struct pci_dev *parent;
+
+	parent = pci_upstream_bridge(dev);
+
+	if (parent && pci_is_pcie(parent))
+		return parent;
+
+	return NULL;
+}
+EXPORT_SYMBOL(pcie_port_upstream_bridge);
+
+/**
  * pcie_port_shutdown_service - shut down given PCI Express port service
  * @dev: PCI Express port service device to handle
  *
diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h
index 9d05621..8322443 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -68,5 +68,6 @@ struct pcie_port_service_driver {
 
 int pcie_port_service_register(struct pcie_port_service_driver *new);
 void pcie_port_service_unregister(struct pcie_port_service_driver *new);
-
+bool pcie_port_query_service(struct pci_dev *dev, u32 port_service);
+struct pci_dev *pcie_port_upstream_bridge(struct pci_dev *dev);
 #endif /* _PCIEPORT_IF_H_ */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 3/4] PCI/portdrv: Implement interface to query the registered service

[Oza Pawandeep]

This patch implements query service interface. So that, any port service
driver can query to know, if the service is active or not.

When multiple service drivers try to take actions, these service drivers
could race or could have conflict of interest.

For e.g. when DPC is enabled, AER should not attempt recovery.

The other interface walks up the parent till root bridge, so that on each
pci_dev service can be queried.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 6bfe986..c7681d9 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -490,6 +490,40 @@ static int pcie_port_remove_service(struct device *dev)
 }
 
 /**
+ * pcie_port_query_service - query the associated port service.
+ * dev: pcie device
+ * @port service: PCI express port service
+ */
+bool pcie_port_query_service(struct pci_dev *dev, u32 port_service)
+{
+	struct pcie_device *pdev;
+
+	list_for_each_entry(pdev, &dev->service_list, slist) {
+		if (pdev->service == port_service)
+			return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(pcie_port_query_service);
+
+/**
+ * pcie_port_upstream_bridge - returns immediate upstream bridge.
+ * dev: pcie device
+ */
+struct pci_dev *pcie_port_upstream_bridge(struct pci_dev *dev)
+{
+	struct pci_dev *parent;
+
+	parent = pci_upstream_bridge(dev);
+
+	if (parent && pci_is_pcie(parent))
+		return parent;
+
+	return NULL;
+}
+EXPORT_SYMBOL(pcie_port_upstream_bridge);
+
+/**
  * pcie_port_shutdown_service - shut down given PCI Express port service
  * @dev: PCI Express port service device to handle
  *
diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h
index 9d05621..8322443 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -68,5 +68,6 @@ struct pcie_port_service_driver {
 
 int pcie_port_service_register(struct pcie_port_service_driver *new);
 void pcie_port_service_unregister(struct pcie_port_service_driver *new);
-
+bool pcie_port_query_service(struct pci_dev *dev, u32 port_service);
+struct pci_dev *pcie_port_upstream_bridge(struct pci_dev *dev);
 #endif /* _PCIEPORT_IF_H_ */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Oza Pawandeep]

PCI Express Base Specification, Rev. 4.0 Version 0.9
6.2.10: Downstream Port Containment (DPC)

DPC is an optional normative feature of a Downstream Port. DPC halts PCI
Express traffic below a Downstream Port after an unmasked uncorrectable
error is detected at or below the Port, avoiding the potential spread of
any data corruption, and permitting error recovery if supported by
software

Triggering DPC disables its Link by directing the LTSSM to the Disabled
state. Once the LTSSM reaches the Disabled state, it remains in that
state until the DPC Trigger Status bit is Cleared

So when DPC service is active and registered to port driver, AER should
not attempt to recover, since DPC will be removing downstream devices,
and do the recovery.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 7448052..a9108ea 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -482,6 +482,27 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
 }
 
 /**
+ * pcie_port_query_uptream_service - query upstream service
+ * @dev: pointer to a pci_dev data structure of agent detecting an error
+ * @service: service to be queried
+ *
+ * Invoked to know the status of the service for pci device.
+ */
+static bool pcie_port_query_uptream_service(struct pci_dev *dev, u32 service)
+{
+	struct pci_dev *upstream_dev = dev;
+
+	do {
+		if (pcie_port_query_service(upstream_dev, service))
+			return true;
+		upstream_dev = pcie_port_upstream_bridge(upstream_dev);
+	} while (upstream_dev);
+
+	return false;
+}
+
+
+/**
  * do_recovery - handle nonfatal/fatal error recovery process
  * @dev: pointer to a pci_dev data structure of agent detecting an error
  * @severity: error severity type
@@ -495,6 +516,18 @@ static void do_recovery(struct pci_dev *dev, int severity)
 	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
 	enum pci_channel_state state;
 
+	/*
+	 * If DPC is enabled, there is no need to attempt recovery.
+	 * Since DPC disables its Link by directing the LTSSM to
+	 * the Disabled state.
+	 * DPC driver will take care of the recovery, there is no need
+	 * for AER driver to race.
+	 */
+	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
+		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
+		return;
+	}
+
 	if (severity == AER_FATAL)
 		state = pci_channel_io_frozen;
 	else
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Oza Pawandeep]

PCI Express Base Specification, Rev. 4.0 Version 0.9
6.2.10: Downstream Port Containment (DPC)

DPC is an optional normative feature of a Downstream Port. DPC halts PCI
Express traffic below a Downstream Port after an unmasked uncorrectable
error is detected at or below the Port, avoiding the potential spread of
any data corruption, and permitting error recovery if supported by
software

Triggering DPC disables its Link by directing the LTSSM to the Disabled
state. Once the LTSSM reaches the Disabled state, it remains in that
state until the DPC Trigger Status bit is Cleared

So when DPC service is active and registered to port driver, AER should
not attempt to recover, since DPC will be removing downstream devices,
and do the recovery.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 7448052..a9108ea 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -482,6 +482,27 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
 }
 
 /**
+ * pcie_port_query_uptream_service - query upstream service
+ * @dev: pointer to a pci_dev data structure of agent detecting an error
+ * @service: service to be queried
+ *
+ * Invoked to know the status of the service for pci device.
+ */
+static bool pcie_port_query_uptream_service(struct pci_dev *dev, u32 service)
+{
+	struct pci_dev *upstream_dev = dev;
+
+	do {
+		if (pcie_port_query_service(upstream_dev, service))
+			return true;
+		upstream_dev = pcie_port_upstream_bridge(upstream_dev);
+	} while (upstream_dev);
+
+	return false;
+}
+
+
+/**
  * do_recovery - handle nonfatal/fatal error recovery process
  * @dev: pointer to a pci_dev data structure of agent detecting an error
  * @severity: error severity type
@@ -495,6 +516,18 @@ static void do_recovery(struct pci_dev *dev, int severity)
 	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
 	enum pci_channel_state state;
 
+	/*
+	 * If DPC is enabled, there is no need to attempt recovery.
+	 * Since DPC disables its Link by directing the LTSSM to
+	 * the Disabled state.
+	 * DPC driver will take care of the recovery, there is no need
+	 * for AER driver to race.
+	 */
+	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
+		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
+		return;
+	}
+
 	if (severity == AER_FATAL)
 		state = pci_channel_io_frozen;
 	else
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Oza Pawandeep]

PCI Express Base Specification, Rev. 4.0 Version 0.9
6.2.10: Downstream Port Containment (DPC)

DPC is an optional normative feature of a Downstream Port. DPC halts PCI
Express traffic below a Downstream Port after an unmasked uncorrectable
error is detected at or below the Port, avoiding the potential spread of
any data corruption, and permitting error recovery if supported by
software

Triggering DPC disables its Link by directing the LTSSM to the Disabled
state. Once the LTSSM reaches the Disabled state, it remains in that
state until the DPC Trigger Status bit is Cleared

So when DPC service is active and registered to port driver, AER should
not attempt to recover, since DPC will be removing downstream devices,
and do the recovery.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 7448052..a9108ea 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -482,6 +482,27 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
 }
 
 /**
+ * pcie_port_query_uptream_service - query upstream service
+ * @dev: pointer to a pci_dev data structure of agent detecting an error
+ * @service: service to be queried
+ *
+ * Invoked to know the status of the service for pci device.
+ */
+static bool pcie_port_query_uptream_service(struct pci_dev *dev, u32 service)
+{
+	struct pci_dev *upstream_dev = dev;
+
+	do {
+		if (pcie_port_query_service(upstream_dev, service))
+			return true;
+		upstream_dev = pcie_port_upstream_bridge(upstream_dev);
+	} while (upstream_dev);
+
+	return false;
+}
+
+
+/**
  * do_recovery - handle nonfatal/fatal error recovery process
  * @dev: pointer to a pci_dev data structure of agent detecting an error
  * @severity: error severity type
@@ -495,6 +516,18 @@ static void do_recovery(struct pci_dev *dev, int severity)
 	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
 	enum pci_channel_state state;
 
+	/*
+	 * If DPC is enabled, there is no need to attempt recovery.
+	 * Since DPC disables its Link by directing the LTSSM to
+	 * the Disabled state.
+	 * DPC driver will take care of the recovery, there is no need
+	 * for AER driver to race.
+	 */
+	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
+		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
+		return;
+	}
+
 	if (severity == AER_FATAL)
 		state = pci_channel_io_frozen;
 	else
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Wed, Nov 15, 2017 at 10:26:48AM +0530, Oza Pawandeep wrote:
> PCI Express Base Specification, Rev. 4.0 Version 0.9
> 6.2.10: Downstream Port Containment (DPC)
> 
> DPC is an optional normative feature of a Downstream Port. DPC halts PCI
> Express traffic below a Downstream Port after an unmasked uncorrectable
> error is detected at or below the Port, avoiding the potential spread of
> any data corruption, and permitting error recovery if supported by
> software
> 
> Triggering DPC disables its Link by directing the LTSSM to the Disabled
> state. Once the LTSSM reaches the Disabled state, it remains in that
> state until the DPC Trigger Status bit is Cleared
> 
> So when DPC service is active and registered to port driver, AER should
> not attempt to recover, since DPC will be removing downstream devices,
> and do the recovery.
> 
> Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 7448052..a9108ea 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -482,6 +482,27 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
>  }
>  
>  /**
> + * pcie_port_query_uptream_service - query upstream service
> + * @dev: pointer to a pci_dev data structure of agent detecting an error
> + * @service: service to be queried
> + *
> + * Invoked to know the status of the service for pci device.
> + */
> +static bool pcie_port_query_uptream_service(struct pci_dev *dev, u32 service)
> +{
> +	struct pci_dev *upstream_dev = dev;
> +
> +	do {
> +		if (pcie_port_query_service(upstream_dev, service))
> +			return true;
> +		upstream_dev = pcie_port_upstream_bridge(upstream_dev);
> +	} while (upstream_dev);
> +
> +	return false;
> +}
> +
> +
> +/**
>   * do_recovery - handle nonfatal/fatal error recovery process
>   * @dev: pointer to a pci_dev data structure of agent detecting an error
>   * @severity: error severity type
> @@ -495,6 +516,18 @@ static void do_recovery(struct pci_dev *dev, int severity)
>  	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
>  	enum pci_channel_state state;
>  
> +	/*
> +	 * If DPC is enabled, there is no need to attempt recovery.
> +	 * Since DPC disables its Link by directing the LTSSM to
> +	 * the Disabled state.
> +	 * DPC driver will take care of the recovery, there is no need
> +	 * for AER driver to race.
> +	 */
> +	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
> +		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
> +		return;
> +	}

What happens without this test?

Does AER read registers from the now-disabled device and get ~0 data?
Or is AER reading registers from the port upstream from the disabled
device and trying to reset the device?

It looks like get_device_error_info() reads registers and doesn't
check to see whether it gets ~0 back.  I'm wondering if we *should* be
checking there and whether doing that would help mitigate the issue
here.

I don't really like the pcie_port_query_uptream_service() approach
(BTW, the name is misspelled) because it feels a little ad hoc and it
makes assumptions here in the AER code about what the DPC code is
doing, e.g., how it has configured PCI_EXP_DPC_CTL.

>  	if (severity == AER_FATAL)
>  		state = pci_channel_io_frozen;
>  	else
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
> a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Wed, Nov 15, 2017 at 10:26:48AM +0530, Oza Pawandeep wrote:
> PCI Express Base Specification, Rev. 4.0 Version 0.9
> 6.2.10: Downstream Port Containment (DPC)
> 
> DPC is an optional normative feature of a Downstream Port. DPC halts PCI
> Express traffic below a Downstream Port after an unmasked uncorrectable
> error is detected at or below the Port, avoiding the potential spread of
> any data corruption, and permitting error recovery if supported by
> software
> 
> Triggering DPC disables its Link by directing the LTSSM to the Disabled
> state. Once the LTSSM reaches the Disabled state, it remains in that
> state until the DPC Trigger Status bit is Cleared
> 
> So when DPC service is active and registered to port driver, AER should
> not attempt to recover, since DPC will be removing downstream devices,
> and do the recovery.
> 
> Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 7448052..a9108ea 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -482,6 +482,27 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
>  }
>  
>  /**
> + * pcie_port_query_uptream_service - query upstream service
> + * @dev: pointer to a pci_dev data structure of agent detecting an error
> + * @service: service to be queried
> + *
> + * Invoked to know the status of the service for pci device.
> + */
> +static bool pcie_port_query_uptream_service(struct pci_dev *dev, u32 service)
> +{
> +	struct pci_dev *upstream_dev = dev;
> +
> +	do {
> +		if (pcie_port_query_service(upstream_dev, service))
> +			return true;
> +		upstream_dev = pcie_port_upstream_bridge(upstream_dev);
> +	} while (upstream_dev);
> +
> +	return false;
> +}
> +
> +
> +/**
>   * do_recovery - handle nonfatal/fatal error recovery process
>   * @dev: pointer to a pci_dev data structure of agent detecting an error
>   * @severity: error severity type
> @@ -495,6 +516,18 @@ static void do_recovery(struct pci_dev *dev, int severity)
>  	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
>  	enum pci_channel_state state;
>  
> +	/*
> +	 * If DPC is enabled, there is no need to attempt recovery.
> +	 * Since DPC disables its Link by directing the LTSSM to
> +	 * the Disabled state.
> +	 * DPC driver will take care of the recovery, there is no need
> +	 * for AER driver to race.
> +	 */
> +	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
> +		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
> +		return;
> +	}

What happens without this test?

Does AER read registers from the now-disabled device and get ~0 data?
Or is AER reading registers from the port upstream from the disabled
device and trying to reset the device?

It looks like get_device_error_info() reads registers and doesn't
check to see whether it gets ~0 back.  I'm wondering if we *should* be
checking there and whether doing that would help mitigate the issue
here.

I don't really like the pcie_port_query_uptream_service() approach
(BTW, the name is misspelled) because it feels a little ad hoc and it
makes assumptions here in the AER code about what the DPC code is
doing, e.g., how it has configured PCI_EXP_DPC_CTL.

>  	if (severity == AER_FATAL)
>  		state = pci_channel_io_frozen;
>  	else
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
> a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Wed, Nov 15, 2017 at 10:26:48AM +0530, Oza Pawandeep wrote:
> PCI Express Base Specification, Rev. 4.0 Version 0.9
> 6.2.10: Downstream Port Containment (DPC)
> 
> DPC is an optional normative feature of a Downstream Port. DPC halts PCI
> Express traffic below a Downstream Port after an unmasked uncorrectable
> error is detected at or below the Port, avoiding the potential spread of
> any data corruption, and permitting error recovery if supported by
> software
> 
> Triggering DPC disables its Link by directing the LTSSM to the Disabled
> state. Once the LTSSM reaches the Disabled state, it remains in that
> state until the DPC Trigger Status bit is Cleared
> 
> So when DPC service is active and registered to port driver, AER should
> not attempt to recover, since DPC will be removing downstream devices,
> and do the recovery.
> 
> Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 7448052..a9108ea 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -482,6 +482,27 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
>  }
>  
>  /**
> + * pcie_port_query_uptream_service - query upstream service
> + * @dev: pointer to a pci_dev data structure of agent detecting an error
> + * @service: service to be queried
> + *
> + * Invoked to know the status of the service for pci device.
> + */
> +static bool pcie_port_query_uptream_service(struct pci_dev *dev, u32 service)
> +{
> +	struct pci_dev *upstream_dev = dev;
> +
> +	do {
> +		if (pcie_port_query_service(upstream_dev, service))
> +			return true;
> +		upstream_dev = pcie_port_upstream_bridge(upstream_dev);
> +	} while (upstream_dev);
> +
> +	return false;
> +}
> +
> +
> +/**
>   * do_recovery - handle nonfatal/fatal error recovery process
>   * @dev: pointer to a pci_dev data structure of agent detecting an error
>   * @severity: error severity type
> @@ -495,6 +516,18 @@ static void do_recovery(struct pci_dev *dev, int severity)
>  	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
>  	enum pci_channel_state state;
>  
> +	/*
> +	 * If DPC is enabled, there is no need to attempt recovery.
> +	 * Since DPC disables its Link by directing the LTSSM to
> +	 * the Disabled state.
> +	 * DPC driver will take care of the recovery, there is no need
> +	 * for AER driver to race.
> +	 */
> +	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
> +		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
> +		return;
> +	}

What happens without this test?

Does AER read registers from the now-disabled device and get ~0 data?
Or is AER reading registers from the port upstream from the disabled
device and trying to reset the device?

It looks like get_device_error_info() reads registers and doesn't
check to see whether it gets ~0 back.  I'm wondering if we *should* be
checking there and whether doing that would help mitigate the issue
here.

I don't really like the pcie_port_query_uptream_service() approach
(BTW, the name is misspelled) because it feels a little ad hoc and it
makes assumptions here in the AER code about what the DPC code is
doing, e.g., how it has configured PCI_EXP_DPC_CTL.

>  	if (severity == AER_FATAL)
>  		state = pci_channel_io_frozen;
>  	else
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
> a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Sinan Kaya]

Hi Bjorn,

On 11/15/2017 4:14 PM, Bjorn Helgaas wrote:
>> +	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
>> +		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
>> +		return;
>> +	}
> What happens without this test?
> 
> Does AER read registers from the now-disabled device and get ~0 data?
> Or is AER reading registers from the port upstream from the disabled
> device and trying to reset the device?
> 
> It looks like get_device_error_info() reads registers and doesn't
> check to see whether it gets ~0 back.  I'm wondering if we *should* be
> checking there and whether doing that would help mitigate the issue
> here.

The issue is two independent software entities are trying to recover the PCIe
link simultaneously. AER and DPC have two different approaches to link recovery.

AER makes a callback into the endpoint drivers for non-fatal errors and hope
that endpoint driver can recover the link. AER also makes a callback in the 
fatal error case but resets the link via secondary bus reset.

The DPC on the other hand stops the drivers immediately since HW took care of
link disable. (Endpoint register reads return ~0 at this point.) DPC driver clears
the interrupt from the DPC capability and brings the link up at the end. Full
enumeration/rescan follows this procedure to go back to functioning state. 

If we don't have this AER-DPC coordination, the endpoint driver gets confused since
it receives a stop command as well as a recover command at about the same time
depending on the timing.

Whether the AER driver reads ~0 or not really depends on timing. The link may come
up from the DPC driver by the time AER driver reaches here as an example.

Bad things do happen. We have seen this with e1000e driver.

Sinan

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Sinan Kaya]

Hi Bjorn,

On 11/15/2017 4:14 PM, Bjorn Helgaas wrote:
>> +	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
>> +		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
>> +		return;
>> +	}
> What happens without this test?
> 
> Does AER read registers from the now-disabled device and get ~0 data?
> Or is AER reading registers from the port upstream from the disabled
> device and trying to reset the device?
> 
> It looks like get_device_error_info() reads registers and doesn't
> check to see whether it gets ~0 back.  I'm wondering if we *should* be
> checking there and whether doing that would help mitigate the issue
> here.

The issue is two independent software entities are trying to recover the PCIe
link simultaneously. AER and DPC have two different approaches to link recovery.

AER makes a callback into the endpoint drivers for non-fatal errors and hope
that endpoint driver can recover the link. AER also makes a callback in the 
fatal error case but resets the link via secondary bus reset.

The DPC on the other hand stops the drivers immediately since HW took care of
link disable. (Endpoint register reads return ~0 at this point.) DPC driver clears
the interrupt from the DPC capability and brings the link up at the end. Full
enumeration/rescan follows this procedure to go back to functioning state. 

If we don't have this AER-DPC coordination, the endpoint driver gets confused since
it receives a stop command as well as a recover command at about the same time
depending on the timing.

Whether the AER driver reads ~0 or not really depends on timing. The link may come
up from the DPC driver by the time AER driver reaches here as an example.

Bad things do happen. We have seen this with e1000e driver.

Sinan

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Thu, Nov 16, 2017 at 09:03:37AM -0500, Sinan Kaya wrote:
> On 11/15/2017 4:14 PM, Bjorn Helgaas wrote:
> >> +	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
> >> +		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
> >> +		return;
> >> +	}
> > What happens without this test?
> > 
> > Does AER read registers from the now-disabled device and get ~0 data?
> > Or is AER reading registers from the port upstream from the disabled
> > device and trying to reset the device?
> > 
> > It looks like get_device_error_info() reads registers and doesn't
> > check to see whether it gets ~0 back.  I'm wondering if we *should* be
> > checking there and whether doing that would help mitigate the issue
> > here.
> 
> The issue is two independent software entities are trying to recover
> the PCIe link simultaneously. AER and DPC have two different
> approaches to link recovery.
> 
> AER makes a callback into the endpoint drivers for non-fatal errors
> and hope that endpoint driver can recover the link. AER also makes a
> callback in the fatal error case but resets the link via secondary
> bus reset.
> 
> The DPC on the other hand stops the drivers immediately since HW
> took care of link disable. (Endpoint register reads return ~0 at
> this point.) DPC driver clears the interrupt from the DPC capability
> and brings the link up at the end. Full enumeration/rescan follows
> this procedure to go back to functioning state. 
> 
> If we don't have this AER-DPC coordination, the endpoint driver gets
> confused since it receives a stop command as well as a recover
> command at about the same time depending on the timing.
> 
> Whether the AER driver reads ~0 or not really depends on timing. The
> link may come up from the DPC driver by the time AER driver reaches
> here as an example.
> 
> Bad things do happen. We have seen this with e1000e driver.

I don't doubt that bad things happen.  I'm just trying to understand
exactly *what* bad things happen and how, so we can fix them cleanly.

I don't know exactly what you mean by "DPC stops the drivers
immediately".  Since the DPC hardware disables the Link, I *think*
you probably mean that driver accesses to the device start failing
(whether the driver notices this is a whole different question).

When the DPC hardware disables the Link, it causes a hot reset for
downstream components.  The DPC interrupt_event_handler() doesn't do
much except remove the device (which detaches the driver) and clear
the DPC Trigger Status bit (which allows hardware to try to retrain
the Link).

So the "stop" and "recover" commands you mention must be related to
AER.  I guess these would be some of the driver callbacks
(error_detected(), mmio_enabled(), slot_reset(), reset_prepare(),
reset_done(), resume())?

In any case, I agree that it probably doesn't make sense to call any
of these callbacks if the DPC driver has already detached the driver
and re-attached it.  The device state is gone because of the hot reset
and the driver state is gone because of the detach/re-attach.

However, I'm not so sure about the period *before* the DPC driver
detaches the driver.  The description of error_detected() says it
cannot assume the device is accessible, so I think there might be an
argument that AER *should* call this for DPC events so the driver has
a chance to clean up before being unceremoniously detached.

I suspect this all probably requires tighter integration between DPC
and AER, and I'm totally fine with that.  I think the current
separation as separate "drivers" is pretty artificial anyway.

Bjorn

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Thu, Nov 16, 2017 at 09:03:37AM -0500, Sinan Kaya wrote:
> On 11/15/2017 4:14 PM, Bjorn Helgaas wrote:
> >> +	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
> >> +		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
> >> +		return;
> >> +	}
> > What happens without this test?
> > 
> > Does AER read registers from the now-disabled device and get ~0 data?
> > Or is AER reading registers from the port upstream from the disabled
> > device and trying to reset the device?
> > 
> > It looks like get_device_error_info() reads registers and doesn't
> > check to see whether it gets ~0 back.  I'm wondering if we *should* be
> > checking there and whether doing that would help mitigate the issue
> > here.
> 
> The issue is two independent software entities are trying to recover
> the PCIe link simultaneously. AER and DPC have two different
> approaches to link recovery.
> 
> AER makes a callback into the endpoint drivers for non-fatal errors
> and hope that endpoint driver can recover the link. AER also makes a
> callback in the fatal error case but resets the link via secondary
> bus reset.
> 
> The DPC on the other hand stops the drivers immediately since HW
> took care of link disable. (Endpoint register reads return ~0 at
> this point.) DPC driver clears the interrupt from the DPC capability
> and brings the link up at the end. Full enumeration/rescan follows
> this procedure to go back to functioning state. 
> 
> If we don't have this AER-DPC coordination, the endpoint driver gets
> confused since it receives a stop command as well as a recover
> command at about the same time depending on the timing.
> 
> Whether the AER driver reads ~0 or not really depends on timing. The
> link may come up from the DPC driver by the time AER driver reaches
> here as an example.
> 
> Bad things do happen. We have seen this with e1000e driver.

I don't doubt that bad things happen.  I'm just trying to understand
exactly *what* bad things happen and how, so we can fix them cleanly.

I don't know exactly what you mean by "DPC stops the drivers
immediately".  Since the DPC hardware disables the Link, I *think*
you probably mean that driver accesses to the device start failing
(whether the driver notices this is a whole different question).

When the DPC hardware disables the Link, it causes a hot reset for
downstream components.  The DPC interrupt_event_handler() doesn't do
much except remove the device (which detaches the driver) and clear
the DPC Trigger Status bit (which allows hardware to try to retrain
the Link).

So the "stop" and "recover" commands you mention must be related to
AER.  I guess these would be some of the driver callbacks
(error_detected(), mmio_enabled(), slot_reset(), reset_prepare(),
reset_done(), resume())?

In any case, I agree that it probably doesn't make sense to call any
of these callbacks if the DPC driver has already detached the driver
and re-attached it.  The device state is gone because of the hot reset
and the driver state is gone because of the detach/re-attach.

However, I'm not so sure about the period *before* the DPC driver
detaches the driver.  The description of error_detected() says it
cannot assume the device is accessible, so I think there might be an
argument that AER *should* call this for DPC events so the driver has
a chance to clean up before being unceremoniously detached.

I suspect this all probably requires tighter integration between DPC
and AER, and I'm totally fine with that.  I think the current
separation as separate "drivers" is pretty artificial anyway.

Bjorn

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Thu, Nov 16, 2017 at 09:03:37AM -0500, Sinan Kaya wrote:
> On 11/15/2017 4:14 PM, Bjorn Helgaas wrote:
> >> +	if (pcie_port_query_uptream_service(dev, PCIE_PORT_SERVICE_DPC)) {
> >> +		dev_info(&dev->dev, "AER: Device recovery to be done by DPC\n");
> >> +		return;
> >> +	}
> > What happens without this test?
> > 
> > Does AER read registers from the now-disabled device and get ~0 data?
> > Or is AER reading registers from the port upstream from the disabled
> > device and trying to reset the device?
> > 
> > It looks like get_device_error_info() reads registers and doesn't
> > check to see whether it gets ~0 back.  I'm wondering if we *should* be
> > checking there and whether doing that would help mitigate the issue
> > here.
> 
> The issue is two independent software entities are trying to recover
> the PCIe link simultaneously. AER and DPC have two different
> approaches to link recovery.
> 
> AER makes a callback into the endpoint drivers for non-fatal errors
> and hope that endpoint driver can recover the link. AER also makes a
> callback in the fatal error case but resets the link via secondary
> bus reset.
> 
> The DPC on the other hand stops the drivers immediately since HW
> took care of link disable. (Endpoint register reads return ~0 at
> this point.) DPC driver clears the interrupt from the DPC capability
> and brings the link up at the end. Full enumeration/rescan follows
> this procedure to go back to functioning state. 
> 
> If we don't have this AER-DPC coordination, the endpoint driver gets
> confused since it receives a stop command as well as a recover
> command at about the same time depending on the timing.
> 
> Whether the AER driver reads ~0 or not really depends on timing. The
> link may come up from the DPC driver by the time AER driver reaches
> here as an example.
> 
> Bad things do happen. We have seen this with e1000e driver.

I don't doubt that bad things happen.  I'm just trying to understand
exactly *what* bad things happen and how, so we can fix them cleanly.

I don't know exactly what you mean by "DPC stops the drivers
immediately".  Since the DPC hardware disables the Link, I *think*
you probably mean that driver accesses to the device start failing
(whether the driver notices this is a whole different question).

When the DPC hardware disables the Link, it causes a hot reset for
downstream components.  The DPC interrupt_event_handler() doesn't do
much except remove the device (which detaches the driver) and clear
the DPC Trigger Status bit (which allows hardware to try to retrain
the Link).

So the "stop" and "recover" commands you mention must be related to
AER.  I guess these would be some of the driver callbacks
(error_detected(), mmio_enabled(), slot_reset(), reset_prepare(),
reset_done(), resume())?

In any case, I agree that it probably doesn't make sense to call any
of these callbacks if the DPC driver has already detached the driver
and re-attached it.  The device state is gone because of the hot reset
and the driver state is gone because of the detach/re-attach.

However, I'm not so sure about the period *before* the DPC driver
detaches the driver.  The description of error_detected() says it
cannot assume the device is accessible, so I think there might be an
argument that AER *should* call this for DPC events so the driver has
a chance to clean up before being unceremoniously detached.

I suspect this all probably requires tighter integration between DPC
and AER, and I'm totally fine with that.  I think the current
separation as separate "drivers" is pretty artificial anyway.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Sinan Kaya]

>>
>> Whether the AER driver reads ~0 or not really depends on timing. The
>> link may come up from the DPC driver by the time AER driver reaches
>> here as an example.
>>
>> Bad things do happen. We have seen this with e1000e driver.
> 
> I don't doubt that bad things happen.  I'm just trying to understand
> exactly *what* bad things happen and how, so we can fix them cleanly.
> 

This was random crashes in the e1000e drivers accompanied with stack
traces coming from WARN and msi allocation routines.

> I don't know exactly what you mean by "DPC stops the drivers
> immediately".  Since the DPC hardware disables the Link, I *think*
> you probably mean that driver accesses to the device start failing
> (whether the driver notices this is a whole different question).

Sorry for not being clear.

I was talking about the pci_stop_and_remove_bus_device() call in DPC's
interrupt_event_handler() function as the "stop command".

> 
> When the DPC hardware disables the Link, it causes a hot reset for
> downstream components.  The DPC interrupt_event_handler() doesn't do
> much except remove the device (which detaches the driver) and clear
> the DPC Trigger Status bit (which allows hardware to try to retrain
> the Link).
> 

That's true.

> So the "stop" and "recover" commands you mention must be related to
> AER.  

I was talking about pci_stop_and_remove_bus_device() vs. error_detected()
as "stop" and "recover"

> I guess these would be some of the driver callbacks
> (error_detected(), mmio_enabled(), slot_reset(), reset_prepare(),
> reset_done(), resume())?
> 
> In any case, I agree that it probably doesn't make sense to call any
> of these callbacks if the DPC driver has already detached the driver
> and re-attached it.  The device state is gone because of the hot reset
> and the driver state is gone because of the detach/re-attach.
> 
> However, I'm not so sure about the period *before* the DPC driver
> detaches the driver.  The description of error_detected() says it
> cannot assume the device is accessible, so I think there might be an
> argument that AER *should* call this for DPC events so the driver has
> a chance to clean up before being unceremoniously detached.

Makes sense. Let us work on this.

> 
> I suspect this all probably requires tighter integration between DPC
> and AER, and I'm totally fine with that.  I think the current
> separation as separate "drivers" is pretty artificial anyway.

Got it. We will try to plumb DPC error handling into AER driver's error
handling mechanism.

Another question:

What do you think about the rescan following link up? The only entity
that does rescan today is hotplug after DPC recovery. There could be
a platform with DPC support but no hotplug support. 

How should we handle it?

We can call rescan from DPC all the time.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Sinan Kaya]

>>
>> Whether the AER driver reads ~0 or not really depends on timing. The
>> link may come up from the DPC driver by the time AER driver reaches
>> here as an example.
>>
>> Bad things do happen. We have seen this with e1000e driver.
> 
> I don't doubt that bad things happen.  I'm just trying to understand
> exactly *what* bad things happen and how, so we can fix them cleanly.
> 

This was random crashes in the e1000e drivers accompanied with stack
traces coming from WARN and msi allocation routines.

> I don't know exactly what you mean by "DPC stops the drivers
> immediately".  Since the DPC hardware disables the Link, I *think*
> you probably mean that driver accesses to the device start failing
> (whether the driver notices this is a whole different question).

Sorry for not being clear.

I was talking about the pci_stop_and_remove_bus_device() call in DPC's
interrupt_event_handler() function as the "stop command".

> 
> When the DPC hardware disables the Link, it causes a hot reset for
> downstream components.  The DPC interrupt_event_handler() doesn't do
> much except remove the device (which detaches the driver) and clear
> the DPC Trigger Status bit (which allows hardware to try to retrain
> the Link).
> 

That's true.

> So the "stop" and "recover" commands you mention must be related to
> AER.  

I was talking about pci_stop_and_remove_bus_device() vs. error_detected()
as "stop" and "recover"

> I guess these would be some of the driver callbacks
> (error_detected(), mmio_enabled(), slot_reset(), reset_prepare(),
> reset_done(), resume())?
> 
> In any case, I agree that it probably doesn't make sense to call any
> of these callbacks if the DPC driver has already detached the driver
> and re-attached it.  The device state is gone because of the hot reset
> and the driver state is gone because of the detach/re-attach.
> 
> However, I'm not so sure about the period *before* the DPC driver
> detaches the driver.  The description of error_detected() says it
> cannot assume the device is accessible, so I think there might be an
> argument that AER *should* call this for DPC events so the driver has
> a chance to clean up before being unceremoniously detached.

Makes sense. Let us work on this.

> 
> I suspect this all probably requires tighter integration between DPC
> and AER, and I'm totally fine with that.  I think the current
> separation as separate "drivers" is pretty artificial anyway.

Got it. We will try to plumb DPC error handling into AER driver's error
handling mechanism.

Another question:

What do you think about the rescan following link up? The only entity
that does rescan today is hotplug after DPC recovery. There could be
a platform with DPC support but no hotplug support. 

How should we handle it?

We can call rescan from DPC all the time.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Thu, Nov 16, 2017 at 03:52:47PM -0500, Sinan Kaya wrote:
> >>
> >> Whether the AER driver reads ~0 or not really depends on timing. The
> >> link may come up from the DPC driver by the time AER driver reaches
> >> here as an example.
> >>
> >> Bad things do happen. We have seen this with e1000e driver.
> > 
> > I don't doubt that bad things happen.  I'm just trying to understand
> > exactly *what* bad things happen and how, so we can fix them cleanly.
> 
> This was random crashes in the e1000e drivers accompanied with stack
> traces coming from WARN and msi allocation routines.

I didn't look in detail, but I'm not sure there's sufficient locking
in the AER path to make it safe from concurrent device removal.  I
suspect AER could be improved both with respect to handling ~0 data
and this potential concurrency issue.

> > So the "stop" and "recover" commands you mention must be related to
> > AER.  
> 
> I was talking about pci_stop_and_remove_bus_device() vs. error_detected()
> as "stop" and "recover"

Thanks for clearing that up!

> > I suspect this all probably requires tighter integration between DPC
> > and AER, and I'm totally fine with that.  I think the current
> > separation as separate "drivers" is pretty artificial anyway.
> 
> Got it. We will try to plumb DPC error handling into AER driver's error
> handling mechanism.

Looking at the AER code today, I noticed it already uses "DPC" in
another sense.  I don't know what it stands for there (probably
"deferred" something), but I don't think it's "Downstream Port
Containment" :)

> What do you think about the rescan following link up? The only entity
> that does rescan today is hotplug after DPC recovery. There could be
> a platform with DPC support but no hotplug support. 
> 
> How should we handle it?

Good question.  If your system does support both DPC and hotplug, I
assume the link comes back up after you clear DPC Trigger Status.
Does pciehp notice that "link up" event and add the device back?

So I think your question is whether the DPC code should explicitly do
a rescan so that if we don't have pciehp, we'll still automatically
rediscover the device.  I dunno, maybe.  Seems like a plausible idea
anyway.

Bjorn

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Thu, Nov 16, 2017 at 03:52:47PM -0500, Sinan Kaya wrote:
> >>
> >> Whether the AER driver reads ~0 or not really depends on timing. The
> >> link may come up from the DPC driver by the time AER driver reaches
> >> here as an example.
> >>
> >> Bad things do happen. We have seen this with e1000e driver.
> > 
> > I don't doubt that bad things happen.  I'm just trying to understand
> > exactly *what* bad things happen and how, so we can fix them cleanly.
> 
> This was random crashes in the e1000e drivers accompanied with stack
> traces coming from WARN and msi allocation routines.

I didn't look in detail, but I'm not sure there's sufficient locking
in the AER path to make it safe from concurrent device removal.  I
suspect AER could be improved both with respect to handling ~0 data
and this potential concurrency issue.

> > So the "stop" and "recover" commands you mention must be related to
> > AER.  
> 
> I was talking about pci_stop_and_remove_bus_device() vs. error_detected()
> as "stop" and "recover"

Thanks for clearing that up!

> > I suspect this all probably requires tighter integration between DPC
> > and AER, and I'm totally fine with that.  I think the current
> > separation as separate "drivers" is pretty artificial anyway.
> 
> Got it. We will try to plumb DPC error handling into AER driver's error
> handling mechanism.

Looking at the AER code today, I noticed it already uses "DPC" in
another sense.  I don't know what it stands for there (probably
"deferred" something), but I don't think it's "Downstream Port
Containment" :)

> What do you think about the rescan following link up? The only entity
> that does rescan today is hotplug after DPC recovery. There could be
> a platform with DPC support but no hotplug support. 
> 
> How should we handle it?

Good question.  If your system does support both DPC and hotplug, I
assume the link comes back up after you clear DPC Trigger Status.
Does pciehp notice that "link up" event and add the device back?

So I think your question is whether the DPC code should explicitly do
a rescan so that if we don't have pciehp, we'll still automatically
rediscover the device.  I dunno, maybe.  Seems like a plausible idea
anyway.

Bjorn

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Bjorn Helgaas]

On Thu, Nov 16, 2017 at 03:52:47PM -0500, Sinan Kaya wrote:
> >>
> >> Whether the AER driver reads ~0 or not really depends on timing. The
> >> link may come up from the DPC driver by the time AER driver reaches
> >> here as an example.
> >>
> >> Bad things do happen. We have seen this with e1000e driver.
> > 
> > I don't doubt that bad things happen.  I'm just trying to understand
> > exactly *what* bad things happen and how, so we can fix them cleanly.
> 
> This was random crashes in the e1000e drivers accompanied with stack
> traces coming from WARN and msi allocation routines.

I didn't look in detail, but I'm not sure there's sufficient locking
in the AER path to make it safe from concurrent device removal.  I
suspect AER could be improved both with respect to handling ~0 data
and this potential concurrency issue.

> > So the "stop" and "recover" commands you mention must be related to
> > AER.  
> 
> I was talking about pci_stop_and_remove_bus_device() vs. error_detected()
> as "stop" and "recover"

Thanks for clearing that up!

> > I suspect this all probably requires tighter integration between DPC
> > and AER, and I'm totally fine with that.  I think the current
> > separation as separate "drivers" is pretty artificial anyway.
> 
> Got it. We will try to plumb DPC error handling into AER driver's error
> handling mechanism.

Looking at the AER code today, I noticed it already uses "DPC" in
another sense.  I don't know what it stands for there (probably
"deferred" something), but I don't think it's "Downstream Port
Containment" :)

> What do you think about the rescan following link up? The only entity
> that does rescan today is hotplug after DPC recovery. There could be
> a platform with DPC support but no hotplug support. 
> 
> How should we handle it?

Good question.  If your system does support both DPC and hotplug, I
assume the link comes back up after you clear DPC Trigger Status.
Does pciehp notice that "link up" event and add the device back?

So I think your question is whether the DPC code should explicitly do
a rescan so that if we don't have pciehp, we'll still automatically
rediscover the device.  I dunno, maybe.  Seems like a plausible idea
anyway.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Sinan Kaya]

On 11/17/2017 7:02 PM, Bjorn Helgaas wrote:
>> What do you think about the rescan following link up? The only entity
>> that does rescan today is hotplug after DPC recovery. There could be
>> a platform with DPC support but no hotplug support. 
>>
>> How should we handle it?
> Good question.  If your system does support both DPC and hotplug, I
> assume the link comes back up after you clear DPC Trigger Status.
> Does pciehp notice that "link up" event and add the device back?

that's correct. we see a link up indication from hotplug driver followed
by a rescan.

> 
> So I think your question is whether the DPC code should explicitly do
> a rescan so that if we don't have pciehp, we'll still automatically
> rediscover the device.  I dunno, maybe.  Seems like a plausible idea
> anyway.

OK, will add it to DPC driver for completeness.


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Sinan Kaya]

On 11/17/2017 7:02 PM, Bjorn Helgaas wrote:
>> What do you think about the rescan following link up? The only entity
>> that does rescan today is hotplug after DPC recovery. There could be
>> a platform with DPC support but no hotplug support. 
>>
>> How should we handle it?
> Good question.  If your system does support both DPC and hotplug, I
> assume the link comes back up after you clear DPC Trigger Status.
> Does pciehp notice that "link up" event and add the device back?

that's correct. we see a link up indication from hotplug driver followed
by a rescan.

> 
> So I think your question is whether the DPC code should explicitly do
> a rescan so that if we don't have pciehp, we'll still automatically
> rediscover the device.  I dunno, maybe.  Seems like a plausible idea
> anyway.

OK, will add it to DPC driver for completeness.


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

RE: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[David Laight]

From: Sinan Kaya
> Sent: 16 November 2017 14:04
...
> The issue is two independent software entities are trying to recover the PCIe
> link simultaneously. AER and DPC have two different approaches to link recovery.
> 
> AER makes a callback into the endpoint drivers for non-fatal errors and hope
> that endpoint driver can recover the link. AER also makes a callback in the
> fatal error case but resets the link via secondary bus reset.
> 
> The DPC on the other hand stops the drivers immediately since HW took care of
> link disable. (Endpoint register reads return ~0 at this point.)

What happens if the 'user' driver doesn't define the error reporting callbacks?
It might be hardened against the ~0u returns from reads - so not OOPS.
It might be appropriate to call the remove() function instead.

> DPC driver clears
> the interrupt from the DPC capability and brings the link up at the end. Full
> enumeration/rescan follows this procedure to go back to functioning state.

That might not be a good idea, very likely it will fail again immediately.

	David

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[David Laight]

From: Sinan Kaya
> Sent: 16 November 2017 14:04
...
> The issue is two independent software entities are trying to recover the PCIe
> link simultaneously. AER and DPC have two different approaches to link recovery.
> 
> AER makes a callback into the endpoint drivers for non-fatal errors and hope
> that endpoint driver can recover the link. AER also makes a callback in the
> fatal error case but resets the link via secondary bus reset.
> 
> The DPC on the other hand stops the drivers immediately since HW took care of
> link disable. (Endpoint register reads return ~0 at this point.)

What happens if the 'user' driver doesn't define the error reporting callbacks?
It might be hardened against the ~0u returns from reads - so not OOPS.
It might be appropriate to call the remove() function instead.

> DPC driver clears
> the interrupt from the DPC capability and brings the link up at the end. Full
> enumeration/rescan follows this procedure to go back to functioning state.

That might not be a good idea, very likely it will fail again immediately.

	David

RE: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[David Laight]

RnJvbTogU2luYW4gS2F5YQ0KPiBTZW50OiAxNiBOb3ZlbWJlciAyMDE3IDE0OjA0DQouLi4NCj4g
VGhlIGlzc3VlIGlzIHR3byBpbmRlcGVuZGVudCBzb2Z0d2FyZSBlbnRpdGllcyBhcmUgdHJ5aW5n
IHRvIHJlY292ZXIgdGhlIFBDSWUNCj4gbGluayBzaW11bHRhbmVvdXNseS4gQUVSIGFuZCBEUEMg
aGF2ZSB0d28gZGlmZmVyZW50IGFwcHJvYWNoZXMgdG8gbGluayByZWNvdmVyeS4NCj4gDQo+IEFF
UiBtYWtlcyBhIGNhbGxiYWNrIGludG8gdGhlIGVuZHBvaW50IGRyaXZlcnMgZm9yIG5vbi1mYXRh
bCBlcnJvcnMgYW5kIGhvcGUNCj4gdGhhdCBlbmRwb2ludCBkcml2ZXIgY2FuIHJlY292ZXIgdGhl
IGxpbmsuIEFFUiBhbHNvIG1ha2VzIGEgY2FsbGJhY2sgaW4gdGhlDQo+IGZhdGFsIGVycm9yIGNh
c2UgYnV0IHJlc2V0cyB0aGUgbGluayB2aWEgc2Vjb25kYXJ5IGJ1cyByZXNldC4NCj4gDQo+IFRo
ZSBEUEMgb24gdGhlIG90aGVyIGhhbmQgc3RvcHMgdGhlIGRyaXZlcnMgaW1tZWRpYXRlbHkgc2lu
Y2UgSFcgdG9vayBjYXJlIG9mDQo+IGxpbmsgZGlzYWJsZS4gKEVuZHBvaW50IHJlZ2lzdGVyIHJl
YWRzIHJldHVybiB+MCBhdCB0aGlzIHBvaW50LikNCg0KV2hhdCBoYXBwZW5zIGlmIHRoZSAndXNl
cicgZHJpdmVyIGRvZXNuJ3QgZGVmaW5lIHRoZSBlcnJvciByZXBvcnRpbmcgY2FsbGJhY2tzPw0K
SXQgbWlnaHQgYmUgaGFyZGVuZWQgYWdhaW5zdCB0aGUgfjB1IHJldHVybnMgZnJvbSByZWFkcyAt
IHNvIG5vdCBPT1BTLg0KSXQgbWlnaHQgYmUgYXBwcm9wcmlhdGUgdG8gY2FsbCB0aGUgcmVtb3Zl
KCkgZnVuY3Rpb24gaW5zdGVhZC4NCg0KPiBEUEMgZHJpdmVyIGNsZWFycw0KPiB0aGUgaW50ZXJy
dXB0IGZyb20gdGhlIERQQyBjYXBhYmlsaXR5IGFuZCBicmluZ3MgdGhlIGxpbmsgdXAgYXQgdGhl
IGVuZC4gRnVsbA0KPiBlbnVtZXJhdGlvbi9yZXNjYW4gZm9sbG93cyB0aGlzIHByb2NlZHVyZSB0
byBnbyBiYWNrIHRvIGZ1bmN0aW9uaW5nIHN0YXRlLg0KDQpUaGF0IG1pZ2h0IG5vdCBiZSBhIGdv
b2QgaWRlYSwgdmVyeSBsaWtlbHkgaXQgd2lsbCBmYWlsIGFnYWluIGltbWVkaWF0ZWx5Lg0KDQoJ
RGF2aWQNCg0K

Re: [PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Sinan Kaya]

On 11/21/2017 11:25 AM, David Laight wrote:
>> The DPC on the other hand stops the drivers immediately since HW took care of
>> link disable. (Endpoint register reads return ~0 at this point.)
> What happens if the 'user' driver doesn't define the error reporting callbacks?
> It might be hardened against the ~0u returns from reads - so not OOPS.
> It might be appropriate to call the remove() function instead.

This is what the DPC driver does in its interrupt handler. 

http://elixir.free-electrons.com/linux/latest/ident/interrupt_event_handler

My understanding is that this will eventually call the remove() function on the
endpoint driver eventually. 

Bjorn had concerns that we are not calling the error handler if registered and
then calling remove() callback while the driver is in the middle of something
could be bad. 

He had concerns if remove() would leave something in a bad state so recovery
would really not work at all and kernel crashes eventually due to data corruption. 

Oza and I are looking for a way to plumb DPC's error handling into AER driver
so that PCI framework has a single place to look for error handling.

for dpc:
1. If an error handler registered, call it for all children devices
2. Remove all children devices from the bus
3. Recover the link with DPC
4. Rescan the entire bus and install the drivers again

> 
>> DPC driver clears
>> the interrupt from the DPC capability and brings the link up at the end. Full
>> enumeration/rescan follows this procedure to go back to functioning state.
> That might not be a good idea, very likely it will fail again immediately.

We can add a policy parameter and not bring up the link if you want to do
troubleshooting at the point of failure or have a way to define how the
system response should be. 

DPC causes a hot reset on the bus. Endpoint should go to reset state and we should
be able to bring up the link without any problems under normal circumstances.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Sinan Kaya]

On 11/21/2017 11:25 AM, David Laight wrote:
>> The DPC on the other hand stops the drivers immediately since HW took care of
>> link disable. (Endpoint register reads return ~0 at this point.)
> What happens if the 'user' driver doesn't define the error reporting callbacks?
> It might be hardened against the ~0u returns from reads - so not OOPS.
> It might be appropriate to call the remove() function instead.

This is what the DPC driver does in its interrupt handler. 

http://elixir.free-electrons.com/linux/latest/ident/interrupt_event_handler

My understanding is that this will eventually call the remove() function on the
endpoint driver eventually. 

Bjorn had concerns that we are not calling the error handler if registered and
then calling remove() callback while the driver is in the middle of something
could be bad. 

He had concerns if remove() would leave something in a bad state so recovery
would really not work at all and kernel crashes eventually due to data corruption. 

Oza and I are looking for a way to plumb DPC's error handling into AER driver
so that PCI framework has a single place to look for error handling.

for dpc:
1. If an error handler registered, call it for all children devices
2. Remove all children devices from the bus
3. Recover the link with DPC
4. Rescan the entire bus and install the drivers again

> 
>> DPC driver clears
>> the interrupt from the DPC capability and brings the link up at the end. Full
>> enumeration/rescan follows this procedure to go back to functioning state.
> That might not be a good idea, very likely it will fail again immediately.

We can add a policy parameter and not bring up the link if you want to do
troubleshooting at the point of failure or have a way to define how the
system response should be. 

DPC causes a hot reset on the bus. Endpoint should go to reset state and we should
be able to bring up the link without any problems under normal circumstances.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.