Re: [PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
On Fri, Oct 11, 2019 at 11:48 AM Andy Shevchenko
wrote:
>
> On Fri, Oct 11, 2019 at 9:49 AM Andy Shevchenko
> wrote:
> >
> > On Thu, Oct 10, 2019 at 11:37 PM Stuart Hayes
> > wrote:
> >
> > > Thank you for the feedback! An infinite loop is used several other
> > > places in
> > > this driver--this keeps the style similar. I can change it as you
> > > suggest,
> > > though, if that would be preferable to consistency.
> >
> > Better to start the change now. I'll look into the file and see how we
> > can improve the rest.
>
> I found only one infinite loop there, the other timeout loop is done
> as do {} while.
> I'll send a patch to refactor the infinite one.
https://lore.kernel.org/linux-pci/20191011090230.81080-1-andriy.shevche...@linux.intel.com/T/#u
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
On Fri, Oct 11, 2019 at 9:49 AM Andy Shevchenko
wrote:
>
> On Thu, Oct 10, 2019 at 11:37 PM Stuart Hayes
> wrote:
>
> > Thank you for the feedback! An infinite loop is used several other places
> > in
> > this driver--this keeps the style similar. I can change it as you suggest,
> > though, if that would be preferable to consistency.
>
> Better to start the change now. I'll look into the file and see how we
> can improve the rest.
I found only one infinite loop there, the other timeout loop is done
as do {} while.
I'll send a patch to refactor the infinite one.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
On Thu, Oct 10, 2019 at 11:37 PM Stuart Hayes wrote: > Thank you for the feedback! An infinite loop is used several other places in > this driver--this keeps the style similar. I can change it as you suggest, > though, if that would be preferable to consistency. Better to start the change now. I'll look into the file and see how we can improve the rest. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
On Thu, Oct 10, 2019 at 12:40 AM Andy Shevchenko
wrote:
>
> On Thu, Oct 10, 2019 at 8:37 AM Andy Shevchenko
> wrote:
> > On Wed, Oct 9, 2019 at 11:05 PM Stuart Hayes
> > wrote:
>
> > > +static void pcie_wait_for_presence(struct pci_dev *pdev)
> > > +{
> > > + int timeout = 1250;
>
> > > + bool pds;
>
> Also this is redundant. Just use the following outside the loop
>
> if (!retries)
>pc_info(...);
>
> .
>
> > > + u16 slot_status;
> > > +
> > > + while (true) {
> > > + pcie_capability_read_word(pdev, PCI_EXP_SLTSTA,
> > > _status);
> > > + pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
> > > + if (pds || timeout <= 0)
> > > + break;
> > > + msleep(10);
> > > + timeout -= 10;
> > > + }
> >
> > Can we avoid infinite loops? They are hard to parse (in most cases,
> > and especially when it's a timeout loop)
> >
> > unsigned int retries = 125; // 1250 ms
> >
> > do {
> > ...
> > } while (--retries);
> >
> > > +
> > > + if (!pds)
> > > + pci_info(pdev, "Presence Detect state not set in 1250
> > > msec\n");
> > > +}
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
Thank you for the feedback! An infinite loop is used several other places in
this driver--this keeps the style similar. I can change it as you suggest,
though, if that would be preferable to consistency.
Re: [PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
On Thu, Oct 10, 2019 at 8:37 AM Andy Shevchenko
wrote:
> On Wed, Oct 9, 2019 at 11:05 PM Stuart Hayes wrote:
> > +static void pcie_wait_for_presence(struct pci_dev *pdev)
> > +{
> > + int timeout = 1250;
> > + bool pds;
Also this is redundant. Just use the following outside the loop
if (!retries)
pc_info(...);
.
> > + u16 slot_status;
> > +
> > + while (true) {
> > + pcie_capability_read_word(pdev, PCI_EXP_SLTSTA,
> > _status);
> > + pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
> > + if (pds || timeout <= 0)
> > + break;
> > + msleep(10);
> > + timeout -= 10;
> > + }
>
> Can we avoid infinite loops? They are hard to parse (in most cases,
> and especially when it's a timeout loop)
>
> unsigned int retries = 125; // 1250 ms
>
> do {
> ...
> } while (--retries);
>
> > +
> > + if (!pds)
> > + pci_info(pdev, "Presence Detect state not set in 1250
> > msec\n");
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
On Wed, Oct 9, 2019 at 11:05 PM Stuart Hayes wrote:
>
> From: Alexandru Gagniuc
>
> 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.
> +static void pcie_wait_for_presence(struct pci_dev *pdev)
> +{
> + int timeout = 1250;
> + bool pds;
> + u16 slot_status;
> +
> + while (true) {
> + pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, _status);
> + pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
> + if (pds || timeout <= 0)
> + break;
> + msleep(10);
> + timeout -= 10;
> + }
Can we avoid infinite loops? They are hard to parse (in most cases,
and especially when it's a timeout loop)
unsigned int retries = 125; // 1250 ms
do {
...
} while (--retries);
> +
> + if (!pds)
> + pci_info(pdev, "Presence Detect state not set in 1250
> msec\n");
> +}
--
With Best Regards,
Andy Shevchenko
[PATCH 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
From: Alexandru Gagniuc
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
Signed-off-by: Stuart Hayes
---
drivers/pci/hotplug/pciehp_hpc.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index dc109d521f30..1282641c6458 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -242,6 +242,25 @@ 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;
+ bool pds;
+ u16 slot_status;
+
+ while (true) {
+ pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, _status);
+ pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
+ if (pds || timeout <= 0)
+ break;
+ msleep(10);
+ timeout -= 10;
+ }
+
+ if (!pds)
+ pci_info(pdev, "Presence Detect state not set in 1250 msec\n");
+}
+
int pciehp_check_link_status(struct controller *ctrl)
{
struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -251,6 +270,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 2/3] PCI: pciehp: Wait for PDS if in-band presence is disabled
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: Stuart Hayes
---
drivers/pci/hotplug/pciehp_hpc.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index dc109d521f30..1282641c6458 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -242,6 +242,25 @@ 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;
+ bool pds;
+ u16 slot_status;
+
+ while (true) {
+ pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, _status);
+ pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
+ if (pds || timeout <= 0)
+ break;
+ msleep(10);
+ timeout -= 10;
+ }
+
+ if (!pds)
+ pci_info(pdev, "Presence Detect state not set in 1250 msec\n");
+}
+
int pciehp_check_link_status(struct controller *ctrl)
{
struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -251,6 +270,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
