| Message ID | alpine.LFD.2.03.1308201637390.1566@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Aug 20, 2013 at 8:41 AM, Sebastian Ott <sebott@linux.vnet.ibm.com> wrote: > 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? > --- > 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 <sebott@linux.vnet.ibm.com> > --- > drivers/pci/pci-driver.c | 30 ++++++++++++++++++++++++++++++ > include/linux/pci.h | 4 ++++ > 2 files changed, 34 insertions(+) > > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -763,6 +763,13 @@ static int pci_pm_resume(struct device * > > #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; For example: int __weak pcibios_pm_freeze(struct device *) { return 0; } > 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 * > return error; > } > > + if (pcibios_pm_ops.freeze) > + pcibios_pm_ops.freeze(dev); return pcibios_pm_freeze(dev); > 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. > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -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); > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 20, 2013 11:08:56 AM Bjorn Helgaas wrote: > On Tue, Aug 20, 2013 at 8:41 AM, Sebastian Ott > <sebott@linux.vnet.ibm.com> wrote: > > 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. 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? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 20, 2013 at 3:33 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Tuesday, August 20, 2013 11:08:56 AM Bjorn Helgaas wrote: >> On Tue, Aug 20, 2013 at 8:41 AM, Sebastian Ott >> <sebott@linux.vnet.ibm.com> wrote: >> > 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. > > 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. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, August 21, 2013 10:23:21 AM Bjorn Helgaas wrote: > On Tue, Aug 20, 2013 at 3:33 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Tuesday, August 20, 2013 11:08:56 AM Bjorn Helgaas wrote: > >> On Tue, Aug 20, 2013 at 8:41 AM, Sebastian Ott > >> <sebott@linux.vnet.ibm.com> wrote: > >> > 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. > > > > 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. No, it's not. I was thinking in general. > 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. I don't have a problem with __weak functions as such, although it's happened to me to overlook an arch-specific variant for a couple of times, but to me it's just inconsistent to use __weak functions here and something different somewhere else. It would be better to choose one convention in my opinion. > 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. I agree here, but -> > 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. So I know that PCI has a history of using __weak symbols, but PM has always been using a different approach. Thanks, Rafael
--- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -763,6 +763,13 @@ static int pci_pm_resume(struct device * #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 * return error; } + if (pcibios_pm_ops.freeze) + pcibios_pm_ops.freeze(dev); + 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. --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -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);
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? Regards, Sebastian --- 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 <sebott@linux.vnet.ibm.com> --- drivers/pci/pci-driver.c | 30 ++++++++++++++++++++++++++++++ include/linux/pci.h | 4 ++++ 2 files changed, 34 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html