PCI: add hibernation hooks

Hello Bjorn,

I'm currently trying to implement hibernate support for pci on s390. To access the config space of a pci function on s390 the function has to be enabled first. So I need some kind of callback to achieve this before the pci core or the device driver tries to access the function after a resume from hibernate.

Would you be OK with the following patch?

---
PCI: add hibernation hooks

Platforms may want to provide architecture-specific functionality when a PCI device is doing a hibernate transition. Add a weak symbol pcibios_pm_ops that architectures can override to do so.

Signed-off-by Sebastian Ott <[email protected]>

---
 drivers/pci/pci-driver.c |   30 ++++++++++++++++++++++++++++++
 include/linux/pci.h      |    4 ++++
 2 files changed, 34 insertions(+)

drivers/pci/pci-driver.c +20 -0

        return 0;
 }
 

@@ -811,6 +821,9 @@ static int pci_pm_freeze_noirq(struct de
 
        pci_pm_set_unknown_state(pci_dev);
 
+       if (pcibios_pm_ops.freeze_noirq)
+               pcibios_pm_ops.freeze_noirq(dev);
+
        return 0;
 }
 

@@ -820,6 +833,9 @@ static int pci_pm_thaw_noirq(struct devi
        struct device_driver *drv = dev->driver;
        int error = 0;
 
+       if (pcibios_pm_ops.thaw_noirq)
+               pcibios_pm_ops.thaw_noirq(dev);
+
        if (pci_has_legacy_pm_support(pci_dev))
                return pci_legacy_resume_early(dev);
 

@@ -837,6 +853,9 @@ static int pci_pm_thaw(struct device *de
        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
        int error = 0;
 
+       if (pcibios_pm_ops.thaw)
+               pcibios_pm_ops.thaw(dev);
+
        if (pci_has_legacy_pm_support(pci_dev))
                return pci_legacy_resume(dev);
 

@@ -878,6 +897,9 @@ static int pci_pm_poweroff(struct device
  Fixup:
        pci_fixup_device(pci_fixup_suspend, pci_dev);
 
+       if (pcibios_pm_ops.poweroff)
+               pcibios_pm_ops.poweroff(dev);
+
        return 0;
 }
 

@@ -911,6 +933,9 @@ static int pci_pm_poweroff_noirq(struct
        if (pci_dev->class == PCI_CLASS_SERIAL_USB_EHCI)
                pci_write_config_word(pci_dev, PCI_COMMAND, 0);
 
+       if (pcibios_pm_ops.poweroff_noirq)
+               pcibios_pm_ops.poweroff_noirq(dev);
+
        return 0;
 }
 

@@ -920,6 +945,9 @@ static int pci_pm_restore_noirq(struct d
        struct device_driver *drv = dev->driver;
        int error = 0;
 
+       if (pcibios_pm_ops.restore_noirq)
+               pcibios_pm_ops.restore_noirq(dev);
+
        pci_pm_default_resume_early(pci_dev);
 
        if (pci_has_legacy_pm_support(pci_dev))

@@ -937,6 +965,8 @@ static int pci_pm_restore(struct device
        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
        int error = 0;
 
+       if (pcibios_pm_ops.restore)
+               pcibios_pm_ops.restore(dev);
        /*
         * This is necessary for the hibernation error path in which restore is
         * called without restoring the standard config registers of the device.

include/linux/pci.h +4 -0

@@ -1648,6 +1648,10 @@ int pcibios_set_pcie_reset_state(struct
 int pcibios_add_device(struct pci_dev *dev);
 void pcibios_release_device(struct pci_dev *dev);
 
+#ifdef CONFIG_HIBERNATE_CALLBACKS
+extern struct dev_pm_ops pcibios_pm_ops;
+#endif
+
 #ifdef CONFIG_PCI_MMCONFIG
 void __init pci_mmcfg_early_init(void);
 void __init pci_mmcfg_late_init(void);

Quoted 8 lines · Sebastian Ott

Hello Bjorn,

I'm currently trying to implement hibernate support for pci on s390. To access the config space of a pci function on s390 the function has to be enabled first. So I need some kind of callback to achieve this before the pci core or the device driver tries to access the function after a resume from hibernate.

Would you be OK with the following patch?

I think doing something like this is fine.

What would it look like if we provided empty weak definitions of the eight functions you need to hook: pcibios_pm_freeze(), pcibios_pm_freeze_noirq(), etc.? Then the callers wouldn't have to do a test before making the call. Not sure which would be better, but it might be more obvious that this is a build-time binding rather than a dynamic binding.

Should we check return values from the arch code?

Quoted 2 lines · Sebastian Ott

Hello Bjorn,

Quoted 6 lines

I'm currently trying to implement hibernate support for pci on s390. To access the config space of a pci function on s390 the function has to be enabled first. So I need some kind of callback to achieve this before the pci core or the device driver tries to access the function after a resume from hibernate.

Would you be OK with the following patch?

I think doing something like this is fine.

Quoted 9 lines · Bjorn Helgaas

Quoted 7 lines · Sebastian Ott

Hello Bjorn,

I'm currently trying to implement hibernate support for pci on s390. To access the config space of a pci function on s390 the function has to be enabled first. So I need some kind of callback to achieve this before the pci core or the device driver tries to access the function after a resume from hibernate.

Quoted 3 lines

Quoted 1 line

Would you be OK with the following patch?

I think doing something like this is fine.

Well, since we seem to be doing everything using __weak functions in the PCI land like in this patch, shouldn't we drop pci_platform_pm and use __weak functions for that stuff too?

Quoted 12 lines · Rafael J. Wysocki

Quoted 11 lines · Bjorn Helgaas

Quoted 8 lines · Sebastian Ott

Hello Bjorn,

I'm currently trying to implement hibernate support for pci on s390. To access the config space of a pci function on s390 the function has to be enabled first. So I need some kind of callback to achieve this before the pci core or the device driver tries to access the function after a resume from hibernate.

Would you be OK with the following patch?

Quoted 5 lines

Quoted 1 line

I think doing something like this is fine.

Well, since we seem to be doing everything using __weak functions in the PCI land like in this patch, shouldn't we drop pci_platform_pm and use __weak functions for that stuff too?

Well, we could consider that, if somebody wanted to do the work, but I don't think we need to tie it to Sebastian's change.

It's hard to tell over email, but it sounds like you have some reluctance to using weak functions? PCI has long used the pcibios_*() style for arch-dependent code. Before we used weak functions, every arch had to implement every pcibios_*() interface, whether the arch needed it or not. Using weak functions just means when an arch doesn't need anything special in pcibios_foo(), it can do nothing at all and automatically get the weak definition.

For Sebastian's changes, the question is whether we want this:

struct dev_pm_ops __weak pcibios_pm_ops;

static int pci_pm_thaw(struct device *dev)

  {
    ...
    if (pcibios_pm_ops.thaw) {
      error = pcibios_pm_ops.thaw(pci_dev)
      if (error)
        return error;
    }
    ...

}
or this:

int __weak pcibios_pm_thaw(struct pci_dev *pdev) { return 0; }

  static int pci_pm_thaw(struct device *dev)
  {
    ...
    error = pcibios_pm_thaw(pci_dev);
    if (error)
      return error;
    ...
  }

I prefer the latter because there's no need to repeat the "pcibios_pm_ops.thaw" reference and it seems more straightforward to read.

But it does have the disadvantage of requiring a weak definition for every hook, instead of a single weak pcibios_pm_ops definition.

There's also the x86 pcibios_enable_irq() style, where we declare "extern int (*pcibios_enable_irq)(struct pci_dev *dev)". The call looks the same as using a weak function, but you can change the pointer at run-time. Run-time control isn't needed here, so it seems overkill to me, but it is a possibility, e.g., make pcibios_pm_thaw() a pointer that defaults to an empty implementation, and arches could override by reassigning it.

-> the prevailing convention for representing platform options like that seems to be to use a pointer to an object containing callback pointers. This is what we do for pci_platform_pm and for suspend_ops, hibernation_ops etc. I think it is just more flexible, because it allows the pointer to be left unset if there is an initialization error of some sort or a command line option disabling something is set etc.

Quoted 6 lines · Rafael J. Wysocki

-> the prevailing convention for representing platform options like that seems to be to use a pointer to an object containing callback pointers. This is what we do for pci_platform_pm and for suspend_ops, hibernation_ops etc. I think it is just more flexible, because it allows the pointer to be left unset if there is an initialization error of some sort or a command line option disabling something is set etc.

Good points. I put merged the error checking into the original patch and put it in my pci/misc branch. Here's the result; let me know if anything needs to be tweaked.

PCI: Add pcibios_pm_ops for optional arch-specific hibernate functionality

From: Sebastian Ott <[email protected]>

Platforms may want to provide architecture-specific functionality when a PCI device is doing a hibernate transition. Add a weak symbol pcibios_pm_ops that architectures can override to do so.

[bhelgaas: fold in return value checks from v2 patch]

Signed-off-by Sebastian Ott <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pci-driver.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h      |    4 ++++
 2 files changed, 47 insertions(+)

drivers/pci/pci-driver.c +43 -0

index e6515e2..98f7b9b 100644
@@ -763,6 +763,13 @@ static int pci_pm_resume(struct device *dev)
 
 #ifdef CONFIG_HIBERNATE_CALLBACKS
 
+
+/*
+ * pcibios_pm_ops - provide arch-specific hooks when a PCI device is doing
+ * a hibernate transition
+ */
+struct dev_pm_ops __weak pcibios_pm_ops;
+
 static int pci_pm_freeze(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);

@@ -786,6 +793,9 @@ static int pci_pm_freeze(struct device *dev)
 			return error;
 	}
 
+	if (pcibios_pm_ops.freeze)
+		return pcibios_pm_ops.freeze(dev);
+
 	return 0;
 }
 

@@ -811,6 +821,9 @@ static int pci_pm_freeze_noirq(struct device *dev)
 
 	pci_pm_set_unknown_state(pci_dev);
 
+	if (pcibios_pm_ops.freeze_noirq)
+		return pcibios_pm_ops.freeze_noirq(dev);
+
 	return 0;
 }
 

@@ -820,6 +833,12 @@ static int pci_pm_thaw_noirq(struct device *dev)
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pcibios_pm_ops.thaw_noirq) {
+		error = pcibios_pm_ops.thaw_noirq(dev);
+		if (error)
+			return error;
+	}
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 

@@ -837,6 +856,12 @@ static int pci_pm_thaw(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int error = 0;
 
+	if (pcibios_pm_ops.thaw) {
+		error = pcibios_pm_ops.thaw(dev);
+		if (error)
+			return error;
+	}
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
 

@@ -878,6 +903,9 @@ static int pci_pm_poweroff(struct device *dev)
  Fixup:
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
+	if (pcibios_pm_ops.poweroff)
+		return pcibios_pm_ops.poweroff(dev);
+
 	return 0;
 }
 

@@ -911,6 +939,9 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 	if (pci_dev->class == PCI_CLASS_SERIAL_USB_EHCI)
 		pci_write_config_word(pci_dev, PCI_COMMAND, 0);
 
+	if (pcibios_pm_ops.poweroff_noirq)
+		return pcibios_pm_ops.poweroff_noirq(dev);
+
 	return 0;
 }
 

@@ -920,6 +951,12 @@ static int pci_pm_restore_noirq(struct device *dev)
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pcibios_pm_ops.restore_noirq) {
+		error = pcibios_pm_ops.restore_noirq(dev);
+		if (error)
+			return error;
+	}
+
 	pci_pm_default_resume_early(pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))

@@ -937,6 +974,12 @@ static int pci_pm_restore(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int error = 0;
 
+	if (pcibios_pm_ops.restore) {
+		error = pcibios_pm_ops.restore(dev);
+		if (error)
+			return error;
+	}
+
 	/*
 	 * This is necessary for the hibernation error path in which restore is
 	 * called without restoring the standard config registers of the device.

include/linux/pci.h +4 -0

index 0fd1f15..89ed123 100644
@@ -1648,6 +1648,10 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev,
 int pcibios_add_device(struct pci_dev *dev);
 void pcibios_release_device(struct pci_dev *dev);
 
+#ifdef CONFIG_HIBERNATE_CALLBACKS
+extern struct dev_pm_ops pcibios_pm_ops;
+#endif
+
 #ifdef CONFIG_PCI_MMCONFIG
 void __init pci_mmcfg_early_init(void);
 void __init pci_mmcfg_late_init(void);