[v3,2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
diff mbox series

Message ID 20191017193256.3636-3-stuart.w.hayes@gmail.com
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series
  • PCI: pciehp: Do not turn off slot if presence comes up after link
Related show

Commit Message

Stuart Hayes Oct. 17, 2019, 7:32 p.m. UTC
From: Alexandru Gagniuc <mr.nuke.me@gmail.com>

When inband presence is disabled, PDS may come up at any time, or not
at all. PDS being low may indicate that the card is still mating, and
we could expect contact bounce to bring down the link as well.

It is reasonable to assume that most cards will mate in a hotplug slot
in about a second. Thus, when we know PDS only reflects out-of-band
presence, it's worthwhile to wait the extra second or so to make sure
the card is properly mated before loading the driver, and to prevent
the hotplug code from disabling a device if the presence detect change
goes active after the device is enabled.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
v2:
  replace while(true) loop with do...while
v3
  remove unused variable declaration (pds)
  modify text of warning message

 drivers/pci/hotplug/pciehp_hpc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Mika Westerberg Oct. 21, 2019, 1:41 p.m. UTC | #1
On Thu, Oct 17, 2019 at 03:32:55PM -0400, Stuart Hayes wrote:
> From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> 
> When inband presence is disabled, PDS may come up at any time, or not
> at all. PDS being low may indicate that the card is still mating, and
> we could expect contact bounce to bring down the link as well.
> 
> It is reasonable to assume that most cards will mate in a hotplug slot
> in about a second. Thus, when we know PDS only reflects out-of-band
> presence, it's worthwhile to wait the extra second or so to make sure
> the card is properly mated before loading the driver, and to prevent
> the hotplug code from disabling a device if the presence detect change
> goes active after the device is enabled.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

One nit below.

> ---
> v2:
>   replace while(true) loop with do...while
> v3
>   remove unused variable declaration (pds)
>   modify text of warning message
> 
>  drivers/pci/hotplug/pciehp_hpc.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index dc109d521f30..02eb811a014f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -242,6 +242,22 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>  	return found;
>  }
>  
> +static void pcie_wait_for_presence(struct pci_dev *pdev)
> +{
> +	int timeout = 1250;
> +	u16 slot_status;
> +
> +	do {
> +		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
> +		if (!!(slot_status & PCI_EXP_SLTSTA_PDS))

It is more readable if you write it like:

		if (slot_status & PCI_EXP_SLTSTA_PDS)

> +			return;
> +		msleep(10);
> +		timeout -= 10;
> +	} while (timeout > 0);
> +
> +	pci_info(pdev, "Timeout waiting for Presence Detect state to be set\n");
> +}
> +
>  int pciehp_check_link_status(struct controller *ctrl)
>  {
>  	struct pci_dev *pdev = ctrl_dev(ctrl);
> @@ -251,6 +267,9 @@ int pciehp_check_link_status(struct controller *ctrl)
>  	if (!pcie_wait_for_link(pdev, true))
>  		return -1;
>  
> +	if (ctrl->inband_presence_disabled)
> +		pcie_wait_for_presence(pdev);
> +
>  	found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
>  					PCI_DEVFN(0, 0));
>  
> -- 
> 2.18.1
Stuart Hayes Oct. 21, 2019, 6:08 p.m. UTC | #2
On 10/21/19 8:41 AM, Mika Westerberg wrote:
> On Thu, Oct 17, 2019 at 03:32:55PM -0400, Stuart Hayes wrote:
>> From: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>
>> When inband presence is disabled, PDS may come up at any time, or not
>> at all. PDS being low may indicate that the card is still mating, and
>> we could expect contact bounce to bring down the link as well.
>>
>> It is reasonable to assume that most cards will mate in a hotplug slot
>> in about a second. Thus, when we know PDS only reflects out-of-band
>> presence, it's worthwhile to wait the extra second or so to make sure
>> the card is properly mated before loading the driver, and to prevent
>> the hotplug code from disabling a device if the presence detect change
>> goes active after the device is enabled.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> One nit below.
> 
>> ---
>> v2:
>>   replace while(true) loop with do...while
>> v3
>>   remove unused variable declaration (pds)
>>   modify text of warning message
>>
>>  drivers/pci/hotplug/pciehp_hpc.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index dc109d521f30..02eb811a014f 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -242,6 +242,22 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>>  	return found;
>>  }
>>  
>> +static void pcie_wait_for_presence(struct pci_dev *pdev)
>> +{
>> +	int timeout = 1250;
>> +	u16 slot_status;
>> +
>> +	do {
>> +		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
>> +		if (!!(slot_status & PCI_EXP_SLTSTA_PDS))
> 
> It is more readable if you write it like:
> 
> 		if (slot_status & PCI_EXP_SLTSTA_PDS)
> 

I agree, it is more readable, and the double bang shouldn't be needed for an 
"if" condition.  Thanks.

>> +			return;
>> +		msleep(10);
>> +		timeout -= 10;
>> +	} while (timeout > 0);
>> +
>> +	pci_info(pdev, "Timeout waiting for Presence Detect state to be set\n");
>> +}
>> +
>>  int pciehp_check_link_status(struct controller *ctrl)
>>  {
>>  	struct pci_dev *pdev = ctrl_dev(ctrl);
>> @@ -251,6 +267,9 @@ int pciehp_check_link_status(struct controller *ctrl)
>>  	if (!pcie_wait_for_link(pdev, true))
>>  		return -1;
>>  
>> +	if (ctrl->inband_presence_disabled)
>> +		pcie_wait_for_presence(pdev);
>> +
>>  	found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
>>  					PCI_DEVFN(0, 0));
>>  
>> -- 
>> 2.18.1

Patch
diff mbox series

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index dc109d521f30..02eb811a014f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -242,6 +242,22 @@  static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
 	return found;
 }
 
+static void pcie_wait_for_presence(struct pci_dev *pdev)
+{
+	int timeout = 1250;
+	u16 slot_status;
+
+	do {
+		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+		if (!!(slot_status & PCI_EXP_SLTSTA_PDS))
+			return;
+		msleep(10);
+		timeout -= 10;
+	} while (timeout > 0);
+
+	pci_info(pdev, "Timeout waiting for Presence Detect state to be set\n");
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -251,6 +267,9 @@  int pciehp_check_link_status(struct controller *ctrl)
 	if (!pcie_wait_for_link(pdev, true))
 		return -1;
 
+	if (ctrl->inband_presence_disabled)
+		pcie_wait_for_presence(pdev);
+
 	found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
 					PCI_DEVFN(0, 0));