[v10,6/7] PCI: Unify wait for link active into generic pci

Commit Message

Oza Pawandeep Feb. 22, 2018, 4:02 p.m. UTC

Clients such as pciehp, dpc are using pcie_wait_link_active, which waits
till the link becomes active or inactive.

Made generic function and moved it to drivers/pci/pci.c

Signed-off-by: Oza Pawandeep <[e-mail]>

Comments

Christoph Hellwig Feb. 22, 2018, 6:43 p.m. UTC | #1

>  
> +/**
> + * pci__wait_for_link - Wait for link till its active/inactive

typo - just wants a single underscore.

> +	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> +	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);

no need for the !! when assigning to a boolean.

> +
> +	while ((ret != active) && (timeout > 0)) {

No need for either pair of inner braces.

> +		msleep(10);
> +		timeout -= 10;
> +		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> +		ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);

Same as above.

> +	}
> +
> +	if (ret == active)
> +		return true;

Seems like the structure is a bit odd.  Why not:

	for (;;) {
		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
		if ((lnk_status & PCI_EXP_LNKSTA_DLLLA) == active)
			return true;
		if (timeout <= 0)
			break;
		timeout -= 10;
	}

Oza Pawandeep Feb. 23, 2018, 6:53 a.m. UTC | #2

On 2018-02-23 00:13, Christoph Hellwig wrote:
>> 
>> +/**
>> + * pci__wait_for_link - Wait for link till its active/inactive
> 
> typo - just wants a single underscore.
> 
>> +	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>> +	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
> 
> no need for the !! when assigning to a boolean.

bool is 8 bit, while
#define  PCI_EXP_LNKSTA_DLLLA	0x2000	/* Data Link Layer Link Active */
so I think we need it. otherwise it will treat lnk_status & 
PCI_EXP_LNKSTA_DLLLA as 0 even if 13th bit is set.

> 
>> +
>> +	while ((ret != active) && (timeout > 0)) {
> 
> No need for either pair of inner braces.
> 
>> +		msleep(10);
>> +		timeout -= 10;
>> +		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>> +		ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
> 
> Same as above.
> 
>> +	}
>> +
>> +	if (ret == active)
>> +		return true;
> 
> Seems like the structure is a bit odd.  Why not:
> 
> 	for (;;) {
> 		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> 		if ((lnk_status & PCI_EXP_LNKSTA_DLLLA) == active)
> 			return true;
> 		if (timeout <= 0)
> 			break;
> 		timeout -= 10;
> 	}

Thanks for suggestion, will do this way. thanks.
Regards,
Oza.

Patch

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 18a42f8..de9b0ea 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -231,25 +231,11 @@  bool pciehp_check_link_active(struct controller *ctrl)
 	return ret;
 }
 
-static void __pcie_wait_link_active(struct controller *ctrl, bool active)
+static bool pcie_wait_link_active(struct controller *ctrl)
 {
-	int timeout = 1000;
-
-	if (pciehp_check_link_active(ctrl) == active)
-		return;
-	while (timeout > 0) {
-		msleep(10);
-		timeout -= 10;
-		if (pciehp_check_link_active(ctrl) == active)
-			return;
-	}
-	ctrl_dbg(ctrl, "Data Link Layer Link Active not %s in 1000 msec\n",
-			active ? "set" : "cleared");
-}
+	struct pci_dev *pdev = ctrl_dev(ctrl);
 
-static void pcie_wait_link_active(struct controller *ctrl)
-{
-	__pcie_wait_link_active(ctrl, true);
+	return pci_wait_for_link(pdev, true);
 }
 
 static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd1..5440696 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4176,6 +4176,40 @@  static int pci_pm_reset(struct pci_dev *dev, int probe)
 	return 0;
 }
 
+/**
+ * pci__wait_for_link - Wait for link till its active/inactive
+ * @pdev: Bridge device
+ * @active: waiting for active or inactive ?
+ *
+ * Use this to wait till link becomes active or inactive.
+ */
+bool pci_wait_for_link(struct pci_dev *pdev, bool active)
+{
+	int timeout = 1000;
+	bool ret;
+	u16 lnk_status;
+
+	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
+	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+
+	while ((ret != active) && (timeout > 0)) {
+		msleep(10);
+		timeout -= 10;
+		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
+		ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+	}
+
+	if (ret == active)
+		return true;
+
+	dev_printk(KERN_DEBUG, &pdev->dev,
+		   "Data Link Layer Link Active not %s in 1000 msec\n",
+		   active ? "set" : "cleared");
+
+	return false;
+}
+EXPORT_SYMBOL(pci_wait_for_link);
+
 void pci_reset_secondary_bus(struct pci_dev *dev)
 {
 	u16 ctrl;
diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 208b427..0524bd3 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -122,19 +122,9 @@  static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 
 static void dpc_wait_link_inactive(struct dpc_dev *dpc)
 {
-	unsigned long timeout = jiffies + HZ;
 	struct pci_dev *pdev = dpc->dev->port;
-	struct device *dev = &dpc->dev->device;
-	u16 lnk_status;
 
-	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
-	while (lnk_status & PCI_EXP_LNKSTA_DLLLA &&
-					!time_after(jiffies, timeout)) {
-		msleep(10);
-		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
-	}
-	if (lnk_status & PCI_EXP_LNKSTA_DLLLA)
-		dev_warn(dev, "Link state not disabled for DPC event\n");
+	pci_wait_for_link(pdev, false);
 }
 
 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 024a1be..cb674c3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1195,6 +1195,7 @@  int pci_add_ext_cap_save_buffer(struct pci_dev *dev,
 int pci_request_selected_regions(struct pci_dev *, int, const char *);
 int pci_request_selected_regions_exclusive(struct pci_dev *, int, const char *);
 void pci_release_selected_regions(struct pci_dev *, int);
+bool pci_wait_for_link(struct pci_dev *pdev, bool active);
 
 /* drivers/pci/bus.c */
 struct pci_bus *pci_bus_get(struct pci_bus *bus);