* [PATCH V3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT @ 2017-03-08 20:39 Sinan Kaya 2017-03-09 22:27 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Sinan Kaya @ 2017-03-08 20:39 UTC (permalink / raw) To: linux-pci, timur Cc: Sinan Kaya, linux-arm-msm, Mayurkumar Patel, open list, linux-arm-kernel When the operating system is booted with the default ASPM policy (POLICY_DEFAULT), current code is querying the enable/disable states from ASPM registers to determine the policy. For example, a BIOS could set the power saving state to performance and clear all ASPM control registers. A balanced ASPM policy could enable L0s and disable L1. A power conscious BIOS could enable both L0s and L1 to trade off latency and performance vs. power. After hotplug removal, pcie_aspm_exit_link_state() function clears the ASPM registers. An insertion following hotplug removal reads incorrect policy as ASPM disabled even though ASPM was enabled during boot. This is caused by the fact that same function is used for reconfiguring ASPM regardless of the of the power on state. This patch builds some state info into pcie_aspm_cap_init() function. Adding a flag and a counter to the struct pci_dev to save the power up policy in the bridge during boot. ASPM enable counter is used as a switch to determine when to use saved value. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> --- drivers/pci/pcie/aspm.c | 21 +++++++++++++++++---- include/linux/pci.h | 2 ++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 17ac1dc..63435b0 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -338,8 +338,9 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) } } -static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) +static void pcie_aspm_cap_init(struct pci_dev *pdev, int blacklist) { + struct pcie_link_state *link = pdev->link_state; struct pci_dev *child, *parent = link->pdev; struct pci_bus *linkbus = parent->subordinate; struct aspm_register_info upreg, dwreg; @@ -397,8 +398,20 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1); link->latency_dw.l1 = calc_l1_latency(dwreg.latency_encoding_l1); - /* Save default state */ - link->aspm_default = link->aspm_enabled; + /* + * Save default state from FW when enabling ASPM for the first time + * during boot by looking at the calculated link->aspm_enabled bits + * above and aspm_enable_cnt will be zero. + * + * If this path is getting called for the second/third time + * (aspm_enable_cnt will be non-zero). Assume that the current state + * of the ASPM registers may not necessarily match what FW asked us to + * do as in the case of hotplug insertion/removal. + */ + if (atomic_inc_return(&pdev->aspm_enable_cnt) == 1) + pdev->aspm_default = link->aspm_default = link->aspm_enabled; + else + link->aspm_default = pdev->aspm_default; /* Setup initial capable state. Will be updated later */ link->aspm_capable = link->aspm_support; @@ -599,7 +612,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) * upstream links also because capable state of them can be * update through pcie_aspm_cap_init(). */ - pcie_aspm_cap_init(link, blacklist); + pcie_aspm_cap_init(pdev, blacklist); /* Setup initial Clock PM state */ pcie_clkpm_cap_init(link, blacklist); diff --git a/include/linux/pci.h b/include/linux/pci.h index e2d1a12..aa7bd7e 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -321,6 +321,8 @@ struct pci_dev { #ifdef CONFIG_PCIEASPM struct pcie_link_state *link_state; /* ASPM link state */ + unsigned int aspm_default; /* ASPM policy set by BIOS */ + atomic_t aspm_enable_cnt; /* ASPM policy initialization */ #endif pci_channel_state_t error_state; /* current connectivity state */ -- 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 related [flat|nested] 5+ messages in thread
* Re: [PATCH V3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT 2017-03-08 20:39 [PATCH V3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT Sinan Kaya @ 2017-03-09 22:27 ` Bjorn Helgaas 2017-03-10 14:36 ` Sinan Kaya 2017-03-13 21:06 ` Sinan Kaya 0 siblings, 2 replies; 5+ messages in thread From: Bjorn Helgaas @ 2017-03-09 22:27 UTC (permalink / raw) To: Sinan Kaya Cc: linux-pci, timur, linux-arm-msm, Mayurkumar Patel, open list, linux-arm-kernel Hi Sinan, On Wed, Mar 08, 2017 at 03:39:11PM -0500, Sinan Kaya wrote: > When the operating system is booted with the default ASPM policy > (POLICY_DEFAULT), current code is querying the enable/disable > states from ASPM registers to determine the policy. > > For example, a BIOS could set the power saving state to performance > and clear all ASPM control registers. A balanced ASPM policy could > enable L0s and disable L1. A power conscious BIOS could enable both > L0s and L1 to trade off latency and performance vs. power. > > After hotplug removal, pcie_aspm_exit_link_state() function clears > the ASPM registers. An insertion following hotplug removal reads > incorrect policy as ASPM disabled even though ASPM was enabled > during boot. > > This is caused by the fact that same function is used for reconfiguring > ASPM regardless of the of the power on state. > > This patch builds some state info into pcie_aspm_cap_init() function. > > Adding a flag and a counter to the struct pci_dev to save the > power up policy in the bridge during boot. ASPM enable counter is > used as a switch to determine when to use saved value. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com> > --- > drivers/pci/pcie/aspm.c | 21 +++++++++++++++++---- > include/linux/pci.h | 2 ++ > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 17ac1dc..63435b0 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -338,8 +338,9 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) > } > } > > -static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) > +static void pcie_aspm_cap_init(struct pci_dev *pdev, int blacklist) > { > + struct pcie_link_state *link = pdev->link_state; > struct pci_dev *child, *parent = link->pdev; > struct pci_bus *linkbus = parent->subordinate; > struct aspm_register_info upreg, dwreg; > @@ -397,8 +398,20 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) > link->latency_up.l1 = calc_l1_latency(upreg.latency_encoding_l1); > link->latency_dw.l1 = calc_l1_latency(dwreg.latency_encoding_l1); > > - /* Save default state */ > - link->aspm_default = link->aspm_enabled; > + /* > + * Save default state from FW when enabling ASPM for the first time > + * during boot by looking at the calculated link->aspm_enabled bits > + * above and aspm_enable_cnt will be zero. > + * > + * If this path is getting called for the second/third time > + * (aspm_enable_cnt will be non-zero). Assume that the current state > + * of the ASPM registers may not necessarily match what FW asked us to > + * do as in the case of hotplug insertion/removal. > + */ > + if (atomic_inc_return(&pdev->aspm_enable_cnt) == 1) > + pdev->aspm_default = link->aspm_default = link->aspm_enabled; > + else > + link->aspm_default = pdev->aspm_default; This is an aspect of the ASPM design that I don't like: - pcie_aspm_init_link_state() is called on a *bridge* after we've enumerated any devices below the bridge, and we allocate the link_state. - pcie_aspm_exit_link_state() is called on an *endpoint*, and if we're removing the last endpoint below a bridge, we release the *parent's* link_state. This leads to the problem you're trying to solve here: we throw away all the bridge's ASPM information when the child is removed, so we lose the original settings from firmware. Your solution is to add some state outside the link_state structure, and initialize it the first time we enable ASPM for the link. With the current design, you don't really have much choice, but I think it ends up being more complicated than it should be. How hard do you think it would be to rework this path slightly so we: - call pcie_aspm_init_link_state() for every device, maybe from pci_init_capabilities() - for bridges, have pcie_aspm_init_link_state() allocate a link_state, regardless of whether it currently has any children, and save the ASPM settings done by firmware - for endpoints, have pcie_aspm_init_link_state() do the actual ASPM setup of the link as it currently does - 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 ? > /* Setup initial capable state. Will be updated later */ > link->aspm_capable = link->aspm_support; > @@ -599,7 +612,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) > * upstream links also because capable state of them can be > * update through pcie_aspm_cap_init(). > */ > - pcie_aspm_cap_init(link, blacklist); > + pcie_aspm_cap_init(pdev, blacklist); > > /* Setup initial Clock PM state */ > pcie_clkpm_cap_init(link, blacklist); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index e2d1a12..aa7bd7e 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -321,6 +321,8 @@ struct pci_dev { > > #ifdef CONFIG_PCIEASPM > struct pcie_link_state *link_state; /* ASPM link state */ > + unsigned int aspm_default; /* ASPM policy set by BIOS */ > + atomic_t aspm_enable_cnt; /* ASPM policy initialization */ > #endif > > pci_channel_state_t error_state; /* current connectivity state */ > -- > 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] 5+ messages in thread
* Re: [PATCH V3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT 2017-03-09 22:27 ` Bjorn Helgaas @ 2017-03-10 14:36 ` Sinan Kaya 2017-03-13 8:49 ` Patel, Mayurkumar 2017-03-13 21:06 ` Sinan Kaya 1 sibling, 1 reply; 5+ messages in thread From: Sinan Kaya @ 2017-03-10 14:36 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, timur, linux-arm-msm, Mayurkumar Patel, open list, linux-arm-kernel Hi Bjorn, On 3/9/2017 5:27 PM, Bjorn Helgaas wrote: > How hard do you think it would be to rework this path slightly so we: > > - call pcie_aspm_init_link_state() for every device, maybe from > pci_init_capabilities() > > - for bridges, have pcie_aspm_init_link_state() allocate a > link_state, regardless of whether it currently has any children, > and save the ASPM settings done by firmware > > - for endpoints, have pcie_aspm_init_link_state() do the actual ASPM > setup of the link as it currently does > > - 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 Thanks for the feedback, Let me take a stab at 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] 5+ messages in thread
* RE: [PATCH V3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT 2017-03-10 14:36 ` Sinan Kaya @ 2017-03-13 8:49 ` Patel, Mayurkumar 0 siblings, 0 replies; 5+ messages in thread From: Patel, Mayurkumar @ 2017-03-13 8:49 UTC (permalink / raw) To: Sinan Kaya, Bjorn Helgaas Cc: linux-pci, timur, linux-arm-msm, open list, linux-arm-kernel > >Hi Bjorn, > >On 3/9/2017 5:27 PM, Bjorn Helgaas wrote: >> How hard do you think it would be to rework this path slightly so we: >> >> - call pcie_aspm_init_link_state() for every device, maybe from >> pci_init_capabilities() >> >> - for bridges, have pcie_aspm_init_link_state() allocate a >> link_state, regardless of whether it currently has any children, >> and save the ASPM settings done by firmware >> >> - for endpoints, have pcie_aspm_init_link_state() do the actual ASPM >> setup of the link as it currently does >> >> - 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 > >Thanks for the feedback, Let me take a stab at this. >Sinan Thanks Bjorn and Sinan. I will wait for new patch-sets from Sinan. > >-- >Sinan Kaya >Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technol= ogies, 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] 5+ messages in thread
* Re: [PATCH V3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT 2017-03-09 22:27 ` Bjorn Helgaas 2017-03-10 14:36 ` Sinan Kaya @ 2017-03-13 21:06 ` Sinan Kaya 1 sibling, 0 replies; 5+ messages in thread From: Sinan Kaya @ 2017-03-13 21:06 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, timur, linux-arm-msm, Mayurkumar Patel, open list, linux-arm-kernel Hi Bjorn, On 3/9/2017 5:27 PM, Bjorn Helgaas wrote: > This is an aspect of the ASPM design that I don't like: > > - pcie_aspm_init_link_state() is called on a *bridge* after we've > enumerated any devices below the bridge, and we allocate the > link_state. > > - pcie_aspm_exit_link_state() is called on an *endpoint*, and if > we're removing the last endpoint below a bridge, we release the > *parent's* link_state. I just posted V4 a minute ago. I divided ASPM init into two so that the ASPM registers are captured in device_add path. There is a section of the ASPM init code that needs to walk all the devices under the bridge. This section of the code is not working in device_add path as the link list for child devices has not been set up yet. Unless you tell me there is a better way to handle this, I don't see how we can remove the call from scan_slot. After further review, I think I can split PATCH V4 3/3 into two. Before diving into too much clean up, I wanted to get some feedback as it is obvious from the messed up cover letter. 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] 5+ messages in thread
end of thread, other threads:[~2017-03-13 21:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-08 20:39 [PATCH V3] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT Sinan Kaya 2017-03-09 22:27 ` Bjorn Helgaas 2017-03-10 14:36 ` Sinan Kaya 2017-03-13 8:49 ` Patel, Mayurkumar 2017-03-13 21:06 ` 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).