Re: [BUGFIX -v3 3/4] PCI/PM: Fix config reg access for D3cold and bridge suspending

Huang Ying
2012-08-10T02:36:42Z

On Wed, 2012-08-08 at 09:07 +0800, 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 <bjorn@mork.no>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> ---
> drivers/pci/pci-sysfs.c | 43 +++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pcie/portdrv_pci.c | 9 ++++++++
> 2 files changed, 52 insertions(+)
>
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -458,6 +458,41 @@ 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);
> + if (pdev->current_state == PCI_D3cold) {
> + /*
> + * Already called pm_runtime_get_noresume above, so
> + * just calling pm_runtime_resume is sufficient, need
> + * not to call pm_runtime_get_sync.
> + */
> + pm_runtime_resume(dev);

Hi, Rafael,

Do you OK with this new version and the comments added above?

Best Regards,
Huang Ying

> + }
> +}
> +
> +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 +519,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 +566,8 @@ pci_read_config(struct file *filp, struc
> --size;
> }
>
> + pci_config_pm_runtime_put(dev);
> +
> return count;
> }
>
> @@ -549,6 +588,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 +628,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)