| Message ID | 1516360204-1586-8-git-send-email-poza@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Address error and recovery for AER and DPC | expand |
On 1/19/2018 6:10 AM, Oza Pawandeep wrote: > +check_link: > + pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status); > + ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA); > + > + if (ret == active) { > + dev_printk(KERN_DEBUG, &pdev->dev, > + "%s: lnk_status = %x\n", __func__, lnk_status); > + return true; > + } > + > + while (timeout > 0) { > + msleep(10); > + timeout -= 10; > + goto check_link; > + } this is weird. I think following is a simpler approach. 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); }
On 1/19/2018 6:10 AM, Oza Pawandeep wrote: > +++ b/drivers/pci/pcie/pcie-dpc.c > @@ -150,37 +150,9 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc) > struct pci_dev *pdev = dpc->dev->port; > struct device *dev = &dpc->dev->device; > u16 lnk_status; > + bool active = false; > > - 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"); > -} I think you should reorder 6 and 7 to eliminate this.
On 19.01.2018 12:10, Oza Pawandeep wrote: > 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 <poza@codeaurora.org> > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 7bab060..26afeff 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -245,25 +245,12 @@ 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); > + bool active = true; > > -static void pcie_wait_link_active(struct controller *ctrl) > -{ > - __pcie_wait_link_active(ctrl, true); > + return pci_wait_for_link(pdev, active); > } > > 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 4a7c686..0de83ea 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2805,7 +2805,7 @@ static void pci_std_enable_acs(struct pci_dev *dev) > pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl); > > /* Source Validation */ > - ctrl |= (cap & PCI_ACS_SV); > +// ctrl |= (cap & PCI_ACS_SV); Could it be, that you missed to fix / clean something up here? Thanks, Stefan
On 2018-01-19 19:49, Stefan Roese wrote: > On 19.01.2018 12:10, Oza Pawandeep wrote: >> 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 <poza@codeaurora.org> >> >> diff --git a/drivers/pci/hotplug/pciehp_hpc.c >> b/drivers/pci/hotplug/pciehp_hpc.c >> index 7bab060..26afeff 100644 >> --- a/drivers/pci/hotplug/pciehp_hpc.c >> +++ b/drivers/pci/hotplug/pciehp_hpc.c >> @@ -245,25 +245,12 @@ 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); >> + bool active = true; >> >> -static void pcie_wait_link_active(struct controller *ctrl) >> -{ >> - __pcie_wait_link_active(ctrl, true); >> + return pci_wait_for_link(pdev, active); >> } >> >> 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 4a7c686..0de83ea 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -2805,7 +2805,7 @@ static void pci_std_enable_acs(struct pci_dev >> *dev) >> pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl); >> >> /* Source Validation */ >> - ctrl |= (cap & PCI_ACS_SV); >> +// ctrl |= (cap & PCI_ACS_SV); > > Could it be, that you missed to fix / clean something up here? > > Thanks, > Stefan yes, will correct this and re-post. Regards, Oza.
On 2018-01-19 19:47, Sinan Kaya wrote: > On 1/19/2018 6:10 AM, Oza Pawandeep wrote: >> +++ b/drivers/pci/pcie/pcie-dpc.c >> @@ -150,37 +150,9 @@ static void dpc_wait_link_inactive(struct dpc_dev >> *dpc) >> struct pci_dev *pdev = dpc->dev->port; >> struct device *dev = &dpc->dev->device; >> u16 lnk_status; >> + bool active = false; >> >> - 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"); >> -} > > I think you should reorder 6 and 7 to eliminate this. Sure will reorder them Regards, Oza.
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 7bab060..26afeff 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -245,25 +245,12 @@ 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); + bool active = true; -static void pcie_wait_link_active(struct controller *ctrl) -{ - __pcie_wait_link_active(ctrl, true); + return pci_wait_for_link(pdev, active); } 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 4a7c686..0de83ea 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2805,7 +2805,7 @@ static void pci_std_enable_acs(struct pci_dev *dev) pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl); /* Source Validation */ - ctrl |= (cap & PCI_ACS_SV); +// ctrl |= (cap & PCI_ACS_SV); /* P2P Request Redirect */ ctrl |= (cap & PCI_ACS_RR); @@ -4079,6 +4079,43 @@ static int pci_pm_reset(struct pci_dev *dev, int probe) return 0; } +/** + * pci__wait_for_link - Wait for link till its active/inactive + * @dev: 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; + +check_link: + pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status); + ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA); + + if (ret == active) { + dev_printk(KERN_DEBUG, &pdev->dev, + "%s: lnk_status = %x\n", __func__, lnk_status); + return true; + } + + while (timeout > 0) { + msleep(10); + timeout -= 10; + goto check_link; + } + 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 1b06a8e..67502a5 100644 --- a/drivers/pci/pcie/pcie-dpc.c +++ b/drivers/pci/pcie/pcie-dpc.c @@ -150,37 +150,9 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc) struct pci_dev *pdev = dpc->dev->port; struct device *dev = &dpc->dev->device; u16 lnk_status; + bool active = false; - 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"); -} - -static bool dpc_wait_link_active(struct pci_dev *pdev) -{ - unsigned long timeout = jiffies + HZ; - u16 lnk_status; - bool ret = true; - - 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(&pdev->dev, "Link state not enabled after DPC event\n"); - ret = false; - } - - return ret; + pci_wait_for_link(pdev, active); } /** @@ -191,7 +163,9 @@ static bool dpc_wait_link_active(struct pci_dev *pdev) */ static void dpc_error_resume(struct pci_dev *pdev) { - if (dpc_wait_link_active(pdev)) { + bool active = true; + + if (pci_wait_for_link(pdev, active)) { pci_lock_rescan_remove(); pci_rescan_bus(pdev->bus); pci_unlock_rescan_remove(); diff --git a/include/linux/pci.h b/include/linux/pci.h index c170c92..f9f6a3d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1201,6 +1201,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);
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 <poza@codeaurora.org>