Re: [RFC 3/5] pci wakeup handler

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: David Brownell [mailto:david-b@xxxxxxxxxxx]
>Sent: Tuesday, September 09, 2008 10:57 AM
>To: Li, Shaohua
>Cc: linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx;
>stern@xxxxxxxxxxxxxxxxxxx
>Subject: Re: [RFC 3/5] pci wakeup handler
>
>On Monday 08 September 2008, shaohua.li@xxxxxxxxx wrote:
>> --- linux.orig/drivers/pci/pci-driver.c       2008-09-08
>13:55:56.000000000 +0800
>> +++ linux/drivers/pci/pci-driver.c    2008-09-08 14:24:42.000000000 +0800
>> @@ -472,12 +472,57 @@ static int pci_pm_resume_noirq(struct de
>>       return error;
>>  }
>>
>> +/*
>> + * Called when dev is suspected to invoke a wakeup event, return 0 if
>yes
>> + * */
>> +static int pci_pm_wakeup_event(struct device *dev)
>
>If wakeup notifications were bus-specific this would
>take "struct pci_dev *pdev" ... and we're missing a
>key part of this stuff, namely the code to sort out
>which devices get this call.
This device is suspected to invoke a wakeup event, might not be true. I suppose other bus requires the same mechanism, so should be a 'struct device'

>> +     struct pci_driver *drv = pdev->driver;
>> +     u16 reg16;
>> +     int spurious = 0;
>> +     int ret = -ENODEV;
>> +
>> +     if (pme_pos == 0) {
>> +             /*
>> +              * Some USB devices haven't PME, but have specific
>registers to
>> +              * control wakeup
>
>The PCI PM spec has words some like:  "Some PCI devices support legacy
>wakeup mechanisms instead of supporting PCI PM capabilities."  Today
>the best example of that is Intel's UHCI controllers, but it's not
>restricted to USB at all.
>
>I suspect that this particular path won't often need to handle anything
>other than those Intel controllers, however.  And so I hope that they
>never share GPEs.  ;)
This is one of the reason calling drv->wakeup_event().

>Another rather important case is bridges.  I observe that with ACPI
>the way PME# is handled for add-in cards _seems_ to be that the bridge
>for that PCI bus segment gets a GPE (presumably matching the PME#
>signal for that bus segment, and maybe for its subsidiaries).  That
>suggests the bridge will need to scan its children to find out which
>one(s) issued PME#.  You didn't include such code here, where that
>notification will be received...
In GPE case, it appears BIOS will detect the exact wakeup device.

In native PME case, if a device is a pcie device, npme will detect the exact device too. If the device is a legacy device, then npme driver will check devices under bridges, please see the npme_pme_target().

We can directly scan children in pci_pm_wakeup_event() too, but GPE case doesn't require it and actually is broken in GPE case as duplication will be added.

I thought this covers all cases in IA platform, right?
>
>> +     /* I see spurious GPE here, just ignore it for now */
>
>Comments about GPEs shouldn't be included here; they're ACPI-specific.
>This is PCI-generic code!
A typo, it should be PME. And actually I found this in npme case instead of ACPI.

>> +     if (!(reg16 & PCI_PM_CTRL_PME_ENABLE))
>> +             spurious = 1;
>> +     reg16 &= ~PCI_PM_CTRL_PME_ENABLE;
>> +     reg16 |= PCI_PM_CTRL_PME_STATUS;
>> +     pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, reg16);
>
>If this device didn't issue PME, then don't clear PME_ENABLE.
Ok, this is reasonable.


>> +     if (drv && drv->pm && drv->pm->base.wakeup_event) {
>> +             if (!ret) /* ignore return value in this case */
>> +                     drv->pm->base.wakeup_event(&pdev->dev);
>> +             else
>> +                     return drv->pm->base.wakeup_event(&pdev->dev);
>
>I'm also puzzled why you want the legacy PM case (UHCI etc)
>to work differently from the "normal" one.
>
>And style-wise I'd really prefer to see
>
>        return pdev->driver->wakeup(pdev);
>
>Or perhaps more generically -- since I still have yet to
>hear an argument why resume() shouldn't suffice to handle
>the wakeup event processing, at least for PCI -- just
>
>        if (pdev->driver->wakeup)
>                return pdev->driver->wakeup(pdev);
>        if (pdev->driver->resume)
>                return pdev->driver->resume(pdev);
>
>Although I don't know what the return value here should
>be interpreted to mean.  Would it be better to return
>void, and just log all "interesting"/error cases?
In my mind, .wakeup_event() just returns if the device invokes a wakeup event, ACPI or NPME will call corresponding .resume method. Suspected device might not invoke wakeup event as you said the bridge case.

Thanks,
Shaohua
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

[Index of Archives]     [Linux ACPI]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [CPU Freq]     [Kernel Newbies]     [Fedora Kernel]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux