Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <7d5de631-3ebc-01ee-69b8-ae105408cb59@intel.com>
Date:   Fri, 11 Mar 2022 19:38:15 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.7.0
Subject: Re: [PATCH 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM
Content-Language: en-US
To:     Rajvi Jingar <rajvi.jingar@intel.com>, <bhelgaas@google.com>
CC:     <david.e.box@linux.intel.com>, <linux-pci@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20220224202937.2414867-1-rajvi.jingar@intel.com>
 <20220224202937.2414867-2-rajvi.jingar@intel.com>
From:   "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Organization: Intel Technology Poland Sp. z o. o., KRS 101882, ul. Slowackiego
 173, 80-298 Gdansk
In-Reply-To: <20220224202937.2414867-2-rajvi.jingar@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MllqUThYS2hpTmE4TGlTZE5IVTc2NmQzM1dCeFQ0WHVlMGk3eXdJL0hJSVlC?=
 =?utf-8?B?K2VjRjF1d3VvRFJjZWRiWVo1bnV3endBMm9TZDRNYVlKMXVqdTE5anJ2Mm9P?=
 =?utf-8?B?T00yN3NOQit6NVJNOXRHbWpsVW4ySHc3T0NlekRiMTJCUGZOeGgzb3J1R2g1?=
 =?utf-8?B?RWdOeGN5RjRhd0NVcndvOEM1eUNWY3JRMGRibThQdGNaSUFlRTRBTE1yWk1O?=
 =?utf-8?B?d21nZ3k5aklmMWVqdnpjQ0xRMjIrMGZsNmd4dnJrdFAxNXM3Njhwb1dPMnZ0?=
 =?utf-8?B?QWcxUUloSXlGQXhZTnFxaDNCT29ZK2RsOWJ0ZE1lYTFUME5DNGpCTFFJUktn?=
 =?utf-8?B?Q3A0UUhneTZMNjlGSjNGWHNVSVluZTRQblF5eHBzUW5NMjc4NXVZbjg4S3g3?=
 =?utf-8?B?L2hiV1RITDUvQ3o2dUZhRkl4bFpjZlNORTFqdUo5WTNFd3JIQTVjVDRma2lX?=
 =?utf-8?B?d21EOFFjR1h1aUhLRTl3Tyt1NnVLZkhPaXhrWitUOXBXYkhpRFhlR01WV2Ns?=
 =?utf-8?B?NmRobHBya0pONTV2N0huSWdRNTJrQmJidGM5dUo1OXNlaXpvdUVoekNXK0Zj?=
 =?utf-8?B?THBtYXRqTVFWdkVWQWpQNks2UGdUY09FY2hINCs0QllTM2NpRXVKS09vRFFi?=
 =?utf-8?B?ZHBxamFObjBrSnVOTzB4NFZUUC9QQ2FUZHpwdkxCMVJaMFNRdGFuRlpSMFd3?=
 =?utf-8?B?OGlReFVrU1MwZmpGM3praTJlTkE5VWZlYVlsQUQzaitxRzdCL05NajFMSzVT?=
 =?utf-8?B?RlhwWXBNTXV4eGV0aHM2eVBYL1psTFoyQzh4Z2M3RTZPdmdLeCt3cTBHblBE?=
 =?utf-8?B?bjhteldyMlBMUXFqYW5yeUFoL2Z6OE9BTWhuRVZLMVQ4dzFmL2hnTVRWYS9G?=
 =?utf-8?B?TUdYeUZyNUJLZHU4ekg5ZUduK0IwWGpOTHpneENuYURKMTR0b0FlQlNmNjZs?=
 =?utf-8?B?UUxnODFTTnFheEsrMGRhZ3Zoa3VjMEJCTDhkWHVpMlQ5QU5mT1NHUnBWM01m?=
 =?utf-8?B?Ync1c1NUVWx4ekcyckhHRHpxcGk5RWNYY0RJbTlYU2Q4SFF3bjJLUFNpTTlD?=
 =?utf-8?B?NG95ZUNsT0tJZWd0U0gxTXhqK1c3NGgzZ1A0K2JvOWRKNmhjRW1XREZyNWN3?=
 =?utf-8?B?NlpmMUJENmxVTkxDZDhlVHpIaUMwWlpONkgreUE4NStaUEcrUFFhOWxmMVF3?=
 =?utf-8?B?OWxJK2Z2TzZyNFRSWTNPeWdLcHBOQlhpaDBVS1dWd2lHbzRSMGJ2ZjNrK3NT?=
 =?utf-8?B?OTk3cW9HMmJ5d0VpK3ZVbkJEUUhFVVpMMHlnd0owYjd4OVdCeEhWRGRYaHlm?=
 =?utf-8?B?M21IdFFvMGtndlZEOE9Ca3graTNPMGhZclYxVGJhMU9XUmVrVmxNSFhGYUpq?=
 =?utf-8?B?L2EwMHpucjcwZ0cvZVA0RVFzN1VTYk9WOVBQMWlLS1FaU2ZmWHpCZnMxczBG?=
 =?utf-8?B?eWdYWXpsOGViUWlBYzg4dEtKQkc1QUVhTUt4YXZMOUEzOWZ6d0hRSTM5R01N?=
 =?utf-8?B?dU1qbUJtVU9TSlRWWUJLQklJdk9PWi9WbDdVQjZYc2lIRFRtNnVPdEZma3F5?=
 =?utf-8?B?ZjVsWC94Zzh0TmdNdGlJMFlHOFJ2ZVo0T1puRE5TZ3VhZ2h3NlpSdDZ1YjUr?=
 =?utf-8?B?azdIWjVpYm1raUt0ZU4xd3pMTnZNWVlDVVE0ci9vTUpxbnVoNnlRd1RPWU9B?=
 =?utf-8?B?ZlFkUnFMTCt4S0Z6YWJoMUNxT0NKd0JpY0JxVjFtSUVqRTJjeWpURVlGajAv?=
 =?utf-8?B?L3lGdVZ3UWNualNTTk16S0NnVmNjN1JoanJhNXlISlJmejlIZU1paUlPWkgv?=
 =?utf-8?B?cXRWK0tKb2VOblhTS3FoZTA4K092eExGcmxMQjQyYTZ1SWJ2VXdxL3FTeFhm?=
 =?utf-8?B?RGMxbU1Yb1N5c2V3eHRjOUt0Y2J5WERQeDVNbVZwcXFKbnkxemJMVnMvSVdE?=
 =?utf-8?B?OFpDWDhzYzhwaG9uamJuYU1yaER0SVppZk80dmg1MnZLdGV6Z3llZmlBMisx?=
 =?utf-8?B?OEZnUFBaeE1BPT0=?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 576b9929-96a7-4b05-49cb-08da038e5250
X-MS-Exchange-CrossTenant-AuthSource: MW5PR11MB5810.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2022 18:38:22.9939
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AQSmfMqRndh9U1a8xrPQWlSv5FqpGaBvex6Njv/jxa6TSSY7fgUwu85owM4AcdLhqaouhFO61HmOrGtGglBf3JIFJfJzOBS05fVfZFw6uyM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5034
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/24/2022 9:29 PM, Rajvi Jingar wrote:
> For the PCIe devices (like nvme) that do not go into D3 state still need to
> disable PTM on PCIe root ports to allow the port to enter a lower-power PM
> state and the SoC to reach a lower-power idle state as a whole. Move the
> pci_disable_ptm() out of pci_prepare_to_sleep() as this code path is not
> followed for devices that do not go into D3. This patch fixes the issue
> seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530 with Coffee
> Lake CPU platforms to get improved residency in low power idle states.
>
> Signed-off-by: Rajvi Jingar <rajvi.jingar@intel.com>
> Suggested-by: David E. Box <david.e.box@linux.intel.com>

I would add a Fixes tag pointing to the commit that introduced 
pci_disable_ptm().

Otherwise I agree with this change:

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>


> ---
>   drivers/pci/pci-driver.c | 11 +++++++++++
>   drivers/pci/pci.c        | 10 ----------
>   2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index ac3f7e1676a9..8be3f81afdf6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -836,6 +836,17 @@ static int pci_pm_suspend_noirq(struct device *dev)
>   
>   	if (!pci_dev->state_saved) {
>   		pci_save_state(pci_dev);
> +
> +		/*
> +		 * There are systems (for example, Intel mobile chips since Coffee
> +		 * Lake) where the power drawn while suspended can be significantly
> +		 * reduced by disabling PTM on PCIe root ports as this allows the
> +		 * port to enter a lower-power PM state and the SoC to reach a
> +		 * lower-power idle state as a whole.
> +		 */
> +		if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> +			pci_disable_ptm(pci_dev);
> +
>   		if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
>   			pci_prepare_to_sleep(pci_dev);
>   	}
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f..f8768672c064 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2660,16 +2660,6 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
>   	if (target_state == PCI_POWER_ERROR)
>   		return -EIO;
>   
> -	/*
> -	 * There are systems (for example, Intel mobile chips since Coffee
> -	 * Lake) where the power drawn while suspended can be significantly
> -	 * reduced by disabling PTM on PCIe root ports as this allows the
> -	 * port to enter a lower-power PM state and the SoC to reach a
> -	 * lower-power idle state as a whole.
> -	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> -		pci_disable_ptm(dev);
> -
>   	pci_enable_wake(dev, target_state, wakeup);
>   
>   	error = pci_set_power_state(dev, target_state);