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

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

[Pawandeep Oza]

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 
goint to recover HW.

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 | 12 ++++++++++++
 drivers/pci/pcie/portdrv_core.c    | 29 +++++++++++++++++++++++++++++
 drivers/pci/probe.c                |  1 +
 include/linux/pci.h                |  2 ++
 include/linux/pcieport_if.h        |  3 ++-
 5 files changed, 46 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 0/4] PCI: query active service list

[Pawandeep Oza]

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 
goint to recover HW.

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 | 12 ++++++++++++
 drivers/pci/pcie/portdrv_core.c    | 29 +++++++++++++++++++++++++++++
 drivers/pci/probe.c                |  1 +
 include/linux/pci.h                |  2 ++
 include/linux/pcieport_if.h        |  3 ++-
 5 files changed, 46 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 1/4] PCI: Add port service list node for pci_dev.

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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 1/4] PCI: Add port service list node for pci_dev.

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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 1/4] PCI: Add port service list node for pci_dev.

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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 2/4] PCI/portdrv: Add/Remove port services to the list

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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 2/4] PCI/portdrv: Add/Remove port services to the list

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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 3/4] PCI/portdrv: Implement interface to query the registered service

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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 interface walks up the parent till root bridge, and if any of the
downstream port has DPC service active, it stops there, since DPC will be
removing all the devices beneath.

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..63fa57a 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -490,6 +490,30 @@ static int pcie_port_remove_service(struct device *dev)
 }
 
 /**
+ * pcie_port_service_query - query if particula port service is enabled.
+ * dev: pcie device
+ * @port service: PCI express port service
+ */
+int pcie_port_query_service(struct pci_dev *dev, u32 port_service)
+{
+	struct pcie_device *pdev;
+	struct pci_dev *parent, *this = dev;
+
+	do {
+		list_for_each_entry(pdev, &this->service_list, slist) {
+			if (pdev->service == port_service)
+				return 1;
+		}
+		parent = pci_upstream_bridge(this);
+		this = parent;
+	} while (parent && pci_is_pcie(parent));
+
+	return 0;
+}
+EXPORT_SYMBOL(pcie_port_query_service);
+
+
+/**
  * 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..bb8febb 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -68,5 +68,5 @@ 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);
-
+int pcie_port_query_service(struct pci_dev *dev, u32 port_service);
 #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 3/4] PCI/portdrv: Implement interface to query the registered service

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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 interface walks up the parent till root bridge, and if any of the
downstream port has DPC service active, it stops there, since DPC will be
removing all the devices beneath.

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..63fa57a 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -490,6 +490,30 @@ static int pcie_port_remove_service(struct device *dev)
 }
 
 /**
+ * pcie_port_service_query - query if particula port service is enabled.
+ * dev: pcie device
+ * @port service: PCI express port service
+ */
+int pcie_port_query_service(struct pci_dev *dev, u32 port_service)
+{
+	struct pcie_device *pdev;
+	struct pci_dev *parent, *this = dev;
+
+	do {
+		list_for_each_entry(pdev, &this->service_list, slist) {
+			if (pdev->service == port_service)
+				return 1;
+		}
+		parent = pci_upstream_bridge(this);
+		this = parent;
+	} while (parent && pci_is_pcie(parent));
+
+	return 0;
+}
+EXPORT_SYMBOL(pcie_port_query_service);
+
+
+/**
  * 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..bb8febb 100644
--- a/include/linux/pcieport_if.h
+++ b/include/linux/pcieport_if.h
@@ -68,5 +68,5 @@ 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);
-
+int pcie_port_query_service(struct pci_dev *dev, u32 port_service);
 #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 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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.

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..8d48d5c 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -495,6 +495,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_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 4/4] PCI/AER: Dont do recovery when DPC is enabled

[Pawandeep Oza]

From: Oza Pawandeep <poza@codeaurora.org>

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.

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..8d48d5c 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -495,6 +495,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_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.

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

[Sinan Kaya]

Some nits only.

>  /**
> + * pcie_port_service_query - query if particula port service is enabled.
> + * dev: pcie device
> + * @port service: PCI express port service
> + */
> +int pcie_port_query_service(struct pci_dev *dev, u32 port_service)
> +{
> +	struct pcie_device *pdev;
> +	struct pci_dev *parent, *this = dev;
> +
> +	do {

While I understand the motivation why this function is searching for DPC capable
parent until the root port, the name of the function does not represent this. 

It might make sense to split this into two where the first function just queries
the capabilities of the immediate device (the list_for_each_section).

> +		list_for_each_entry(pdev, &this->service_list, slist) {
> +			if (pdev->service == port_service)
> +				return 1;
> +		}

Another function to query all parents until you reach the root port.

> +		parent = pci_upstream_bridge(this);
> +		this = parent;
> +	} while (parent && pci_is_pcie(parent));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(pcie_port_query_service);
> +





-- 
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: [3/4] PCI/portdrv: Implement interface to query the registered service

[Sinan Kaya]

Some nits only.

>  /**
> + * pcie_port_service_query - query if particula port service is enabled.
> + * dev: pcie device
> + * @port service: PCI express port service
> + */
> +int pcie_port_query_service(struct pci_dev *dev, u32 port_service)
> +{
> +	struct pcie_device *pdev;
> +	struct pci_dev *parent, *this = dev;
> +
> +	do {

While I understand the motivation why this function is searching for DPC capable
parent until the root port, the name of the function does not represent this. 

It might make sense to split this into two where the first function just queries
the capabilities of the immediate device (the list_for_each_section).

> +		list_for_each_entry(pdev, &this->service_list, slist) {
> +			if (pdev->service == port_service)
> +				return 1;
> +		}

Another function to query all parents until you reach the root port.

> +		parent = pci_upstream_bridge(this);
> +		this = parent;
> +	} while (parent && pci_is_pcie(parent));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(pcie_port_query_service);
> +





-- 
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.

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

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

[Sinan Kaya]

Some nits only.

>  /**
> + * pcie_port_service_query - query if particula port service is enabled.
> + * dev: pcie device
> + * @port service: PCI express port service
> + */
> +int pcie_port_query_service(struct pci_dev *dev, u32 port_service)
> +{
> +	struct pcie_device *pdev;
> +	struct pci_dev *parent, *this = dev;
> +
> +	do {

While I understand the motivation why this function is searching for DPC capable
parent until the root port, the name of the function does not represent this. 

It might make sense to split this into two where the first function just queries
the capabilities of the immediate device (the list_for_each_section).

> +		list_for_each_entry(pdev, &this->service_list, slist) {
> +			if (pdev->service == port_service)
> +				return 1;
> +		}

Another function to query all parents until you reach the root port.

> +		parent = pci_upstream_bridge(this);
> +		this = parent;
> +	} while (parent && pci_is_pcie(parent));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(pcie_port_query_service);
> +





-- 
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: [3/4] PCI/portdrv: Implement interface to query the registered service

[poza]

On 2017-11-14 02:22, Sinan Kaya wrote:
> Some nits only.
> 
>>  /**
>> + * pcie_port_service_query - query if particula port service is 
>> enabled.
>> + * dev: pcie device
>> + * @port service: PCI express port service
>> + */
>> +int pcie_port_query_service(struct pci_dev *dev, u32 port_service)
>> +{
>> +	struct pcie_device *pdev;
>> +	struct pci_dev *parent, *this = dev;
>> +
>> +	do {
> 
> While I understand the motivation why this function is searching for 
> DPC capable
> parent until the root port, the name of the function does not represent 
> this.
> 

Will change it to more relevant one.

> It might make sense to split this into two where the first function 
> just queries
> the capabilities of the immediate device (the list_for_each_section).
> 
>> +		list_for_each_entry(pdev, &this->service_list, slist) {
>> +			if (pdev->service == port_service)
>> +				return 1;
>> +		}
> 
> Another function to query all parents until you reach the root port.
> 
>> +		parent = pci_upstream_bridge(this);
>> +		this = parent;
>> +	} while (parent && pci_is_pcie(parent));
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(pcie_port_query_service);
>> +

yes that makes sense; will split the function as you are suggesting.

Regards,
Oza.

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

[poza]

On 2017-11-14 02:22, Sinan Kaya wrote:
> Some nits only.
> 
>>  /**
>> + * pcie_port_service_query - query if particula port service is 
>> enabled.
>> + * dev: pcie device
>> + * @port service: PCI express port service
>> + */
>> +int pcie_port_query_service(struct pci_dev *dev, u32 port_service)
>> +{
>> +	struct pcie_device *pdev;
>> +	struct pci_dev *parent, *this = dev;
>> +
>> +	do {
> 
> While I understand the motivation why this function is searching for 
> DPC capable
> parent until the root port, the name of the function does not represent 
> this.
> 

Will change it to more relevant one.

> It might make sense to split this into two where the first function 
> just queries
> the capabilities of the immediate device (the list_for_each_section).
> 
>> +		list_for_each_entry(pdev, &this->service_list, slist) {
>> +			if (pdev->service == port_service)
>> +				return 1;
>> +		}
> 
> Another function to query all parents until you reach the root port.
> 
>> +		parent = pci_upstream_bridge(this);
>> +		this = parent;
>> +	} while (parent && pci_is_pcie(parent));
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(pcie_port_query_service);
>> +

yes that makes sense; will split the function as you are suggesting.

Regards,
Oza.


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

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

[poza at codeaurora.org]

On 2017-11-14 02:22, Sinan Kaya wrote:
> Some nits only.
> 
>>  /**
>> + * pcie_port_service_query - query if particula port service is 
>> enabled.
>> + * dev: pcie device
>> + * @port service: PCI express port service
>> + */
>> +int pcie_port_query_service(struct pci_dev *dev, u32 port_service)
>> +{
>> +	struct pcie_device *pdev;
>> +	struct pci_dev *parent, *this = dev;
>> +
>> +	do {
> 
> While I understand the motivation why this function is searching for 
> DPC capable
> parent until the root port, the name of the function does not represent 
> this.
> 

Will change it to more relevant one.

> It might make sense to split this into two where the first function 
> just queries
> the capabilities of the immediate device (the list_for_each_section).
> 
>> +		list_for_each_entry(pdev, &this->service_list, slist) {
>> +			if (pdev->service == port_service)
>> +				return 1;
>> +		}
> 
> Another function to query all parents until you reach the root port.
> 
>> +		parent = pci_upstream_bridge(this);
>> +		this = parent;
>> +	} while (parent && pci_is_pcie(parent));
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(pcie_port_query_service);
>> +

yes that makes sense; will split the function as you are suggesting.

Regards,
Oza.