* [PATCH V8 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() [not found] <1491627351-1111-1-git-send-email-okaya@codeaurora.org> @ 2017-04-08 4:55 ` Sinan Kaya 2017-04-13 20:51 ` Bjorn Helgaas 2017-04-08 4:55 ` [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two Sinan Kaya ` (3 subsequent siblings) 4 siblings, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-08 4:55 UTC (permalink / raw) To: linux-pci, timur Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya, Bjorn Helgaas, Rajat Jain, Julia Lawall, David Daney, Shawn Lin, linux-kernel We need a callback from pci_init_capabilities function for every single new PCI device that is currently being added. pci_aspm_init() will be used to save the power on state of the HW. Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/pci/pcie/aspm.c | 10 ++++++++++ drivers/pci/probe.c | 3 +++ include/linux/pci.h | 2 ++ 3 files changed, 15 insertions(+) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 1dfa10c..dc36717 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -827,6 +827,16 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) } /* + * pci_aspm_init: Initiate PCI express link state. + * It is called from device_add for every single pci device. + * @pdev: all pci devices + */ +int pci_aspm_init(struct pci_dev *pdev) +{ + return 0; +} + +/* * pcie_aspm_init_link_state: Initiate PCI express link state. * It is called after the pcie and its children devices are scanned. * @pdev: the root port or switch downstream port diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index dfc9a27..1e19364 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1847,6 +1847,9 @@ static void pci_init_capabilities(struct pci_dev *dev) /* Advanced Error Reporting */ pci_aer_init(dev); + + /* Active State Power Management */ + pci_aspm_init(dev); } /* diff --git a/include/linux/pci.h b/include/linux/pci.h index eb3da1a..8828dd7 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1396,8 +1396,10 @@ static inline int pci_irq_get_node(struct pci_dev *pdev, int vec) #ifdef CONFIG_PCIEASPM bool pcie_aspm_support_enabled(void); +int pci_aspm_init(struct pci_dev *pdev); #else static inline bool pcie_aspm_support_enabled(void) { return false; } +static inline int pci_aspm_init(struct pci_dev *pdev) { return -ENODEV; } #endif #ifdef CONFIG_PCIEAER -- 1.9.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V8 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() 2017-04-08 4:55 ` [PATCH V8 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() Sinan Kaya @ 2017-04-13 20:51 ` Bjorn Helgaas 2017-04-14 19:10 ` Sinan Kaya 0 siblings, 1 reply; 27+ messages in thread From: Bjorn Helgaas @ 2017-04-13 20:51 UTC (permalink / raw) To: Sinan Kaya Cc: linux-pci, timur, mayurkumar.patel, David Daney, linux-arm-msm, Shawn Lin, linux-kernel, Julia Lawall, Bjorn Helgaas, Rajat Jain, linux-arm-kernel Hi Sinan, On Sat, Apr 08, 2017 at 12:55:47AM -0400, Sinan Kaya wrote: > We need a callback from pci_init_capabilities function for every > single new PCI device that is currently being added. > > pci_aspm_init() will be used to save the power on state of the HW. > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/pci/pcie/aspm.c | 10 ++++++++++ > drivers/pci/probe.c | 3 +++ > include/linux/pci.h | 2 ++ > 3 files changed, 15 insertions(+) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 1dfa10c..dc36717 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -827,6 +827,16 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > } > > /* > + * pci_aspm_init: Initiate PCI express link state. > + * It is called from device_add for every single pci device. > + * @pdev: all pci devices > + */ > +int pci_aspm_init(struct pci_dev *pdev) > +{ > + return 0; > +} > + > +/* > * pcie_aspm_init_link_state: Initiate PCI express link state. > * It is called after the pcie and its children devices are scanned. > * @pdev: the root port or switch downstream port > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index dfc9a27..1e19364 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1847,6 +1847,9 @@ static void pci_init_capabilities(struct pci_dev *dev) > > /* Advanced Error Reporting */ > pci_aer_init(dev); > + > + /* Active State Power Management */ > + pci_aspm_init(dev); > } > > /* > diff --git a/include/linux/pci.h b/include/linux/pci.h > index eb3da1a..8828dd7 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1396,8 +1396,10 @@ static inline int pci_irq_get_node(struct pci_dev *pdev, int vec) > > #ifdef CONFIG_PCIEASPM > bool pcie_aspm_support_enabled(void); > +int pci_aspm_init(struct pci_dev *pdev); I think these can go in drivers/pci/pci.h instead of the public include/linux/pci.h. There's no ASPM section in drivers/pci/pci.h yet, but I think we should add one. It looks like the following things from include/linux/pci-aspm.h could be moved there as well, since they're only used by the PCI core: void pcie_aspm_init_link_state(struct pci_dev *pdev); void pcie_aspm_exit_link_state(struct pci_dev *pdev); void pcie_aspm_pm_state_change(struct pci_dev *pdev); void pcie_aspm_powersave_config_link(struct pci_dev *pdev); void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev); void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev); This would be a separate patch. > #else > static inline bool pcie_aspm_support_enabled(void) { return false; } > +static inline int pci_aspm_init(struct pci_dev *pdev) { return -ENODEV; } > #endif > > #ifdef CONFIG_PCIEAER > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() 2017-04-13 20:51 ` Bjorn Helgaas @ 2017-04-14 19:10 ` Sinan Kaya 0 siblings, 0 replies; 27+ messages in thread From: Sinan Kaya @ 2017-04-14 19:10 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, timur, mayurkumar.patel, David Daney, linux-arm-msm, Shawn Lin, linux-kernel, Julia Lawall, Bjorn Helgaas, Rajat Jain, linux-arm-kernel On 4/13/2017 4:51 PM, Bjorn Helgaas wrote: > I think these can go in drivers/pci/pci.h instead of the public > include/linux/pci.h. There's no ASPM section in drivers/pci/pci.h > yet, but I think we should add one. > > It looks like the following things from include/linux/pci-aspm.h could > be moved there as well, since they're only used by the PCI core: > > void pcie_aspm_init_link_state(struct pci_dev *pdev); > void pcie_aspm_exit_link_state(struct pci_dev *pdev); > void pcie_aspm_pm_state_change(struct pci_dev *pdev); > void pcie_aspm_powersave_config_link(struct pci_dev *pdev); > void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev); > void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev); > > This would be a separate patch. > Sure, let me work on this. >> #else >> static inline bool pcie_aspm_support_enabled(void) { return false; } >> +static inline int pci_aspm_init(struct pci_dev *pdev) { return -ENODEV; } >> #endif -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two [not found] <1491627351-1111-1-git-send-email-okaya@codeaurora.org> 2017-04-08 4:55 ` [PATCH V8 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() Sinan Kaya @ 2017-04-08 4:55 ` Sinan Kaya 2017-04-12 19:16 ` Rajat Jain 2017-04-08 4:55 ` [PATCH V8 3/5] PCI/ASPM: add init hook to device_add Sinan Kaya ` (2 subsequent siblings) 4 siblings, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-08 4:55 UTC (permalink / raw) To: linux-pci, timur Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya, Bjorn Helgaas, Rajat Jain, Shawn Lin, Julia Lawall, David Daney, linux-kernel Split pci_aspm_init() body into pci_aspm_init_upstream() and pci_aspm_init_downstream() for bridge and endpoint specific code behavior. Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/pci/pcie/aspm.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index dc36717..a80d64b 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -826,6 +826,16 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) return link; } +static int pci_aspm_init_downstream(struct pci_dev *pdev) +{ + return 0; +} + +static int pci_aspm_init_upstream(struct pci_dev *pdev) +{ + return 0; +} + /* * pci_aspm_init: Initiate PCI express link state. * It is called from device_add for every single pci device. @@ -833,7 +843,10 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) */ int pci_aspm_init(struct pci_dev *pdev) { - return 0; + if (!pdev->has_secondary_link) + return pci_aspm_init_downstream(pdev); + + return pci_aspm_init_upstream(pdev); } /* -- 1.9.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two 2017-04-08 4:55 ` [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two Sinan Kaya @ 2017-04-12 19:16 ` Rajat Jain 2017-04-13 18:25 ` Bjorn Helgaas 0 siblings, 1 reply; 27+ messages in thread From: Rajat Jain @ 2017-04-12 19:16 UTC (permalink / raw) To: Sinan Kaya Cc: linux-pci, timur, Patel, Mayurkumar, linux-arm-msm, linux-arm-kernel, Bjorn Helgaas, Shawn Lin, Julia Lawall, David Daney, linux-kernel, Rajat Jain On Fri, Apr 7, 2017 at 9:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > Split pci_aspm_init() body into pci_aspm_init_upstream() > and pci_aspm_init_downstream() for bridge and endpoint > specific code behavior. > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/pci/pcie/aspm.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index dc36717..a80d64b 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -826,6 +826,16 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > return link; > } > > +static int pci_aspm_init_downstream(struct pci_dev *pdev) > +{ > + return 0; > +} > + > +static int pci_aspm_init_upstream(struct pci_dev *pdev) > +{ > + return 0; > +} > + > /* > * pci_aspm_init: Initiate PCI express link state. > * It is called from device_add for every single pci device. > @@ -833,7 +843,10 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > */ > int pci_aspm_init(struct pci_dev *pdev) > { > - return 0; > + if (!pdev->has_secondary_link) > + return pci_aspm_init_downstream(pdev); > + > + return pci_aspm_init_upstream(pdev); > } Nit: if (x_flag()) return x(); return y(); May be better than if (!x_flag) return y(); return x(); > > /* > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two 2017-04-12 19:16 ` Rajat Jain @ 2017-04-13 18:25 ` Bjorn Helgaas 2017-04-14 19:10 ` Sinan Kaya 0 siblings, 1 reply; 27+ messages in thread From: Bjorn Helgaas @ 2017-04-13 18:25 UTC (permalink / raw) To: Rajat Jain Cc: Sinan Kaya, linux-pci, Timur Tabi, Patel, Mayurkumar, linux-arm-msm, linux-arm, Shawn Lin, Julia Lawall, David Daney, linux-kernel, Rajat Jain On Wed, Apr 12, 2017 at 2:16 PM, Rajat Jain <rajatja@google.com> wrote: > On Fri, Apr 7, 2017 at 9:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> Split pci_aspm_init() body into pci_aspm_init_upstream() >> and pci_aspm_init_downstream() for bridge and endpoint >> specific code behavior. >> >> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> --- >> drivers/pci/pcie/aspm.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c >> index dc36717..a80d64b 100644 >> --- a/drivers/pci/pcie/aspm.c >> +++ b/drivers/pci/pcie/aspm.c >> @@ -826,6 +826,16 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) >> return link; >> } >> >> +static int pci_aspm_init_downstream(struct pci_dev *pdev) >> +{ >> + return 0; >> +} >> + >> +static int pci_aspm_init_upstream(struct pci_dev *pdev) >> +{ >> + return 0; >> +} >> + >> /* >> * pci_aspm_init: Initiate PCI express link state. >> * It is called from device_add for every single pci device. >> @@ -833,7 +843,10 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) >> */ >> int pci_aspm_init(struct pci_dev *pdev) >> { >> - return 0; >> + if (!pdev->has_secondary_link) >> + return pci_aspm_init_downstream(pdev); >> + >> + return pci_aspm_init_upstream(pdev); >> } > > Nit: > > if (x_flag()) > return x(); > return y(); > > May be better than > > if (!x_flag) > return y(); > return x(); I agree, and I made that change on my branch. I also renamed these to pci_aspm_init_downstream_port() (for Root Ports and Switch Downstream Ports) and pci_aspm_init_upstream_port() (for Switch Upstream Ports and Endpoints) to try to match the terminology in the spec. FWIW, your email didn't seem to make it to the list, Rajat. Maybe some gmail weirdness? I don't *see* anything wrong, but ... I'm responding via gmail, so my response probably won't make it to the list either. Bjorn ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two 2017-04-13 18:25 ` Bjorn Helgaas @ 2017-04-14 19:10 ` Sinan Kaya 0 siblings, 0 replies; 27+ messages in thread From: Sinan Kaya @ 2017-04-14 19:10 UTC (permalink / raw) To: Bjorn Helgaas, Rajat Jain Cc: linux-pci, Timur Tabi, Patel, Mayurkumar, linux-arm-msm, linux-arm, Shawn Lin, Julia Lawall, David Daney, linux-kernel, Rajat Jain On 4/13/2017 2:25 PM, Bjorn Helgaas wrote: > I agree, and I made that change on my branch. I also renamed these to > pci_aspm_init_downstream_port() (for Root Ports and Switch Downstream > Ports) and pci_aspm_init_upstream_port() (for Switch Upstream Ports > and Endpoints) to try to match the terminology in the spec. OK. I reordered and renamed the functions according to your suggestions on my local copy too. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V8 3/5] PCI/ASPM: add init hook to device_add [not found] <1491627351-1111-1-git-send-email-okaya@codeaurora.org> 2017-04-08 4:55 ` [PATCH V8 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() Sinan Kaya 2017-04-08 4:55 ` [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two Sinan Kaya @ 2017-04-08 4:55 ` Sinan Kaya 2017-04-13 20:48 ` Bjorn Helgaas 2017-04-08 4:55 ` [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Sinan Kaya 2017-04-08 4:55 ` [PATCH V8 5/5] PCI/ASPM: move link_state cleanup to bridge remove Sinan Kaya 4 siblings, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-08 4:55 UTC (permalink / raw) To: linux-pci, timur Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya, Bjorn Helgaas, Rajat Jain, David Daney, Julia Lawall, linux-kernel For bridges, have pcie_aspm_init_link_state() allocate a link_state, regardless of whether it currently has any children. pcie_aspm_init_link_state(): Called for bridges (upstream end of link) after all children have been enumerated. No longer needs to check aspm_support_enabled or pdev->has_secondary_link or the VIA quirk: pci_aspm_init() already checked that stuff, so we only need to check pdev->link_state here. Now that we are calling alloc_pcie_link_state() from pci_aspm_init() , get rid of pci_function_0 function and detect downstream link in pci_aspm_init_upstream() instead when the function number is 0. Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/pci/pcie/aspm.c | 72 ++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index a80d64b..e33f84b 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) } } -/* - * The L1 PM substate capability is only implemented in function 0 in a - * multi function device. - */ -static struct pci_dev *pci_function_0(struct pci_bus *linkbus) -{ - struct pci_dev *child; - - list_for_each_entry(child, &linkbus->devices, bus_list) - if (PCI_FUNC(child->devfn) == 0) - return child; - return NULL; -} - /* Calculate L1.2 PM substate timing parameters */ static void aspm_calc_l1ss_info(struct pcie_link_state *link, struct aspm_register_info *upreg, @@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) INIT_LIST_HEAD(&link->children); INIT_LIST_HEAD(&link->link); link->pdev = pdev; - link->downstream = pci_function_0(pdev->subordinate); /* * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe @@ -828,11 +813,33 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) static int pci_aspm_init_downstream(struct pci_dev *pdev) { + struct pcie_link_state *link; + struct pci_dev *parent; + + /* + * The L1 PM substate capability is only implemented in function 0 in a + * multi function device. + */ + if (PCI_FUNC(pdev->devfn) != 0) + return -EINVAL; + + parent = pdev->bus->self; + if (!parent) + return -EINVAL; + + link = parent->link_state; + link->downstream = pdev; return 0; } static int pci_aspm_init_upstream(struct pci_dev *pdev) { + struct pcie_link_state *link; + + link = alloc_pcie_link_state(pdev); + if (!link) + return -ENOMEM; + return 0; } @@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev) */ int pci_aspm_init(struct pci_dev *pdev) { + if (!aspm_support_enabled) + return 0; + + if (!pci_is_pcie(pdev)) + return -EINVAL; + + /* VIA has a strange chipset, root port is under a bridge */ + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT && + pdev->bus->self) + return 0; + if (!pdev->has_secondary_link) return pci_aspm_init_downstream(pdev); @@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) struct pcie_link_state *link; int blacklist = !!pcie_aspm_sanity_check(pdev); - if (!aspm_support_enabled) - return; - - if (pdev->link_state) - return; - - /* - * We allocate pcie_link_state for the component on the upstream - * end of a Link, so there's nothing to do unless this device has a - * Link on its secondary side. - */ - if (!pdev->has_secondary_link) - return; - - /* VIA has a strange chipset, root port is under a bridge */ - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT && - pdev->bus->self) + if (!pdev->link_state) return; + link = pdev->link_state; down_read(&pci_bus_sem); if (list_empty(&pdev->subordinate->devices)) goto out; mutex_lock(&aspm_lock); - link = alloc_pcie_link_state(pdev); - if (!link) - goto unlock; + /* * Setup initial ASPM state. Note that we need to configure * upstream links also because capable state of them can be @@ -910,7 +911,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) pcie_set_clkpm(link, policy_to_clkpm_state(link)); } -unlock: mutex_unlock(&aspm_lock); out: up_read(&pci_bus_sem); -- 1.9.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add 2017-04-08 4:55 ` [PATCH V8 3/5] PCI/ASPM: add init hook to device_add Sinan Kaya @ 2017-04-13 20:48 ` Bjorn Helgaas 2017-04-13 21:02 ` Bjorn Helgaas 0 siblings, 1 reply; 27+ messages in thread From: Bjorn Helgaas @ 2017-04-13 20:48 UTC (permalink / raw) To: Sinan Kaya Cc: linux-pci, timur, mayurkumar.patel, David Daney, linux-arm-msm, linux-kernel, Julia Lawall, Bjorn Helgaas, Rajat Jain, linux-arm-kernel Hi Sinan, On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote: > For bridges, have pcie_aspm_init_link_state() allocate a link_state, > regardless of whether it currently has any children. > > pcie_aspm_init_link_state(): Called for bridges (upstream end of > link) after all children have been enumerated. No longer needs to > check aspm_support_enabled or pdev->has_secondary_link or the VIA > quirk: pci_aspm_init() already checked that stuff, so we only need > to check pdev->link_state here. > > Now that we are calling alloc_pcie_link_state() from pci_aspm_init() > , get rid of pci_function_0 function and detect downstream link in > pci_aspm_init_upstream() instead when the function number is 0. > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/pci/pcie/aspm.c | 72 ++++++++++++++++++++++++------------------------- > 1 file changed, 36 insertions(+), 36 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index a80d64b..e33f84b 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) > } > } > > -/* > - * The L1 PM substate capability is only implemented in function 0 in a > - * multi function device. > - */ > -static struct pci_dev *pci_function_0(struct pci_bus *linkbus) > -{ > - struct pci_dev *child; > - > - list_for_each_entry(child, &linkbus->devices, bus_list) > - if (PCI_FUNC(child->devfn) == 0) > - return child; > - return NULL; > -} > - > /* Calculate L1.2 PM substate timing parameters */ > static void aspm_calc_l1ss_info(struct pcie_link_state *link, > struct aspm_register_info *upreg, > @@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > INIT_LIST_HEAD(&link->children); > INIT_LIST_HEAD(&link->link); > link->pdev = pdev; > - link->downstream = pci_function_0(pdev->subordinate); > > /* > * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe > @@ -828,11 +813,33 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > > static int pci_aspm_init_downstream(struct pci_dev *pdev) > { > + struct pcie_link_state *link; > + struct pci_dev *parent; > + > + /* > + * The L1 PM substate capability is only implemented in function 0 in a > + * multi function device. > + */ > + if (PCI_FUNC(pdev->devfn) != 0) > + return -EINVAL; > + > + parent = pdev->bus->self; > + if (!parent) > + return -EINVAL; > + > + link = parent->link_state; > + link->downstream = pdev; > return 0; > } > > static int pci_aspm_init_upstream(struct pci_dev *pdev) > { > + struct pcie_link_state *link; > + > + link = alloc_pcie_link_state(pdev); > + if (!link) > + return -ENOMEM; This allocates the link_state when we enumerate a Downstream Port instead of when we enumerate a child device. We want the link_state lifetime to match that of the Downstream Port, so this seems good. But we shouldn't at the same time change the link_state deallocation so it happens when the Downstream Port is removed, not when the child device is removed? > return 0; > } > > @@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev) > */ > int pci_aspm_init(struct pci_dev *pdev) > { > + if (!aspm_support_enabled) > + return 0; > + > + if (!pci_is_pcie(pdev)) > + return -EINVAL; > + > + /* VIA has a strange chipset, root port is under a bridge */ > + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT && > + pdev->bus->self) > + return 0; > + > if (!pdev->has_secondary_link) > return pci_aspm_init_downstream(pdev); > > @@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) > struct pcie_link_state *link; > int blacklist = !!pcie_aspm_sanity_check(pdev); > > - if (!aspm_support_enabled) > - return; > - > - if (pdev->link_state) > - return; > - > - /* > - * We allocate pcie_link_state for the component on the upstream > - * end of a Link, so there's nothing to do unless this device has a > - * Link on its secondary side. > - */ > - if (!pdev->has_secondary_link) > - return; > - > - /* VIA has a strange chipset, root port is under a bridge */ > - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT && > - pdev->bus->self) > + if (!pdev->link_state) > return; > > + link = pdev->link_state; > down_read(&pci_bus_sem); > if (list_empty(&pdev->subordinate->devices)) > goto out; > > mutex_lock(&aspm_lock); > - link = alloc_pcie_link_state(pdev); > - if (!link) > - goto unlock; > + > /* > * Setup initial ASPM state. Note that we need to configure > * upstream links also because capable state of them can be > @@ -910,7 +911,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) > pcie_set_clkpm(link, policy_to_clkpm_state(link)); > } > > -unlock: > mutex_unlock(&aspm_lock); > out: > up_read(&pci_bus_sem); > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add 2017-04-13 20:48 ` Bjorn Helgaas @ 2017-04-13 21:02 ` Bjorn Helgaas 2017-04-14 1:19 ` Sinan Kaya 0 siblings, 1 reply; 27+ messages in thread From: Bjorn Helgaas @ 2017-04-13 21:02 UTC (permalink / raw) To: Sinan Kaya Cc: mayurkumar.patel, David Daney, linux-pci, timur, linux-kernel, Julia Lawall, linux-arm-msm, Bjorn Helgaas, Rajat Jain, linux-arm-kernel On Thu, Apr 13, 2017 at 03:48:00PM -0500, Bjorn Helgaas wrote: > Hi Sinan, > > On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote: > > For bridges, have pcie_aspm_init_link_state() allocate a link_state, > > regardless of whether it currently has any children. > > > > pcie_aspm_init_link_state(): Called for bridges (upstream end of > > link) after all children have been enumerated. No longer needs to > > check aspm_support_enabled or pdev->has_secondary_link or the VIA > > quirk: pci_aspm_init() already checked that stuff, so we only need > > to check pdev->link_state here. > > > > Now that we are calling alloc_pcie_link_state() from pci_aspm_init() > > , get rid of pci_function_0 function and detect downstream link in > > pci_aspm_init_upstream() instead when the function number is 0. > > > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > > --- > > drivers/pci/pcie/aspm.c | 72 ++++++++++++++++++++++++------------------------- > > 1 file changed, 36 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index a80d64b..e33f84b 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) > > } > > } > > > > -/* > > - * The L1 PM substate capability is only implemented in function 0 in a > > - * multi function device. > > - */ > > -static struct pci_dev *pci_function_0(struct pci_bus *linkbus) > > -{ > > - struct pci_dev *child; > > - > > - list_for_each_entry(child, &linkbus->devices, bus_list) > > - if (PCI_FUNC(child->devfn) == 0) > > - return child; > > - return NULL; > > -} > > - > > /* Calculate L1.2 PM substate timing parameters */ > > static void aspm_calc_l1ss_info(struct pcie_link_state *link, > > struct aspm_register_info *upreg, > > @@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > > INIT_LIST_HEAD(&link->children); > > INIT_LIST_HEAD(&link->link); > > link->pdev = pdev; > > - link->downstream = pci_function_0(pdev->subordinate); > > > > /* > > * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe > > @@ -828,11 +813,33 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > > > > static int pci_aspm_init_downstream(struct pci_dev *pdev) > > { > > + struct pcie_link_state *link; > > + struct pci_dev *parent; > > + > > + /* > > + * The L1 PM substate capability is only implemented in function 0 in a > > + * multi function device. > > + */ > > + if (PCI_FUNC(pdev->devfn) != 0) > > + return -EINVAL; > > + > > + parent = pdev->bus->self; > > + if (!parent) > > + return -EINVAL; > > + > > + link = parent->link_state; > > + link->downstream = pdev; > > return 0; > > } > > > > static int pci_aspm_init_upstream(struct pci_dev *pdev) > > { > > + struct pcie_link_state *link; > > + > > + link = alloc_pcie_link_state(pdev); > > + if (!link) > > + return -ENOMEM; > > This allocates the link_state when we enumerate a Downstream Port > instead of when we enumerate a child device. We want the link_state > lifetime to match that of the Downstream Port, so this seems good. > > But we shouldn't at the same time change the link_state deallocation > so it happens when the Downstream Port is removed, not when the child > device is removed? I do see that you change the deallocation in patch [5/5], but I think the deallocation change should be in the same patch as the allocation change. Otherwise I think we have a use-after-free problem in this sequence: # initial enumeration pci_device_add(downstream_port) pci_aspm_init(downstream_port) alloc_pcie_link_state pci_device_add(endpoint) pci_aspm_init(endpoint) # hot-remove endpoint pci_stop_dev(endpoint) pcie_aspm_exit_link_state(endpoint) link = parent->link_state free_link_state(link) # hot-add endpoint pci_aspm_init(endpoint) link = parent->link_state <--- use after free ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add 2017-04-13 21:02 ` Bjorn Helgaas @ 2017-04-14 1:19 ` Sinan Kaya 2017-04-14 1:30 ` Bjorn Helgaas 0 siblings, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-14 1:19 UTC (permalink / raw) To: Bjorn Helgaas Cc: mayurkumar.patel, David Daney, linux-pci, timur, linux-kernel, Julia Lawall, linux-arm-msm, Bjorn Helgaas, Rajat Jain, linux-arm-kernel On 4/13/2017 5:02 PM, Bjorn Helgaas wrote: > I do see that you change the deallocation in patch [5/5], but I think > the deallocation change should be in the same patch as the allocation > change. Otherwise I think we have a use-after-free problem in this > sequence: Sure, I'll reorder. As you can see here, link will be only removed if root port is being removed. Without this, we'll hit the use after free issue you mentioned. if (pdev->has_secondary_link) { link = pdev->link_state; down_read(&pci_bus_sem); mutex_lock(&aspm_lock); list_del(&link->sibling); list_del(&link->link); /* Clock PM is for endpoint device */ free_link_state(link); mutex_unlock(&aspm_lock); up_read(&pci_bus_sem); return; } -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add 2017-04-14 1:19 ` Sinan Kaya @ 2017-04-14 1:30 ` Bjorn Helgaas 0 siblings, 0 replies; 27+ messages in thread From: Bjorn Helgaas @ 2017-04-14 1:30 UTC (permalink / raw) To: Sinan Kaya Cc: Bjorn Helgaas, Patel, Mayurkumar, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Rajat Jain, linux-arm On Thu, Apr 13, 2017 at 8:19 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 4/13/2017 5:02 PM, Bjorn Helgaas wrote: >> I do see that you change the deallocation in patch [5/5], but I think >> the deallocation change should be in the same patch as the allocation >> change. Otherwise I think we have a use-after-free problem in this >> sequence: > > Sure, I'll reorder. As you can see here, link will be only removed if > root port is being removed. > > Without this, we'll hit the use after free issue you mentioned. > > if (pdev->has_secondary_link) { > link = pdev->link_state; > down_read(&pci_bus_sem); > mutex_lock(&aspm_lock); > > list_del(&link->sibling); > list_del(&link->link); > > /* Clock PM is for endpoint device */ > free_link_state(link); > mutex_unlock(&aspm_lock); > up_read(&pci_bus_sem); > return; > } Right, but this "pdev->has_secondary_link" check is in your new code and doesn't show up until patch [5/5]. As of *this* patch [3/5], the link is removed when the endpoint is removed, so we could hit the use-after-free. Granted, we'll only be susceptible to this while bisecting because normally all the patches will be applied. But I think this patch will make more sense and be easier to review if it makes the link_state lifetime match the Port's lifetime. Bjorn ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init [not found] <1491627351-1111-1-git-send-email-okaya@codeaurora.org> ` (2 preceding siblings ...) 2017-04-08 4:55 ` [PATCH V8 3/5] PCI/ASPM: add init hook to device_add Sinan Kaya @ 2017-04-08 4:55 ` Sinan Kaya 2017-04-12 19:19 ` Rajat Jain 2017-04-08 4:55 ` [PATCH V8 5/5] PCI/ASPM: move link_state cleanup to bridge remove Sinan Kaya 4 siblings, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-08 4:55 UTC (permalink / raw) To: linux-pci, timur Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya, Bjorn Helgaas, Rajat Jain, Yinghai Lu, David Daney, Shawn Lin, Julia Lawall, linux-kernel Now that we added a hook to be called from device_add, save the default values from the HW registers early in the boot for further reuse during hot device add/remove operations. If the link is down during boot, assume that we want to enable L0s and L1 following hotplug insertion as well as L1SS if supported. Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/pci/pcie/aspm.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index e33f84b..c7da087 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -505,8 +505,10 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) */ if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S) link->aspm_support |= ASPM_STATE_L0S; - if (dwreg.enabled & PCIE_LINK_STATE_L0S) + if (dwreg.enabled & PCIE_LINK_STATE_L0S) { link->aspm_enabled |= ASPM_STATE_L0S_UP; + link->aspm_default |= ASPM_STATE_L0S_UP; + } if (upreg.enabled & PCIE_LINK_STATE_L0S) link->aspm_enabled |= ASPM_STATE_L0S_DW; link->latency_up.l0s = calc_l0s_latency(upreg.latency_encoding_l0s); @@ -542,9 +544,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) if (link->aspm_support & ASPM_STATE_L1SS) aspm_calc_l1ss_info(link, &upreg, &dwreg); - /* Save default state */ - link->aspm_default = link->aspm_enabled; - /* Setup initial capable state. Will be updated later */ link->aspm_capable = link->aspm_support; /* @@ -835,11 +834,38 @@ static int pci_aspm_init_downstream(struct pci_dev *pdev) static int pci_aspm_init_upstream(struct pci_dev *pdev) { struct pcie_link_state *link; + struct aspm_register_info upreg; + u16 lnk_status; + bool ret; link = alloc_pcie_link_state(pdev); if (!link) return -ENOMEM; + pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status); + ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA); + + if (ret) { + pcie_get_aspm_reg(pdev, &upreg); + if (upreg.enabled & PCIE_LINK_STATE_L0S) + link->aspm_default |= ASPM_STATE_L0S_DW; + if (upreg.enabled & PCIE_LINK_STATE_L1) + link->aspm_default |= ASPM_STATE_L1; + if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1) + link->aspm_default |= ASPM_STATE_L1_1; + if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2) + link->aspm_default |= ASPM_STATE_L1_2; + if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1) + link->aspm_default |= ASPM_STATE_L1_1_PCIPM; + if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2) + link->aspm_default |= ASPM_STATE_L1_2_PCIPM; + } else { + if (!pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS)) + link->aspm_default = ASPM_STATE_L0S | ASPM_STATE_L1; + else + link->aspm_default = ASPM_STATE_ALL; + } + return 0; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-08 4:55 ` [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Sinan Kaya @ 2017-04-12 19:19 ` Rajat Jain 2017-04-14 19:12 ` Sinan Kaya 0 siblings, 1 reply; 27+ messages in thread From: Rajat Jain @ 2017-04-12 19:19 UTC (permalink / raw) To: Sinan Kaya Cc: linux-pci, timur, Patel, Mayurkumar, linux-arm-msm, linux-arm-kernel, Bjorn Helgaas, Yinghai Lu, David Daney, Shawn Lin, Julia Lawall, linux-kernel, Rajat Jain On Fri, Apr 7, 2017 at 9:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > Now that we added a hook to be called from device_add, save the > default values from the HW registers early in the boot for further > reuse during hot device add/remove operations. > > If the link is down during boot, assume that we want to enable L0s > and L1 following hotplug insertion as well as L1SS if supported. IIUC, so far POLICY_DEFAULT meant that we'd just use & follow what BIOS has done, and play it safe (never try to be more opportunistic). With this change however, we'd be slightly overstepping and giving ourselves benefit of doubt if the BIOS could not enable ASPM states because the link was not up. This may be good, but I think we should call it out, and add some more elaborate comment on the POLICY_DEFAULT description (what to, and what not to expect in different situations). It is important because existing systems today, that used to boot without cards and later hotplugged them, didn't have ASPM states enabled. They will now suddenly start seeing all ASPM states enabled including L1 substates for the first time (if supported). My system is not hotplug capable (I have the EP soldered on board, so couldn't do much testing, except for sanity. Please feel free to use my Reviewed-by. > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/pci/pcie/aspm.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index e33f84b..c7da087 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -505,8 +505,10 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) > */ > if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S) > link->aspm_support |= ASPM_STATE_L0S; > - if (dwreg.enabled & PCIE_LINK_STATE_L0S) > + if (dwreg.enabled & PCIE_LINK_STATE_L0S) { > link->aspm_enabled |= ASPM_STATE_L0S_UP; > + link->aspm_default |= ASPM_STATE_L0S_UP; > + } > if (upreg.enabled & PCIE_LINK_STATE_L0S) > link->aspm_enabled |= ASPM_STATE_L0S_DW; > link->latency_up.l0s = calc_l0s_latency(upreg.latency_encoding_l0s); > @@ -542,9 +544,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) > if (link->aspm_support & ASPM_STATE_L1SS) > aspm_calc_l1ss_info(link, &upreg, &dwreg); > > - /* Save default state */ > - link->aspm_default = link->aspm_enabled; > - > /* Setup initial capable state. Will be updated later */ > link->aspm_capable = link->aspm_support; > /* > @@ -835,11 +834,38 @@ static int pci_aspm_init_downstream(struct pci_dev *pdev) > static int pci_aspm_init_upstream(struct pci_dev *pdev) > { > struct pcie_link_state *link; > + struct aspm_register_info upreg; > + u16 lnk_status; > + bool ret; > > link = alloc_pcie_link_state(pdev); > if (!link) > return -ENOMEM; > > + pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status); > + ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA); > + > + if (ret) { > + pcie_get_aspm_reg(pdev, &upreg); > + if (upreg.enabled & PCIE_LINK_STATE_L0S) > + link->aspm_default |= ASPM_STATE_L0S_DW; > + if (upreg.enabled & PCIE_LINK_STATE_L1) > + link->aspm_default |= ASPM_STATE_L1; > + if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1) > + link->aspm_default |= ASPM_STATE_L1_1; > + if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2) > + link->aspm_default |= ASPM_STATE_L1_2; > + if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1) > + link->aspm_default |= ASPM_STATE_L1_1_PCIPM; > + if (upreg.l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2) > + link->aspm_default |= ASPM_STATE_L1_2_PCIPM; > + } else { > + if (!pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS)) > + link->aspm_default = ASPM_STATE_L0S | ASPM_STATE_L1; > + else > + link->aspm_default = ASPM_STATE_ALL; > + } Optional: May be consider moving this code (more aptly) to pcie_aspm_cap_init() by adding a check for link-up before we start reading downstream registers there? I guess you'll need to move the call to pcie_aspm_cap_init() a little further up in pcie_aspm_init_link_state(). > + > return 0; > } > > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-12 19:19 ` Rajat Jain @ 2017-04-14 19:12 ` Sinan Kaya 2017-04-14 21:44 ` Bjorn Helgaas 0 siblings, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-14 19:12 UTC (permalink / raw) To: Rajat Jain Cc: linux-pci, timur, Patel, Mayurkumar, linux-arm-msm, linux-arm-kernel, Bjorn Helgaas, Yinghai Lu, David Daney, Shawn Lin, Julia Lawall, linux-kernel, Rajat Jain Bjorn, On 4/12/2017 3:19 PM, Rajat Jain wrote: > On Fri, Apr 7, 2017 at 9:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> Now that we added a hook to be called from device_add, save the >> default values from the HW registers early in the boot for further >> reuse during hot device add/remove operations. >> >> If the link is down during boot, assume that we want to enable L0s >> and L1 following hotplug insertion as well as L1SS if supported. > > IIUC, so far POLICY_DEFAULT meant that we'd just use & follow what > BIOS has done, and play it safe (never try to be more opportunistic). > With this change however, we'd be slightly overstepping and giving > ourselves benefit of doubt if the BIOS could not enable ASPM states > because the link was not up. This may be good, but I think we should > call it out, and add some more elaborate comment on the POLICY_DEFAULT > description (what to, and what not to expect in different situations). > > It is important because existing systems today, that used to boot > without cards and later hotplugged them, didn't have ASPM states > enabled. They will now suddenly start seeing all ASPM states enabled > including L1 substates for the first time (if supported). > Rajat has a good point here. Would you like me to update the ASPM document with this new behavior for hotplug? Do you have another behavior preference when it comes this? Sinan -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-14 19:12 ` Sinan Kaya @ 2017-04-14 21:44 ` Bjorn Helgaas 2017-04-14 22:17 ` Sinan Kaya 0 siblings, 1 reply; 27+ messages in thread From: Bjorn Helgaas @ 2017-04-14 21:44 UTC (permalink / raw) To: Sinan Kaya Cc: Rajat Jain, Patel, Mayurkumar, Rajat Jain, David Daney, linux-pci, timur, linux-kernel, Julia Lawall, linux-arm-msm, Bjorn Helgaas, Yinghai Lu, Shawn Lin, linux-arm-kernel, Myron Stowe [+cc Myron, lkml] On Fri, Apr 14, 2017 at 03:12:35PM -0400, Sinan Kaya wrote: > Bjorn, > > On 4/12/2017 3:19 PM, Rajat Jain wrote: > > On Fri, Apr 7, 2017 at 9:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > >> Now that we added a hook to be called from device_add, save the > >> default values from the HW registers early in the boot for further > >> reuse during hot device add/remove operations. > >> > >> If the link is down during boot, assume that we want to enable L0s > >> and L1 following hotplug insertion as well as L1SS if supported. > > > > IIUC, so far POLICY_DEFAULT meant that we'd just use & follow what > > BIOS has done, and play it safe (never try to be more opportunistic). > > With this change however, we'd be slightly overstepping and giving > > ourselves benefit of doubt if the BIOS could not enable ASPM states > > because the link was not up. This may be good, but I think we should > > call it out, and add some more elaborate comment on the POLICY_DEFAULT > > description (what to, and what not to expect in different situations). > > > > It is important because existing systems today, that used to boot > > without cards and later hotplugged them, didn't have ASPM states > > enabled. They will now suddenly start seeing all ASPM states enabled > > including L1 substates for the first time (if supported). > > > > Rajat has a good point here. Would you like me to update the ASPM document > with this new behavior for hotplug? > > Do you have another behavior preference when it comes this? That *is* a very good point. I think the change in behavior could be surprising. I wonder if we should revise our understanding of what POLICY_DEFAULT means. If we decided it means "the kernel never changes any ASPM config", it would be clear that we keep the BIOS configuration for everything present at boot, and we don't enable ASPM for any hot-added devices. I think the motivation for this series is to apply the BIOS's power management policy to hot-added devices. There's no direct way to know the BIOS's policy, so we're trying to infer it from the boot-time link configurations. Should we even *try* to apply the BIOS's policy? I don't know. If a platform really wanted to maintain control over ASPM and apply its policy consistently, I think it could do that by setting ACPI_FADT_NO_ASPM and using acpiphp instead of pciehp. Then the OS would keep its mitts off ASPM, and the BIOS would have a chance to configure ASPM for hot-added devices before giving them to the OS. Here are the possibilities I see for POLICY_DEFAULT: 1) Never touch ASPM config (what we have today) Boot-present devices: ASPM config retained from BIOS Hot-added devices: ASPM disabled (poweron state) 2) Linux maintains BIOS policy (conservative) Boot-present devices: ASPM config retained from BIOS Hot-added devices (slot occupied at boot): use boot-time ASPM config Hot-added devices (slot empty at boot): ASPM disabled 3) Linux maintains BIOS policy (aggressive, your current patch) Boot-present devices: ASPM config retained from BIOS Hot-added devices (slot occupied at boot): use boot-time ASPM config Hot-added devices (slot empty at boot): ASPM enabled I'm becoming less convinced that options 2 or 3 make sense. For one thing, they're both hard to describe concisely because there are too many special cases, and that's always a red flag for me. Even for a given BIOS power management policy, the ASPM configuration may depend on the particular device; for example, a balanced policy might enable ASPM for USB devices but not for NICs. So I'm not sure it really makes sense to remember what BIOS did for the card that was in the slot at boot-time and apply that to a possibly different card hot-added later. I think there's an argument to be made that if we care about ASPM configuration, we should be using a non-POLICY_DEFAULT setting. And I think there's value in having POLICY_DEFAULT be the absolute lowest- risk setting, which I think means option 1. What do you think? Bjorn ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-14 21:44 ` Bjorn Helgaas @ 2017-04-14 22:17 ` Sinan Kaya 2017-04-17 16:38 ` Bjorn Helgaas 0 siblings, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-14 22:17 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rajat Jain, Patel, Mayurkumar, Rajat Jain, David Daney, linux-pci, timur, linux-kernel, Julia Lawall, linux-arm-msm, Bjorn Helgaas, Yinghai Lu, Shawn Lin, linux-arm-kernel, Myron Stowe On 4/14/2017 5:44 PM, Bjorn Helgaas wrote: > I think there's an argument to be made that if we care about ASPM > configuration, we should be using a non-POLICY_DEFAULT setting. And I > think there's value in having POLICY_DEFAULT be the absolute lowest- > risk setting, which I think means option 1. > > What do you think? I see your point. The counter argument is that most of the users do not know what an ASPM kernel command line is unless they understand PCI language. I have been using the powersave policy option until now. I recently realized that nobody except me is using this option. Therefore, we are wasting power by default following a hotplug insertion. This is the case where I'm trying to avoid. With the introduction of NVMe u.2 drives, hotplug is becoming more and more mainstream. I decided to take the matters into my hand with this series for this very reason. Like you said, BIOS is out of the picture with pciehp. There is nobody to configure ASPM following a hotplug insertion. I can also claim that If user wants performance, they should boot with the performance policy or pcie_aspm=off parameters. I saw this recommendation in multiple DPDK tuning documents. Like you said, what do we do by default is the question. Should we opt for safe like we are doing, or try to save some power. Maybe, we are missing a HPP option from the PCI spec. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-14 22:17 ` Sinan Kaya @ 2017-04-17 16:38 ` Bjorn Helgaas 2017-04-17 17:50 ` Sinan Kaya 0 siblings, 1 reply; 27+ messages in thread From: Bjorn Helgaas @ 2017-04-17 16:38 UTC (permalink / raw) To: Sinan Kaya Cc: Bjorn Helgaas, Rajat Jain, Patel, Mayurkumar, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe On Fri, Apr 14, 2017 at 5:17 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 4/14/2017 5:44 PM, Bjorn Helgaas wrote: >> I think there's an argument to be made that if we care about ASPM >> configuration, we should be using a non-POLICY_DEFAULT setting. And I >> think there's value in having POLICY_DEFAULT be the absolute lowest- >> risk setting, which I think means option 1. >> >> What do you think? > > I see your point. The counter argument is that most of the users do not > know what an ASPM kernel command line is unless they understand PCI > language. I don't think the answer is using the "pcie_aspm.policy=" boot argument. I certainly don't want users to have to deal with that. I wish we didn't even have that parameter. I think we need runtime knobs instead (and I guess we already have /sys/module/pcie_aspm/parameters/policy and /sys/.../link_state), and distro userspace should use them. I'm envisioning something in "System Settings / Power" or similar. Basically I think the policy doesn't *have* to be dictated by a kernel boot-time parameter, so it should not be. > I have been using the powersave policy option until now. I recently realized > that nobody except me is using this option. Therefore, we are wasting > power by default following a hotplug insertion. > > This is the case where I'm trying to avoid. With the introduction of NVMe > u.2 drives, hotplug is becoming more and more mainstream. I decided to > take the matters into my hand with this series for this very reason. > > Like you said, BIOS is out of the picture with pciehp. There is nobody > to configure ASPM following a hotplug insertion. > > I can also claim that If user wants performance, they should boot with > the performance policy or pcie_aspm=off parameters. > > I saw this recommendation in multiple DPDK tuning documents. > > Like you said, what do we do by default is the question. Should we opt > for safe like we are doing, or try to save some power. I think safety is paramount. Every user should be able to boot safely without any kernel parameters. We don't want users to have a problem booting and then have to search for a workaround like booting with "pcie_aspm=off". Most users will never do that. Here's a long-term strawman proposal, see what you think: - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. - Default aspm_policy is POLICY_DEFAULT always. - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled ASPM, we leave it that way; we leave ASPM disabled on hot-added devices. - Deprecate kernel boot parameters (possibly keep pcie_aspm=off for debugging use). - Use /sys/module/pcie_aspm/parameters/policy for run-time system-wide control, including for future hot-added devices. - Remove CONFIG_PCIEASPM_DEBUG and enable that code always, so we have fine-grained run-time control. > Maybe, we are missing a HPP option from the PCI spec. That's an interesting idea. _HPX does have provision for manipulating Link Control bits (see ACPI r5.0, sec 6.2.8.3), but I don't think very many systems implement it. And there's currently no connection between program_hpp_type2() and aspm.c, so I'm a little worried that we might have issues if a system did implement an _HPX that sets any of the ASPM bits. Bjorn ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-17 16:38 ` Bjorn Helgaas @ 2017-04-17 17:50 ` Sinan Kaya 2017-04-21 7:46 ` Patel, Mayurkumar 0 siblings, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-17 17:50 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, Rajat Jain, Patel, Mayurkumar, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: >> Like you said, what do we do by default is the question. Should we opt >> for safe like we are doing, or try to save some power. > I think safety is paramount. Every user should be able to boot safely > without any kernel parameters. We don't want users to have a problem > booting and then have to search for a workaround like booting with > "pcie_aspm=off". Most users will never do that. > OK, no problem with leaving the behavior as it is. My initial approach was #2. We knew this way that user had full control over the ASPM policy by changing the BIOS option. Then, Mayurkumar complained that ASPM is not enabled following a hotplug insertion to an empty slot. That's when I switched to #3 as it sounded like a good thing to have for us. > Here's a long-term strawman proposal, see what you think: > > - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. > - Default aspm_policy is POLICY_DEFAULT always. > - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled > ASPM, we leave it that way; we leave ASPM disabled on hot-added > devices. I can easily see people complaining the other way around. There could be some boot FW that doesn't know what ASPM is and this particular system could rely on the compile time option for enabling power options. Maybe, a command line option will be required for them to keep the existing behavior. > - Deprecate kernel boot parameters (possibly keep pcie_aspm=off for > debugging use). > - Use /sys/module/pcie_aspm/parameters/policy for run-time > system-wide control, including for future hot-added devices. > - Remove CONFIG_PCIEASPM_DEBUG and enable that code always, so we > have fine-grained run-time control. > Runtime control sounds like a better plan. We need hooks into the system power management policy. >> Maybe, we are missing a HPP option from the PCI spec. > That's an interesting idea. _HPX does have provision for manipulating > Link Control bits (see ACPI r5.0, sec 6.2.8.3), but I don't think very > many systems implement it. And there's currently no connection > between program_hpp_type2() and aspm.c, so I'm a little worried that > we might have issues if a system did implement an _HPX that sets any > of the ASPM bits. I looked at the spec some more. These are there to restore the register settings following hotplug insertion. I agree it won't play nice with ASPM as the control bits need to be enabled in coordination with the upstream device. I think the right approach is to let the userspace feed the required policy to the kernel like you suggested. Then, it needs to be per port via link_state to have the most flexibility. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-17 17:50 ` Sinan Kaya @ 2017-04-21 7:46 ` Patel, Mayurkumar 2017-04-21 13:50 ` Sinan Kaya 2017-04-25 18:45 ` Bjorn Helgaas 0 siblings, 2 replies; 27+ messages in thread From: Patel, Mayurkumar @ 2017-04-21 7:46 UTC (permalink / raw) To: Sinan Kaya, Bjorn Helgaas Cc: Bjorn Helgaas, Rajat Jain, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe Hi Bjorn/Kaya, > >On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: >>> Like you said, what do we do by default is the question. Should we opt >>> for safe like we are doing, or try to save some power. >> I think safety is paramount. Every user should be able to boot safely >> without any kernel parameters. We don't want users to have a problem >> booting and then have to search for a workaround like booting with >> "pcie_aspm=off". Most users will never do that. >> > >OK, no problem with leaving the behavior as it is. > >My initial approach was #2. We knew this way that user had full control >over the ASPM policy by changing the BIOS option. Then, Mayurkumar >complained that ASPM is not enabled following a hotplug insertion to an >empty slot. That's when I switched to #3 as it sounded like a good thing >to have for us. > >> Here's a long-term strawman proposal, see what you think: >> >> - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. >> - Default aspm_policy is POLICY_DEFAULT always. >> - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled >> ASPM, we leave it that way; we leave ASPM disabled on hot-added >> devices. > I am also ok with leaving the same behavior as now. But still following is something open I feel besides, Which may be there in your comments redundantly. The current problem is, pcie_aspm_exit_link_state() disables the ASPM configuration even if POLICY_DEFAULT was set. I am seeing already following problem(or may be influence) with it. The Endpoint I have does not have does not have "Presence detect change" mechanism. Hot plug is working with Link status events. When link is in L1 or L1SS and if EP is powered off, no Link status change event are triggered (It might be the expected behavior in L1 or L1SS). When next time EP is powered on there are link down and link up events coming one after other. BIOS enables ASPM on Root port and Endpoint, but while processing link status down, pcie_aspm_exit_link_state() clears the ASPM already which were enabled by BIOS. If we want to follow above approach then shall we consider having something similar as following? diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 1dfa10c..bf5be6d 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -940,7 +940,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) parent_link = link->parent; /* All functions are removed, so just disable ASPM for the link */ - pcie_config_aspm_link(link, 0); + if (aspm_policy != POLICY_DEFAULT) + pcie_config_aspm_link(link, 0); list_del(&link->sibling); list_del(&link->link); /* Clock PM is for endpoint device */ >I can easily see people complaining the other way around. There >could be some boot FW that doesn't know what ASPM is and this particular >system could rely on the compile time option for enabling power options. >Maybe, a command line option will be required for them to keep the existing >behavior. > >> - Deprecate kernel boot parameters (possibly keep pcie_aspm=off for >> debugging use). >> - Use /sys/module/pcie_aspm/parameters/policy for run-time >> system-wide control, including for future hot-added devices. >> - Remove CONFIG_PCIEASPM_DEBUG and enable that code always, so we >> have fine-grained run-time control. >> > >Runtime control sounds like a better plan. We need hooks into the system >power management policy. > >>> Maybe, we are missing a HPP option from the PCI spec. >> That's an interesting idea. _HPX does have provision for manipulating >> Link Control bits (see ACPI r5.0, sec 6.2.8.3), but I don't think very >> many systems implement it. And there's currently no connection >> between program_hpp_type2() and aspm.c, so I'm a little worried that >> we might have issues if a system did implement an _HPX that sets any >> of the ASPM bits. > >I looked at the spec some more. These are there to restore the register >settings following hotplug insertion. I agree it won't play nice with ASPM >as the control bits need to be enabled in coordination with the upstream >device. > >I think the right approach is to let the userspace feed the required >policy to the kernel like you suggested. Then, it needs to be per port >via link_state to have the most flexibility. > > >-- >Sinan Kaya >Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. >Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-21 7:46 ` Patel, Mayurkumar @ 2017-04-21 13:50 ` Sinan Kaya 2017-04-21 14:13 ` Patel, Mayurkumar 2017-04-25 18:45 ` Bjorn Helgaas 1 sibling, 1 reply; 27+ messages in thread From: Sinan Kaya @ 2017-04-21 13:50 UTC (permalink / raw) To: Patel, Mayurkumar, Bjorn Helgaas Cc: Bjorn Helgaas, Rajat Jain, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe On 4/21/2017 3:46 AM, Patel, Mayurkumar wrote: > If we want to follow above approach then shall we consider having something similar as following? Do you see this problem if you boot with pcie_aspm.policy=powersave option? -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-21 13:50 ` Sinan Kaya @ 2017-04-21 14:13 ` Patel, Mayurkumar 0 siblings, 0 replies; 27+ messages in thread From: Patel, Mayurkumar @ 2017-04-21 14:13 UTC (permalink / raw) To: Sinan Kaya, Bjorn Helgaas Cc: Bjorn Helgaas, Rajat Jain, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe > >On 4/21/2017 3:46 AM, Patel, Mayurkumar wrote: >> If we want to follow above approach then shall we consider having something similar as following? > >Do you see this problem if you boot with pcie_aspm.policy=powersave option? > No problems. with pcie_aspm.policy=powersave(L1SS are not enabled in this case but L1 stays ok all the time after many Power(hotplug) cycles but I think that is expected with this policy) and pcie_aspm.policy=powersupersave (L1/L1SS both stays ok all the time). > >-- >Sinan Kaya >Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. >Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-21 7:46 ` Patel, Mayurkumar 2017-04-21 13:50 ` Sinan Kaya @ 2017-04-25 18:45 ` Bjorn Helgaas 2017-05-02 12:02 ` Patel, Mayurkumar 1 sibling, 1 reply; 27+ messages in thread From: Bjorn Helgaas @ 2017-04-25 18:45 UTC (permalink / raw) To: Patel, Mayurkumar Cc: Sinan Kaya, Bjorn Helgaas, Rajat Jain, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe On Fri, Apr 21, 2017 at 2:46 AM, Patel, Mayurkumar <mayurkumar.patel@intel.com> wrote: > Hi Bjorn/Kaya, > > >> >>On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: >>>> Like you said, what do we do by default is the question. Should we opt >>>> for safe like we are doing, or try to save some power. >>> I think safety is paramount. Every user should be able to boot safely >>> without any kernel parameters. We don't want users to have a problem >>> booting and then have to search for a workaround like booting with >>> "pcie_aspm=off". Most users will never do that. >>> >> >>OK, no problem with leaving the behavior as it is. >> >>My initial approach was #2. We knew this way that user had full control >>over the ASPM policy by changing the BIOS option. Then, Mayurkumar >>complained that ASPM is not enabled following a hotplug insertion to an >>empty slot. That's when I switched to #3 as it sounded like a good thing >>to have for us. >> >>> Here's a long-term strawman proposal, see what you think: >>> >>> - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. >>> - Default aspm_policy is POLICY_DEFAULT always. >>> - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled >>> ASPM, we leave it that way; we leave ASPM disabled on hot-added >>> devices. >> > I am also ok with leaving the same behavior as now. > But still following is something open I feel besides, Which may be there in your comments redundantly. > The current problem is, pcie_aspm_exit_link_state() disables the ASPM configuration even > if POLICY_DEFAULT was set. We call pcie_aspm_exit_link_state() when removing an endpoint. When we remove an endpoint, I think disabling ASPM is the right thing to do. The spec (PCIe r3.1, sec 5.4.1.3) says "Software must not enable L0s in either direction on a given Link unless components on both sides of the Link each support L0s; otherwise, the result is undefined." > I am seeing already following problem(or may be influence) with it. The Endpoint I have does not have > does not have "Presence detect change" mechanism. Hot plug is working with Link status events. > When link is in L1 or L1SS and if EP is powered off, no Link status change event are triggered (It might be > the expected behavior in L1 or L1SS). When next time EP is powered on there are link down and > link up events coming one after other. BIOS enables ASPM on Root port and Endpoint, but while > processing link status down, pcie_aspm_exit_link_state() clears the ASPM already which were enabled by BIOS. > If we want to follow above approach then shall we consider having something similar as following? The proposal was to leave ASPM disabled on hot-added devices. If the endpoint was powered off and powered back on again, I think that device looks like a hot-added device, doesn't it? Bjorn ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-04-25 18:45 ` Bjorn Helgaas @ 2017-05-02 12:02 ` Patel, Mayurkumar 2017-05-03 21:10 ` Bjorn Helgaas 0 siblings, 1 reply; 27+ messages in thread From: Patel, Mayurkumar @ 2017-05-02 12:02 UTC (permalink / raw) To: Bjorn Helgaas Cc: Sinan Kaya, Bjorn Helgaas, Rajat Jain, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe Hi Bjorn > >On Fri, Apr 21, 2017 at 2:46 AM, Patel, Mayurkumar ><mayurkumar.patel@intel.com> wrote: >> Hi Bjorn/Kaya, >> >> >>> >>>On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: >>>>> Like you said, what do we do by default is the question. Should we opt >>>>> for safe like we are doing, or try to save some power. >>>> I think safety is paramount. Every user should be able to boot safely >>>> without any kernel parameters. We don't want users to have a problem >>>> booting and then have to search for a workaround like booting with >>>> "pcie_aspm=off". Most users will never do that. >>>> >>> >>>OK, no problem with leaving the behavior as it is. >>> >>>My initial approach was #2. We knew this way that user had full control >>>over the ASPM policy by changing the BIOS option. Then, Mayurkumar >>>complained that ASPM is not enabled following a hotplug insertion to an >>>empty slot. That's when I switched to #3 as it sounded like a good thing >>>to have for us. >>> >>>> Here's a long-term strawman proposal, see what you think: >>>> >>>> - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. >>>> - Default aspm_policy is POLICY_DEFAULT always. >>>> - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled >>>> ASPM, we leave it that way; we leave ASPM disabled on hot-added >>>> devices. >>> >> I am also ok with leaving the same behavior as now. >> But still following is something open I feel besides, Which may be there in your comments redundantly. >> The current problem is, pcie_aspm_exit_link_state() disables the ASPM configuration even >> if POLICY_DEFAULT was set. > >We call pcie_aspm_exit_link_state() when removing an endpoint. When >we remove an endpoint, I think disabling ASPM is the right thing to >do. The spec (PCIe r3.1, sec 5.4.1.3) says "Software must not enable >L0s in either direction on a given Link unless components on both >sides of the Link each support L0s; otherwise, the result is >undefined." > Yes, you are right and per spec also it makes sense that ASPM needs to be disabled. But, if POLICY_DEFAULT is set then, shouldn't BIOS take care of disabling ASPM? >> I am seeing already following problem(or may be influence) with it. The Endpoint I have does not have >> does not have "Presence detect change" mechanism. Hot plug is working with Link status events. >> When link is in L1 or L1SS and if EP is powered off, no Link status change event are triggered (It might be >> the expected behavior in L1 or L1SS). When next time EP is powered on there are link down and >> link up events coming one after other. BIOS enables ASPM on Root port and Endpoint, but while >> processing link status down, pcie_aspm_exit_link_state() clears the ASPM already which were enabled by BIOS. >> If we want to follow above approach then shall we consider having something similar as following? > >The proposal was to leave ASPM disabled on hot-added devices. If the >endpoint was powered off and powered back on again, I think that >device looks like a hot-added device, doesn't it? > Yes, it is hot-added device. Also, I understand, for POLICY_DEFAULT, OS would/should not touch ASPM(enable/disable), but BIOS could still (enable/disable), right? Currently, what happens in my system is as following, (each 2nd power cycle/hotplug of Endpoint disables ASPM): First Power cycle (When ASPM L1 is already enabled): device gets powered off -> there are no Link status events, so no pcie hotplug interrupt and pcie_aspm_exit_link_state() triggered. When the device gets powered on again -> Link down/Link up events are coming back to back. First Link down is served. (BIOS checks for the Link status and enables ASPM already, as the device is actually powered back). OS calls pcie_aspm_exit_link_state() and ASPM gets disabled by OS. Second Power cycle (When ASPM L1 is disabled after above): device gets powered off -> there are link status events, pcie hotplug interrupt is triggered and pcie_aspm_exit_link_state() triggered. OS disables ASPM. BIOS checks Link status and disables ASPM too. When the device gets powered on -> BIOS enables ASPM and as this is pcie hotplug insertion, OS does not interfere and we have ASPM enabled. The above sequence happens each 2nd power cycle of the hotplug device. So One could still argue if POLICY_DEFAULT is set, then why OS disables ASPM if it is not meant to touch configuration. This is why I proposed following kind of change, so that OS would not touch ASPM, if POLICY_DEFAULT is set. Also, With the below change, everything relies on BIOS for ASPM when POLICY_DEFAULT is set and I see above problem gets resolved. Also, the existing ASPM behavior does not have impact, unless specific BIOS does not disable ASPM on Root Port when device gets removed. >Bjorn Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-05-02 12:02 ` Patel, Mayurkumar @ 2017-05-03 21:10 ` Bjorn Helgaas 2017-05-15 9:10 ` Patel, Mayurkumar 0 siblings, 1 reply; 27+ messages in thread From: Bjorn Helgaas @ 2017-05-03 21:10 UTC (permalink / raw) To: Patel, Mayurkumar Cc: Bjorn Helgaas, Sinan Kaya, Rajat Jain, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe On Tue, May 02, 2017 at 12:02:53PM +0000, Patel, Mayurkumar wrote: > >On Fri, Apr 21, 2017 at 2:46 AM, Patel, Mayurkumar > ><mayurkumar.patel@intel.com> wrote: > >>>On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: > >>>>> Like you said, what do we do by default is the question. Should we opt > >>>>> for safe like we are doing, or try to save some power. > >>>> I think safety is paramount. Every user should be able to boot safely > >>>> without any kernel parameters. We don't want users to have a problem > >>>> booting and then have to search for a workaround like booting with > >>>> "pcie_aspm=off". Most users will never do that. > >>> > >>>OK, no problem with leaving the behavior as it is. > >>> > >>>My initial approach was #2. We knew this way that user had full control > >>>over the ASPM policy by changing the BIOS option. Then, Mayurkumar > >>>complained that ASPM is not enabled following a hotplug insertion to an > >>>empty slot. That's when I switched to #3 as it sounded like a good thing > >>>to have for us. > >>> > >>>> Here's a long-term strawman proposal, see what you think: > >>>> > >>>> - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. > >>>> - Default aspm_policy is POLICY_DEFAULT always. > >>>> - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled > >>>> ASPM, we leave it that way; we leave ASPM disabled on hot-added > >>>> devices. > >>> > >> I am also ok with leaving the same behavior as now. But still > >> following is something open I feel besides, Which may be there in > >> your comments redundantly. The current problem is, > >> pcie_aspm_exit_link_state() disables the ASPM configuration even > >> if POLICY_DEFAULT was set. > > > >We call pcie_aspm_exit_link_state() when removing an endpoint. When > >we remove an endpoint, I think disabling ASPM is the right thing to > >do. The spec (PCIe r3.1, sec 5.4.1.3) says "Software must not enable > >L0s in either direction on a given Link unless components on both > >sides of the Link each support L0s; otherwise, the result is > >undefined." > > Yes, you are right and per spec also it makes sense that ASPM needs > to be disabled. But, if POLICY_DEFAULT is set then, shouldn't BIOS > take care of disabling ASPM? No, I don't think so. POLICY_DEFAULT is a Linux thing and BIOS doesn't know anything about it. ASPM can be configured by BIOS before handoff to Linux, but after handoff it should be managed either entirely by BIOS or entirely by Linux. If BIOS wants to retain ASPM control, it would have to tell the OS *not* to use ASPM, and it would have to use ACPI hotplug. In this case, POLICY_DEFAULT is irrelevant because Linux shouldn't do anything with ASPM. But normally BIOS allows Linux to control ASPM, and we would use native PCIe hotplug (pciehp) instead of ACPI hotplug, and BIOS has no opportunity to enable or disable ASPM on hotplug events. > >> I am seeing already following problem(or may be influence) with > >> it. The Endpoint I have does not have does not have "Presence > >> detect change" mechanism. Hot plug is working with Link status > >> events. When link is in L1 or L1SS and if EP is powered off, no > >> Link status change event are triggered (It might be the expected > >> behavior in L1 or L1SS). When next time EP is powered on there > >> are link down and link up events coming one after other. BIOS > >> enables ASPM on Root port and Endpoint, but while processing link > >> status down, pcie_aspm_exit_link_state() clears the ASPM already > >> which were enabled by BIOS. If we want to follow above approach > >> then shall we consider having something similar as following? > > > >The proposal was to leave ASPM disabled on hot-added devices. If > >the endpoint was powered off and powered back on again, I think > >that device looks like a hot-added device, doesn't it? > > Yes, it is hot-added device. Also, I understand, for POLICY_DEFAULT, > OS would/should not touch ASPM(enable/disable), but BIOS could still > (enable/disable), right? Maybe I'm misunderstanding your question. There are two questions here: 1) Does the BIOS allow Linux to manage ASPM? 2) If Linux does manage ASPM, what policy does it use? If BIOS doesn't allow Linux to manage ASPM, POLICY_DEFAULT is irrelevant. If BIOS does allow Linux to manage ASPM, POLICY_DEFAULT means Linux should use the settings made by BIOS. The user could select a different policy, and then Linux would change the ASPM configuration accordingly. > Currently, what happens in my system is as following, (each 2nd > power cycle/hotplug of Endpoint disables ASPM): > > > First Power cycle (When ASPM L1 is already enabled): device gets > powered off -> there are no Link status events, so no pcie hotplug > interrupt and pcie_aspm_exit_link_state() triggered. If the Downstream Port leading to your Endpoint is hotplug capable, doesn't the spec require that it can report link state changes (PCIe r3.1, sec 7.8.6, 7.8.10, 7.8.11)? > When the device gets powered on again -> Link down/Link up events > are coming back to back. First Link down is served. (BIOS checks > for the Link status and enables ASPM already, as the device is > actually powered back). OS calls pcie_aspm_exit_link_state() and > ASPM gets disabled by OS. > > Second Power cycle (When ASPM L1 is disabled after above): device > gets powered off -> there are link status events, pcie hotplug > interrupt is triggered and pcie_aspm_exit_link_state() triggered. > OS disables ASPM. BIOS checks Link status and disables ASPM too. > When the device gets powered on -> BIOS enables ASPM and as this is > pcie hotplug insertion, OS does not interfere and we have ASPM > enabled. I don't understand this sequence. If we're using native PCIe hotplug, BIOS should not be involved to enable ASPM when the device is powered on. > The above sequence happens each 2nd power cycle of the hotplug > device. > > So One could still argue if POLICY_DEFAULT is set, then why OS > disables ASPM if it is not meant to touch configuration. This is > why I proposed following kind of change, so that OS would not touch > ASPM, if POLICY_DEFAULT is set. Also, With the below change, > everything relies on BIOS for ASPM when POLICY_DEFAULT is set and I > see above problem gets resolved. Also, the existing ASPM behavior > does not have impact, unless specific BIOS does not disable ASPM on > Root Port when device gets removed. ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init 2017-05-03 21:10 ` Bjorn Helgaas @ 2017-05-15 9:10 ` Patel, Mayurkumar 0 siblings, 0 replies; 27+ messages in thread From: Patel, Mayurkumar @ 2017-05-15 9:10 UTC (permalink / raw) To: 'Bjorn Helgaas' Cc: Bjorn Helgaas, Sinan Kaya, Rajat Jain, Rajat Jain, David Daney, linux-pci, Timur Tabi, linux-kernel, Julia Lawall, linux-arm-msm, Yinghai Lu, Shawn Lin, linux-arm, Myron Stowe Hi Bjorn Thanks a lot for your reply and explanations. Sorry for my late reply due to some other emergencies. > >On Tue, May 02, 2017 at 12:02:53PM +0000, Patel, Mayurkumar wrote: >> >On Fri, Apr 21, 2017 at 2:46 AM, Patel, Mayurkumar >> ><mayurkumar.patel@intel.com> wrote: >> >>>On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: >> >>>>> Like you said, what do we do by default is the question. Should we opt >> >>>>> for safe like we are doing, or try to save some power. >> >>>> I think safety is paramount. Every user should be able to boot safely >> >>>> without any kernel parameters. We don't want users to have a problem >> >>>> booting and then have to search for a workaround like booting with >> >>>> "pcie_aspm=off". Most users will never do that. >> >>> >> >>>OK, no problem with leaving the behavior as it is. >> >>> >> >>>My initial approach was #2. We knew this way that user had full control >> >>>over the ASPM policy by changing the BIOS option. Then, Mayurkumar >> >>>complained that ASPM is not enabled following a hotplug insertion to an >> >>>empty slot. That's when I switched to #3 as it sounded like a good thing >> >>>to have for us. >> >>> >> >>>> Here's a long-term strawman proposal, see what you think: >> >>>> >> >>>> - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. >> >>>> - Default aspm_policy is POLICY_DEFAULT always. >> >>>> - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled >> >>>> ASPM, we leave it that way; we leave ASPM disabled on hot-added >> >>>> devices. >> >>> >> >> I am also ok with leaving the same behavior as now. But still >> >> following is something open I feel besides, Which may be there in >> >> your comments redundantly. The current problem is, >> >> pcie_aspm_exit_link_state() disables the ASPM configuration even >> >> if POLICY_DEFAULT was set. >> > >> >We call pcie_aspm_exit_link_state() when removing an endpoint. When >> >we remove an endpoint, I think disabling ASPM is the right thing to >> >do. The spec (PCIe r3.1, sec 5.4.1.3) says "Software must not enable >> >L0s in either direction on a given Link unless components on both >> >sides of the Link each support L0s; otherwise, the result is >> >undefined." >> >> Yes, you are right and per spec also it makes sense that ASPM needs >> to be disabled. But, if POLICY_DEFAULT is set then, shouldn't BIOS >> take care of disabling ASPM? > >No, I don't think so. POLICY_DEFAULT is a Linux thing and BIOS >doesn't know anything about it. > >ASPM can be configured by BIOS before handoff to Linux, but after >handoff it should be managed either entirely by BIOS or entirely by >Linux. If BIOS wants to retain ASPM control, it would have to tell >the OS *not* to use ASPM, and it would have to use ACPI hotplug. In >this case, POLICY_DEFAULT is irrelevant because Linux shouldn't do >anything with ASPM. > >But normally BIOS allows Linux to control ASPM, and we would use >native PCIe hotplug (pciehp) instead of ACPI hotplug, and BIOS has no >opportunity to enable or disable ASPM on hotplug events. > BIOS that I am having, has an SMI handler Which gets triggered upon Hotplug (Data Link Layer State Changed) Interrupt Which configures ASPM L1/L1SS in BIOS and We are still using Native Hotplug driver. Sounds like BIOS we have in our System, does not inform OS that it wants control ASPM. >> >> I am seeing already following problem(or may be influence) with >> >> it. The Endpoint I have does not have does not have "Presence >> >> detect change" mechanism. Hot plug is working with Link status >> >> events. When link is in L1 or L1SS and if EP is powered off, no >> >> Link status change event are triggered (It might be the expected >> >> behavior in L1 or L1SS). When next time EP is powered on there >> >> are link down and link up events coming one after other. BIOS >> >> enables ASPM on Root port and Endpoint, but while processing link >> >> status down, pcie_aspm_exit_link_state() clears the ASPM already >> >> which were enabled by BIOS. If we want to follow above approach >> >> then shall we consider having something similar as following? >> > >> >The proposal was to leave ASPM disabled on hot-added devices. If >> >the endpoint was powered off and powered back on again, I think >> >that device looks like a hot-added device, doesn't it? >> >> Yes, it is hot-added device. Also, I understand, for POLICY_DEFAULT, >> OS would/should not touch ASPM(enable/disable), but BIOS could still >> (enable/disable), right? > >Maybe I'm misunderstanding your question. There are two questions >here: > > 1) Does the BIOS allow Linux to manage ASPM? > > 2) If Linux does manage ASPM, what policy does it use? > >If BIOS doesn't allow Linux to manage ASPM, POLICY_DEFAULT is >irrelevant. If BIOS does allow Linux to manage ASPM, POLICY_DEFAULT >means Linux should use the settings made by BIOS. The user could >select a different policy, and then Linux would change the ASPM >configuration accordingly. > Ok understood. >> Currently, what happens in my system is as following, (each 2nd >> power cycle/hotplug of Endpoint disables ASPM): >> >> >> First Power cycle (When ASPM L1 is already enabled): device gets >> powered off -> there are no Link status events, so no pcie hotplug >> interrupt and pcie_aspm_exit_link_state() triggered. > >If the Downstream Port leading to your Endpoint is hotplug capable, >doesn't the spec require that it can report link state changes (PCIe >r3.1, sec 7.8.6, 7.8.10, 7.8.11)? > >> When the device gets powered on again -> Link down/Link up events >> are coming back to back. First Link down is served. (BIOS checks >> for the Link status and enables ASPM already, as the device is >> actually powered back). OS calls pcie_aspm_exit_link_state() and >> ASPM gets disabled by OS. >> >> Second Power cycle (When ASPM L1 is disabled after above): device >> gets powered off -> there are link status events, pcie hotplug >> interrupt is triggered and pcie_aspm_exit_link_state() triggered. >> OS disables ASPM. BIOS checks Link status and disables ASPM too. >> When the device gets powered on -> BIOS enables ASPM and as this is >> pcie hotplug insertion, OS does not interfere and we have ASPM >> enabled. > >I don't understand this sequence. If we're using native PCIe hotplug, >BIOS should not be involved to enable ASPM when the device is powered >on. > >> The above sequence happens each 2nd power cycle of the hotplug >> device. >> >> So One could still argue if POLICY_DEFAULT is set, then why OS >> disables ASPM if it is not meant to touch configuration. This is >> why I proposed following kind of change, so that OS would not touch >> ASPM, if POLICY_DEFAULT is set. Also, With the below change, >> everything relies on BIOS for ASPM when POLICY_DEFAULT is set and I >> see above problem gets resolved. Also, the existing ASPM behavior >> does not have impact, unless specific BIOS does not disable ASPM on >> Root Port when device gets removed. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V8 5/5] PCI/ASPM: move link_state cleanup to bridge remove [not found] <1491627351-1111-1-git-send-email-okaya@codeaurora.org> ` (3 preceding siblings ...) 2017-04-08 4:55 ` [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Sinan Kaya @ 2017-04-08 4:55 ` Sinan Kaya 4 siblings, 0 replies; 27+ messages in thread From: Sinan Kaya @ 2017-04-08 4:55 UTC (permalink / raw) To: linux-pci, timur Cc: mayurkumar.patel, linux-arm-msm, linux-arm-kernel, Sinan Kaya, Bjorn Helgaas, Rajat Jain, Yinghai Lu, David Daney, Shawn Lin, Julia Lawall, linux-kernel For endpoints, change pcie_aspm_exit_link_state() so it cleans up the device's own state and disables ASPM if necessary, but doesn't remove the parent's link_state. For bridges, change pcie_aspm_exit_link_state() so it frees the bridge's own link_state. Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/pci/pcie/aspm.c | 20 +++++++++++++++----- drivers/pci/remove.c | 3 +-- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index c7da087..d99fa3f 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -972,6 +972,21 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) struct pci_dev *parent = pdev->bus->self; struct pcie_link_state *link, *root, *parent_link; + if (pdev->has_secondary_link) { + link = pdev->link_state; + down_read(&pci_bus_sem); + mutex_lock(&aspm_lock); + + list_del(&link->sibling); + list_del(&link->link); + + /* Clock PM is for endpoint device */ + free_link_state(link); + mutex_unlock(&aspm_lock); + up_read(&pci_bus_sem); + return; + } + if (!parent || !parent->link_state) return; @@ -990,11 +1005,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) /* All functions are removed, so just disable ASPM for the link */ pcie_config_aspm_link(link, 0); - list_del(&link->sibling); - list_del(&link->link); - /* Clock PM is for endpoint device */ - free_link_state(link); - /* Recheck latencies and configure upstream links */ if (parent_link) { pcie_update_aspm_capable(root); diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 73a03d3..7e14ebd 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -25,8 +25,7 @@ static void pci_stop_dev(struct pci_dev *dev) dev->is_added = 0; } - if (dev->bus->self) - pcie_aspm_exit_link_state(dev); + pcie_aspm_exit_link_state(dev); } static void pci_destroy_dev(struct pci_dev *dev) -- 1.9.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
end of thread, other threads:[~2017-05-15 9:10 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1491627351-1111-1-git-send-email-okaya@codeaurora.org> 2017-04-08 4:55 ` [PATCH V8 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() Sinan Kaya 2017-04-13 20:51 ` Bjorn Helgaas 2017-04-14 19:10 ` Sinan Kaya 2017-04-08 4:55 ` [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two Sinan Kaya 2017-04-12 19:16 ` Rajat Jain 2017-04-13 18:25 ` Bjorn Helgaas 2017-04-14 19:10 ` Sinan Kaya 2017-04-08 4:55 ` [PATCH V8 3/5] PCI/ASPM: add init hook to device_add Sinan Kaya 2017-04-13 20:48 ` Bjorn Helgaas 2017-04-13 21:02 ` Bjorn Helgaas 2017-04-14 1:19 ` Sinan Kaya 2017-04-14 1:30 ` Bjorn Helgaas 2017-04-08 4:55 ` [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Sinan Kaya 2017-04-12 19:19 ` Rajat Jain 2017-04-14 19:12 ` Sinan Kaya 2017-04-14 21:44 ` Bjorn Helgaas 2017-04-14 22:17 ` Sinan Kaya 2017-04-17 16:38 ` Bjorn Helgaas 2017-04-17 17:50 ` Sinan Kaya 2017-04-21 7:46 ` Patel, Mayurkumar 2017-04-21 13:50 ` Sinan Kaya 2017-04-21 14:13 ` Patel, Mayurkumar 2017-04-25 18:45 ` Bjorn Helgaas 2017-05-02 12:02 ` Patel, Mayurkumar 2017-05-03 21:10 ` Bjorn Helgaas 2017-05-15 9:10 ` Patel, Mayurkumar 2017-04-08 4:55 ` [PATCH V8 5/5] PCI/ASPM: move link_state cleanup to bridge remove Sinan Kaya
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).