From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753322AbdJLQtA (ORCPT ); Thu, 12 Oct 2017 12:49:00 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:46344 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752795AbdJLQs6 (ORCPT ); Thu, 12 Oct 2017 12:48:58 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 40AEB60159 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=okaya@codeaurora.org Subject: Re: [PATCH 4/5] PCI: wait device ready after pci_pm_reset() To: Bjorn Helgaas Cc: "linux-pci@vger.kernel.org >> Linux PCI" , timur@codeaurora.org, alex.williamson@redhat.com, linux-arm-msm@vger.kernel.org, Bjorn Helgaas , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org References: <1506212218-29103-1-git-send-email-okaya@codeaurora.org> <1506212218-29103-4-git-send-email-okaya@codeaurora.org> <20171011220625.GV25517@bhelgaas-glaptop.roam.corp.google.com> From: Sinan Kaya Message-ID: <2702792d-e076-45a9-5b8a-82e1499cb624@codeaurora.org> Date: Thu, 12 Oct 2017 12:48:54 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171011220625.GV25517@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/11/2017 6:06 PM, Bjorn Helgaas wrote: >> static int pci_pm_reset(struct pci_dev *dev, int probe) >> { >> + unsigned int delay = dev->d3_delay; >> u16 csr; >> >> if (!dev->pm_cap || dev->dev_flags & PCI_DEV_FLAGS_NO_PM_RESET) >> @@ -3988,7 +3989,10 @@ static int pci_pm_reset(struct pci_dev *dev, int probe) >> pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr); >> pci_dev_d3_sleep(dev); >> >> - return 0; >> + if (delay < pci_pm_d3_delay) >> + delay = pci_pm_d3_delay; >> + >> + return pci_dev_wait(dev, "PM D3->D0", delay, 1000); > 1) Why do we wait up to 1 second here, when we wait up to 60 seconds > for the other methods? Can they all be the same? Maybe a #define for > it? I know you want to have similar behavior for systems that do and do not support CRS. That was the reason why I converted flr wait function to into dev_wait function. However, here is the problem: For systems that do not support CRS, there is no way of knowing whether we are reading 0xFFFFFFFF because the endpoint is not reachable due to an error like "it doesn't support this reset type" or if it is actually emitting a CRS. If one system has a problem with pm_reset, this code would add an unnecessary 1 second delay into the reset path. If I make it 60 it would be something like: 1. try reset method A 2. wait 60 seconds 3. try reset method B 4. wait 60 seconds. 5. try reset method C 6. wait 60 seconds This might end up being a regression on some system. I'm still leaning towards a wait only if we are observing a CRS. What's your thought on this? then the sequence would be. 1. try reset method A 2. if CRS pending, wait 60 seconds 3. try reset method B 4. if CRS pending, wait 60 seconds. 5. try reset method C 6. if CRS pending, wait 60 seconds > > 2) I don't really like the fact that we do the initial sleep one place > and then pass the length of that sleep here. It's hard to verify > they're the same and keep them in sync. I think the only thing you > use initial_wait for is to include that time in the dmesg messages. > Maybe we should just omit that time from the message and drop the > parameter? > This was for printing reasons like you spotted, I can certainly get rid of the initial_wait. -- 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.