| Message ID | 20191017193256.3636-3-stuart.w.hayes@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PCI: pciehp: Do not turn off slot if presence comes up after link | expand |
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
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
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));