On Tue, Aug 20, 2013 at 3:33 PM, Rafael J. Wysocki <rjw@xxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
