[PATCH -v2] pciehp: Checking pci conf reading to new added device instead of sleep 1s

[PATCH -v2] pciehp: Checking pci conf reading to new added device instead of sleep 1s

[Yinghai Lu]

During reviewing
|	PCI: pciehp: wait 1000 ms before Link Training check
Linus said:
>...
> That's a *long* time, and it's irritating to the user. It makes the
> user think "the machine is slow".
>...
> And quite frankly, an unconditional one-second delay here seems bad.
>Two seconds was unacceptable, one second is just bad.

Try to access the pci conf of pci device that is supposed to show up in 1s,
if could read back valid vender/device id, could bail out early.

Related discussion could be found:
	https://lkml.org/lkml/2011/12/6/339

-v2: seperate code to pci_bus_read_dev_vendor_id() from pci_scan_device()
    and reuse it from pciehp code. Suggested by Matthew Wilcox.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/hotplug/pciehp_hpc.c |   49 +++++++++++++++++++++++++++------------
 drivers/pci/pci.h                |    2 +
 drivers/pci/probe.c              |   44 +++++++++++++++++++++--------------
 3 files changed, 63 insertions(+), 32 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
@@ -272,6 +272,37 @@ static void pcie_wait_link_active(struct
 	ctrl_dbg(ctrl, "Data Link Layer Link Active not set in 1000 msec\n");
 }
 
+static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
+{
+	u32 l;
+	int count = 0;
+	u32 lx[50];
+	int delay = 1000, step = 20;
+	bool found = false;
+
+	do {
+		found = pci_bus_read_dev_vendor_id(bus, devfn, &l, step);
+		lx[count++] = l;
+
+		if (found)
+			break;
+
+		msleep(step);
+		delay -= step;
+	} while (delay > 0);
+
+	if (count > 1 && pciehp_debug) {
+		int i;
+		printk(KERN_DEBUG "pci %04x:%02x:%02x.%d id reading try %d times with interval %d ms\n",
+			pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), count, step);
+		for (i = 0; i < count; i++)
+			printk(KERN_DEBUG "  [%02d]=%08x\n", i, lx[i]);
+	}
+
+	return found;
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	u16 lnk_status;
@@ -287,13 +318,9 @@ int pciehp_check_link_status(struct cont
         else
                 msleep(1000);
 
-	/*
-	 * Need to wait for 1000 ms after Data Link Layer Link Active
-	 * (DLLLA) bit reads 1b before sending configuration request.
-	 * We need it before checking Link Training (LT) bit becuase
-	 * LT is still set even after DLLLA bit is set on some platform.
-	 */
-	msleep(1000);
+	/* wait 100ms before read pci conf, and try in 1s */
+	msleep(100);
+	pci_bus_check_dev(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0));
 
 	retval = pciehp_readw(ctrl, PCI_EXP_LNKSTA, &lnk_status);
 	if (retval) {
@@ -309,14 +336,6 @@ int pciehp_check_link_status(struct cont
 		return retval;
 	}
 
-	/*
-	 * If the port supports Link speeds greater than 5.0 GT/s, we
-	 * must wait for 100 ms after Link training completes before
-	 * sending configuration request.
-	 */
-	if (ctrl->pcie->port->subordinate->max_bus_speed > PCIE_SPEED_5_0GT)
-		msleep(100);
-
 	pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status);
 
 	return retval;
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -1119,40 +1119,50 @@ struct pci_dev *alloc_pci_dev(void)
 }
 EXPORT_SYMBOL(alloc_pci_dev);
 
-/*
- * Read the config data for a PCI device, sanity-check it
- * and fill in the dev structure...
- */
-static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
+bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
+				 int crs_timeout)
 {
-	struct pci_dev *dev;
-	u32 l;
 	int delay = 1;
 
-	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
-		return NULL;
+	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
+		return false;
 
 	/* some broken boards return 0 or ~0 if a slot is empty: */
-	if (l == 0xffffffff || l == 0x00000000 ||
-	    l == 0x0000ffff || l == 0xffff0000)
-		return NULL;
+	if (*l == 0xffffffff || *l == 0x00000000 ||
+	    *l == 0x0000ffff || *l == 0xffff0000)
+		return false;
 
 	/* Configuration request Retry Status */
-	while (l == 0xffff0001) {
+	while (*l == 0xffff0001) {
 		msleep(delay);
 		delay *= 2;
-		if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
-			return NULL;
+		if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
+			return false;
 		/* Card hasn't responded in 60 seconds?  Must be stuck. */
-		if (delay > 60 * 1000) {
+		if (delay > crs_timeout) {
 			printk(KERN_WARNING "pci %04x:%02x:%02x.%d: not "
 					"responding\n", pci_domain_nr(bus),
 					bus->number, PCI_SLOT(devfn),
 					PCI_FUNC(devfn));
-			return NULL;
+			return false;
 		}
 	}
 
+	return true;
+}
+
+/*
+ * Read the config data for a PCI device, sanity-check it
+ * and fill in the dev structure...
+ */
+static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
+{
+	struct pci_dev *dev;
+	u32 l;
+
+	if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
+		return NULL;
+
 	dev = alloc_pci_dev();
 	if (!dev)
 		return NULL;
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -208,6 +208,8 @@ enum pci_bar_type {
 	pci_bar_mem64,		/* A 64-bit memory BAR */
 };
 
+bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
+                                 int crs_timeout);
 extern int pci_setup_device(struct pci_dev *dev);
 extern int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 				struct resource *res, unsigned int reg);

Re: [PATCH -v2] pciehp: Checking pci conf reading to new added device instead of sleep 1s

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Fri, 16 Dec 2011 16:52:45 -0800
Yinghai Lu <yinghai@kernel.org> wrote:

> During reviewing
> |	PCI: pciehp: wait 1000 ms before Link Training check
> Linus said:
> >...
> > That's a *long* time, and it's irritating to the user. It makes the
> > user think "the machine is slow".
> >...
> > And quite frankly, an unconditional one-second delay here seems bad.
> >Two seconds was unacceptable, one second is just bad.
> 
> Try to access the pci conf of pci device that is supposed to show up in 1s,
> if could read back valid vender/device id, could bail out early.
> 
> Related discussion could be found:
> 	https://lkml.org/lkml/2011/12/6/339
> 
> -v2: seperate code to pci_bus_read_dev_vendor_id() from pci_scan_device()
>     and reuse it from pciehp code. Suggested by Matthew Wilcox.

Kenji-san, you ok with this one?  Any chance you could test?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH -v2] pciehp: Checking pci conf reading to new added device instead of sleep 1s

[Kenji Kaneshige]

(2011/12/17 9:52), Yinghai Lu wrote:
> During reviewing
> |	PCI: pciehp: wait 1000 ms before Link Training check
> Linus said:
>> ...
>> That's a *long* time, and it's irritating to the user. It makes the
>> user think "the machine is slow".
>> ...
>> And quite frankly, an unconditional one-second delay here seems bad.
>> Two seconds was unacceptable, one second is just bad.
>
> Try to access the pci conf of pci device that is supposed to show up in 1s,
> if could read back valid vender/device id, could bail out early.
>
> Related discussion could be found:
> 	https://lkml.org/lkml/2011/12/6/339
>
> -v2: seperate code to pci_bus_read_dev_vendor_id() from pci_scan_device()
>      and reuse it from pciehp code. Suggested by Matthew Wilcox.
>
> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>
> ---
>   drivers/pci/hotplug/pciehp_hpc.c |   49 +++++++++++++++++++++++++++------------
>   drivers/pci/pci.h                |    2 +
>   drivers/pci/probe.c              |   44 +++++++++++++++++++++--------------
>   3 files changed, 63 insertions(+), 32 deletions(-)
>
> Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c
> +++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c
> @@ -272,6 +272,37 @@ static void pcie_wait_link_active(struct
>   	ctrl_dbg(ctrl, "Data Link Layer Link Active not set in 1000 msec\n");
>   }
>
> +static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
> +{
> +	u32 l;
> +	int count = 0;
> +	u32 lx[50];

I don't think it's a good idea to allocate lx[] array on stack.

> +	int delay = 1000, step = 20;
> +	bool found = false;
> +
> +	do {
> +		found = pci_bus_read_dev_vendor_id(bus, devfn,&l, step);
> +		lx[count++] = l;
> +
> +		if (found)
> +			break;
> +
> +		msleep(step);

I don't think msleep() is needed here because pci_bus_read_dev_vendor_id()
has timeout. And even if this msleep() is removed, maximum wait time in
pci_bus_read_dev_vendor_id() looks much longer than 1sec (IIUC, it looks
1ms + 2ms + 4ms + 8ms + ...)


> +		delay -= step;
> +	} while (delay>  0);
> +
> +	if (count>  1&&  pciehp_debug) {
> +		int i;
> +		printk(KERN_DEBUG "pci %04x:%02x:%02x.%d id reading try %d times with interval %d ms\n",
> +			pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
> +			PCI_FUNC(devfn), count, step);
> +		for (i = 0; i<  count; i++)
> +			printk(KERN_DEBUG "  [%02d]=%08x\n", i, lx[i]);
> +	}

Why not printing the debug message each time in the while() loop?


> +
> +	return found;
> +}
> +
>   int pciehp_check_link_status(struct controller *ctrl)
>   {
>   	u16 lnk_status;
> @@ -287,13 +318,9 @@ int pciehp_check_link_status(struct cont
>           else
>                   msleep(1000);
>
> -	/*
> -	 * Need to wait for 1000 ms after Data Link Layer Link Active
> -	 * (DLLLA) bit reads 1b before sending configuration request.
> -	 * We need it before checking Link Training (LT) bit becuase
> -	 * LT is still set even after DLLLA bit is set on some platform.
> -	 */
> -	msleep(1000);
> +	/* wait 100ms before read pci conf, and try in 1s */
> +	msleep(100);
> +	pci_bus_check_dev(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0));

We need to check the result of pci_bus_check_dev() and return error if
the the result is false.

Regards,
Kenji Kaneshige



>
>   	retval = pciehp_readw(ctrl, PCI_EXP_LNKSTA,&lnk_status);
>   	if (retval) {
> @@ -309,14 +336,6 @@ int pciehp_check_link_status(struct cont
>   		return retval;
>   	}
>
> -	/*
> -	 * If the port supports Link speeds greater than 5.0 GT/s, we
> -	 * must wait for 100 ms after Link training completes before
> -	 * sending configuration request.
> -	 */
> -	if (ctrl->pcie->port->subordinate->max_bus_speed>  PCIE_SPEED_5_0GT)
> -		msleep(100);
> -
>   	pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status);
>
>   	return retval;
> Index: linux-2.6/drivers/pci/probe.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/probe.c
> +++ linux-2.6/drivers/pci/probe.c
> @@ -1119,40 +1119,50 @@ struct pci_dev *alloc_pci_dev(void)
>   }
>   EXPORT_SYMBOL(alloc_pci_dev);
>
> -/*
> - * Read the config data for a PCI device, sanity-check it
> - * and fill in the dev structure...
> - */
> -static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
> +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
> +				 int crs_timeout)
>   {
> -	struct pci_dev *dev;
> -	u32 l;
>   	int delay = 1;
>
> -	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID,&l))
> -		return NULL;
> +	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
> +		return false;
>
>   	/* some broken boards return 0 or ~0 if a slot is empty: */
> -	if (l == 0xffffffff || l == 0x00000000 ||
> -	    l == 0x0000ffff || l == 0xffff0000)
> -		return NULL;
> +	if (*l == 0xffffffff || *l == 0x00000000 ||
> +	    *l == 0x0000ffff || *l == 0xffff0000)
> +		return false;
>
>   	/* Configuration request Retry Status */
> -	while (l == 0xffff0001) {
> +	while (*l == 0xffff0001) {
>   		msleep(delay);
>   		delay *= 2;
> -		if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID,&l))
> -			return NULL;
> +		if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
> +			return false;
>   		/* Card hasn't responded in 60 seconds?  Must be stuck. */
> -		if (delay>  60 * 1000) {
> +		if (delay>  crs_timeout) {
>   			printk(KERN_WARNING "pci %04x:%02x:%02x.%d: not "
>   					"responding\n", pci_domain_nr(bus),
>   					bus->number, PCI_SLOT(devfn),
>   					PCI_FUNC(devfn));
> -			return NULL;
> +			return false;
>   		}
>   	}
>
> +	return true;
> +}
> +
> +/*
> + * Read the config data for a PCI device, sanity-check it
> + * and fill in the dev structure...
> + */
> +static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
> +{
> +	struct pci_dev *dev;
> +	u32 l;
> +
> +	if (!pci_bus_read_dev_vendor_id(bus, devfn,&l, 60*1000))
> +		return NULL;
> +
>   	dev = alloc_pci_dev();
>   	if (!dev)
>   		return NULL;
> Index: linux-2.6/drivers/pci/pci.h
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.h
> +++ linux-2.6/drivers/pci/pci.h
> @@ -208,6 +208,8 @@ enum pci_bar_type {
>   	pci_bar_mem64,		/* A 64-bit memory BAR */
>   };
>
> +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
> +                                 int crs_timeout);
>   extern int pci_setup_device(struct pci_dev *dev);
>   extern int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>   				struct resource *res, unsigned int reg);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH -v2] pciehp: Checking pci conf reading to new added device instead of sleep 1s

[Kenji Kaneshige]

(2011/12/17 9:52), Yinghai Lu wrote:

<snip.>
> 
> -/*
> - * Read the config data for a PCI device, sanity-check it
> - * and fill in the dev structure...
> - */
> -static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
> +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
> +				 int crs_timeout)

EXPORT_SYMBOL() for pci_bus_read_dev_vendor_id() is needed to build
pciehp as a kernel module.

Regards,
Kenji Kaneshige

Re: [PATCH -v2] pciehp: Checking pci conf reading to new added device instead of sleep 1s

[Yinghai Lu]

2012/1/19 Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> (2011/12/17 9:52), Yinghai Lu wrote:
>
> <snip.>
>>
>> -/*
>> - * Read the config data for a PCI device, sanity-check it
>> - * and fill in the dev structure...
>> - */
>> -static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
>> +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>> +                              int crs_timeout)
>
> EXPORT_SYMBOL() for pci_bus_read_dev_vendor_id() is needed to build
> pciehp as a kernel module.
>

Thanks for reviewing.

Will update the patch accordingly and separate it to two at least.

Yinghai