From: Sinan Kaya <okaya@codeaurora.org> To: Bjorn Helgaas <bhelgaas@google.com> Cc: Bjorn Helgaas <helgaas@kernel.org>, Rajat Jain <rajatja@google.com>, "Patel, Mayurkumar" <mayurkumar.patel@intel.com>, Rajat Jain <rajatxjain@gmail.com>, David Daney <david.daney@cavium.com>, "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, Timur Tabi <timur@codeaurora.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Julia Lawall <Julia.Lawall@lip6.fr>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, Yinghai Lu <yinghai@kernel.org>, Shawn Lin <shawn.lin@rock-chips.com>, linux-arm <linux-arm-kernel@lists.infradead.org>, Myron Stowe <myron.stowe@redhat.com> Subject: Re: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Date: Mon, 17 Apr 2017 13:50:01 -0400 [thread overview] Message-ID: <c0c2f48f-d725-058e-a1ad-92ab4b6e7fb3@codeaurora.org> (raw) In-Reply-To: <CAErSpo7p_2dTDTC7J4HdQUw_5GH8081dUZ=XVqd=sRLrA_FJnA@mail.gmail.com> 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.
WARNING: multiple messages have this Message-ID (diff)
From: okaya@codeaurora.org (Sinan Kaya) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Date: Mon, 17 Apr 2017 13:50:01 -0400 [thread overview] Message-ID: <c0c2f48f-d725-058e-a1ad-92ab4b6e7fb3@codeaurora.org> (raw) In-Reply-To: <CAErSpo7p_2dTDTC7J4HdQUw_5GH8081dUZ=XVqd=sRLrA_FJnA@mail.gmail.com> 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.
next prev parent reply other threads:[~2017-04-17 17:50 UTC|newest] Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-04-08 4:55 [PATCH V8 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT Sinan Kaya 2017-04-08 4:55 ` Sinan Kaya 2017-04-08 4:55 ` Sinan Kaya 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-08 4:55 ` Sinan Kaya 2017-04-13 20:51 ` Bjorn Helgaas 2017-04-13 20:51 ` Bjorn Helgaas 2017-04-14 19:10 ` Sinan Kaya 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-08 4:55 ` Sinan Kaya 2017-04-08 4:55 ` Sinan Kaya 2017-04-12 19:16 ` Rajat Jain 2017-04-12 19:16 ` Rajat Jain 2017-04-13 18:25 ` Bjorn Helgaas 2017-04-13 18:25 ` Bjorn Helgaas 2017-04-13 18:25 ` Bjorn Helgaas 2017-04-14 19:10 ` Sinan Kaya 2017-04-14 19:10 ` Sinan Kaya 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-08 4:55 ` Sinan Kaya 2017-04-08 4:55 ` Sinan Kaya 2017-04-13 20:48 ` Bjorn Helgaas 2017-04-13 20:48 ` Bjorn Helgaas 2017-04-13 20:48 ` Bjorn Helgaas 2017-04-13 21:02 ` Bjorn Helgaas 2017-04-13 21:02 ` Bjorn Helgaas 2017-04-13 21:02 ` Bjorn Helgaas 2017-04-14 1:19 ` Sinan Kaya 2017-04-14 1:19 ` Sinan Kaya 2017-04-14 1:30 ` Bjorn Helgaas 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-08 4:55 ` Sinan Kaya 2017-04-08 4:55 ` Sinan Kaya 2017-04-12 19:19 ` Rajat Jain 2017-04-12 19:19 ` Rajat Jain 2017-04-12 19:19 ` Rajat Jain 2017-04-14 19:12 ` Sinan Kaya 2017-04-14 19:12 ` Sinan Kaya 2017-04-14 19:12 ` Sinan Kaya 2017-04-14 21:44 ` Bjorn Helgaas 2017-04-14 21:44 ` Bjorn Helgaas 2017-04-14 21:44 ` Bjorn Helgaas 2017-04-14 22:17 ` Sinan Kaya 2017-04-14 22:17 ` Sinan Kaya 2017-04-17 16:38 ` Bjorn Helgaas 2017-04-17 16:38 ` Bjorn Helgaas 2017-04-17 17:50 ` Sinan Kaya [this message] 2017-04-17 17:50 ` Sinan Kaya 2017-04-21 7:46 ` Patel, Mayurkumar 2017-04-21 7:46 ` Patel, Mayurkumar 2017-04-21 7:46 ` Patel, Mayurkumar 2017-04-21 7:46 ` Patel, Mayurkumar 2017-04-21 13:50 ` Sinan Kaya 2017-04-21 13:50 ` Sinan Kaya 2017-04-21 14:13 ` Patel, Mayurkumar 2017-04-21 14:13 ` Patel, Mayurkumar 2017-04-21 14:13 ` Patel, Mayurkumar 2017-04-21 14:13 ` Patel, Mayurkumar 2017-04-25 18:45 ` Bjorn Helgaas 2017-04-25 18:45 ` Bjorn Helgaas 2017-05-02 12:02 ` Patel, Mayurkumar 2017-05-02 12:02 ` Patel, Mayurkumar 2017-05-02 12:02 ` Patel, Mayurkumar 2017-05-03 21:10 ` Bjorn Helgaas 2017-05-03 21:10 ` Bjorn Helgaas 2017-05-03 21:10 ` Bjorn Helgaas 2017-05-15 9:10 ` Patel, Mayurkumar 2017-05-15 9:10 ` Patel, Mayurkumar 2017-05-15 9:10 ` Patel, Mayurkumar 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 2017-04-08 4:55 ` Sinan Kaya 2017-04-08 4:55 ` Sinan Kaya 2017-04-10 11:37 ` [PATCH V8 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT Patel, Mayurkumar 2017-04-10 11:37 ` Patel, Mayurkumar 2017-04-10 11:37 ` Patel, Mayurkumar 2017-04-10 13:07 ` Sinan Kaya 2017-04-10 13:07 ` Sinan Kaya 2017-04-10 13:07 ` Sinan Kaya 2017-04-10 13:11 ` Patel, Mayurkumar 2017-04-10 13:11 ` Patel, Mayurkumar 2017-04-10 13:11 ` Patel, Mayurkumar 2017-04-11 21:19 ` Bjorn Helgaas 2017-04-11 21:19 ` Bjorn Helgaas 2017-04-11 21:19 ` Bjorn Helgaas 2017-04-11 21:27 ` Sinan Kaya 2017-04-11 21:27 ` Sinan Kaya 2017-04-11 22:41 ` Bjorn Helgaas 2017-04-11 22:41 ` Bjorn Helgaas 2017-04-11 22:41 ` Bjorn Helgaas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c0c2f48f-d725-058e-a1ad-92ab4b6e7fb3@codeaurora.org \ --to=okaya@codeaurora.org \ --cc=Julia.Lawall@lip6.fr \ --cc=bhelgaas@google.com \ --cc=david.daney@cavium.com \ --cc=helgaas@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=mayurkumar.patel@intel.com \ --cc=myron.stowe@redhat.com \ --cc=rajatja@google.com \ --cc=rajatxjain@gmail.com \ --cc=shawn.lin@rock-chips.com \ --cc=timur@codeaurora.org \ --cc=yinghai@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.