From patchwork Fri Apr 21 07:46:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Patel, Mayurkumar" X-Patchwork-Id: 753181 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3w8SWL15lqz9s73 for ; Fri, 21 Apr 2017 17:47:26 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1036160AbdDUHqq (ORCPT ); Fri, 21 Apr 2017 03:46:46 -0400 Received: from mga07.intel.com ([134.134.136.100]:51013 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1036145AbdDUHqh (ORCPT ); Fri, 21 Apr 2017 03:46:37 -0400 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga105.jf.intel.com with ESMTP; 21 Apr 2017 00:46:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,229,1488873600"; d="scan'208";a="77126009" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by orsmga002.jf.intel.com with ESMTP; 21 Apr 2017 00:46:32 -0700 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.187]) by IRSMSX109.ger.corp.intel.com ([169.254.13.12]) with mapi id 14.03.0319.002; Fri, 21 Apr 2017 08:46:27 +0100 From: "Patel, Mayurkumar" To: Sinan Kaya , Bjorn Helgaas CC: Bjorn Helgaas , Rajat Jain , "Rajat Jain" , David Daney , "linux-pci@vger.kernel.org" , Timur Tabi , "linux-kernel@vger.kernel.org" , Julia Lawall , linux-arm-msm , Yinghai Lu , Shawn Lin , linux-arm , Myron Stowe Subject: RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Thread-Topic: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Thread-Index: AQHSsCR5lQf5ZNPCu0maCYKOKGfA0KHCEjuAgAMi3oCAACqMAIAACP0AgARYXoCAABQGgIAEPdjQ Date: Fri, 21 Apr 2017 07:46:27 +0000 Message-ID: <92EBB4272BF81E4089A7126EC1E7B2846676C7EF@IRSMSX101.ger.corp.intel.com> References: <1491627351-1111-1-git-send-email-okaya@codeaurora.org> <1491627351-1111-5-git-send-email-okaya@codeaurora.org> <20170414214452.GA21870@bhelgaas-glaptop.roam.corp.google.com> <66168dde-7719-6f74-3f06-8e4724dd2918@codeaurora.org> In-Reply-To: Accept-Language: de-DE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [163.33.239.180] MIME-Version: 1.0 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org 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? >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 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 */