diff mbox series

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

Message ID 1510260399-14886-4-git-send-email-poza@codeaurora.org
State Superseded
Headers show
Series PCI: query active service list | expand

Commit Message

Oza Pawandeep Nov. 9, 2017, 8:46 p.m. UTC 
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>

Comments

Sinan Kaya Nov. 13, 2017, 8:52 p.m. UTC | #1 
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);
> +
Oza Pawandeep Nov. 13, 2017, 11:26 p.m. UTC | #2 
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.
diff mbox series

Patch

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_ */