From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ogre.sisk.pl ([193.178.161.156]:40913 "EHLO ogre.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932310Ab2HOUEj (ORCPT ); Wed, 15 Aug 2012 16:04:39 -0400 From: "Rafael J. Wysocki" To: Huang Ying Subject: Re: [UPDATE][PATCH -v3 3/4] PCI/PM: Fix config reg access for D3cold and bridge suspending Date: Wed, 15 Aug 2012 22:10:35 +0200 Cc: Bjorn Helgaas , linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-pm@vger.kernel.org, Alan Stern References: <1344994983-9092-1-git-send-email-ying.huang@intel.com> In-Reply-To: <1344994983-9092-1-git-send-email-ying.huang@intel.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-2" Message-Id: <201208152210.35938.rjw@sisk.pl> Sender: linux-pci-owner@vger.kernel.org List-ID: On Wednesday, August 15, 2012, Huang Ying wrote: > This patch fixes the following bug: > > http://marc.info/?l=linux-pci&m=134338059022620&w=2 > > Where lspci does not work properly if a device and the corresponding > parent bridge (such as PCIe port) is suspended. This is because the > device configuration space registers will be not accessible if the > corresponding parent bridge is suspended or the device is put into > D3cold state. > > To solve the issue, the bridge/PCIe port connected to the device is > put into active state before read/write configuration space registers. > If the device is in D3cold state, it will be put into active state > too. > > To avoid resume/suspend PCIe port for each configuration register > read/write, a small delay is added before the PCIe port to go > suspended. > > Reported-by: Bjorn Mork > Signed-off-by: Huang Ying Reviewed-by: Rafael J. Wysocki Although the delay might have been #defined. > --- > drivers/pci/pci-sysfs.c | 42 +++++++++++++++++++++++++++++++++++++++++ > drivers/pci/pcie/portdrv_pci.c | 9 ++++++++ > 2 files changed, 51 insertions(+) > > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -458,6 +458,40 @@ boot_vga_show(struct device *dev, struct > } > struct device_attribute vga_attr = __ATTR_RO(boot_vga); > > +static void > +pci_config_pm_runtime_get(struct pci_dev *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device *parent = dev->parent; > + > + if (parent) > + pm_runtime_get_sync(parent); > + pm_runtime_get_noresume(dev); > + /* > + * pdev->current_state is set to PCI_D3cold during suspending, > + * so wait until suspending completes > + */ > + pm_runtime_barrier(dev); > + /* > + * Only need to resume devices in D3cold, because config > + * registers are still accessible for devices suspended but > + * not in D3cold. > + */ > + if (pdev->current_state == PCI_D3cold) > + pm_runtime_resume(dev); > +} > + > +static void > +pci_config_pm_runtime_put(struct pci_dev *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device *parent = dev->parent; > + > + pm_runtime_put(dev); > + if (parent) > + pm_runtime_put_sync(parent); > +} > + > static ssize_t > pci_read_config(struct file *filp, struct kobject *kobj, > struct bin_attribute *bin_attr, > @@ -484,6 +518,8 @@ pci_read_config(struct file *filp, struc > size = count; > } > > + pci_config_pm_runtime_get(dev); > + > if ((off & 1) && size) { > u8 val; > pci_user_read_config_byte(dev, off, &val); > @@ -529,6 +565,8 @@ pci_read_config(struct file *filp, struc > --size; > } > > + pci_config_pm_runtime_put(dev); > + > return count; > } > > @@ -549,6 +587,8 @@ pci_write_config(struct file* filp, stru > count = size; > } > > + pci_config_pm_runtime_get(dev); > + > if ((off & 1) && size) { > pci_user_write_config_byte(dev, off, data[off - init_off]); > off++; > @@ -587,6 +627,8 @@ pci_write_config(struct file* filp, stru > --size; > } > > + pci_config_pm_runtime_put(dev); > + > return count; > } > > --- a/drivers/pci/pcie/portdrv_pci.c > +++ b/drivers/pci/pcie/portdrv_pci.c > @@ -140,9 +140,17 @@ static int pcie_port_runtime_resume(stru > { > return 0; > } > + > +static int pcie_port_runtime_idle(struct device *dev) > +{ > + /* Delay for a short while to prevent too frequent suspend/resume */ > + pm_schedule_suspend(dev, 10); > + return -EBUSY; > +} > #else > #define pcie_port_runtime_suspend NULL > #define pcie_port_runtime_resume NULL > +#define pcie_port_runtime_idle NULL > #endif > > static const struct dev_pm_ops pcie_portdrv_pm_ops = { > @@ -155,6 +163,7 @@ static const struct dev_pm_ops pcie_port > .resume_noirq = pcie_port_resume_noirq, > .runtime_suspend = pcie_port_runtime_suspend, > .runtime_resume = pcie_port_runtime_resume, > + .runtime_idle = pcie_port_runtime_idle, > }; > > #define PCIE_PORTDRV_PM_OPS (&pcie_portdrv_pm_ops) > >