Re: [RFC 3/5] pci wakeup handler

[Shaohua Li <shaohua.li@intel.com>]

On Mon, Oct 20, 2008 at 03:50:40AM +0800, Rafael J. Wysocki wrote:
> On Thursday, 11 of September 2008, Shaohua Li wrote:
> > pci subsystem wakeup handler.
> 
> Perhaps add a bit more explanation here - what is introduced, why and why this
> particular way.
I'll add a kernel doc in later post.

> > +static bool pci_handle_one_wakeup_event(struct pci_dev *pdev)
> > +{
> 
> I don't really like that being a boolean function.  I'd make it return 0 on
> success and error code on failure.
Oh, in my previous post, somebody like a boolean and then you like an int
in the mail list. Either is ok to me, but I'd like to have a reason
instead of a 'like' or 'unlike'.

> > +     /* clear PME status and disable PME to avoid interrupt flood */
> > +     pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
> > +     if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
> > +             return false;
> > +     /* I see spurious PME here, just ignore it for now */
> > +     if (!(pmcsr & PCI_PM_CTRL_PME_ENABLE))
> > +             spurious = true;
> > +     else
> > +             pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
> 
> If you do this unconditionally, you'll be able to use pci_pme_active() for it.
> Actually, you can use pci_pme_enabled() for checking if PME is enabled
> and pci_pme_status() for checking if the PME status is set.  Then,
> you can remove the reference to the config space from here and use
> those low-level callbacks instead of them.
ok.
> > +     pmcsr |= PCI_PM_CTRL_PME_STATUS;
> > +     pci_write_config_word(pdev, pme_pos + PCI_PM_CTRL, pmcsr);
> > +
> > +     if (spurious)
> > +             return false;
> > +     return true;
> > +out:
> > +     if (drv && drv->pm && drv->pm->base.wakeup_event)
> > +             return drv->pm->base.wakeup_event(&pdev->dev);
> 
> I'd move this into the 'if (!pme_pos)' block.  And is this what we want really?
> In this case the driver's wakeup_event() will be responsible for checking
> if the wake-up event is valid etc.
Yes, device driver should check if a wake-up event is valid.

> > +             device_receive_wakeup_event(&target->dev);
> 
> Why do we use device_receive_wakeup_event() here?
the device receives wakeup event, so it should do something.

> 
> > +             return ret;
> > +     }
> > +
> > +     if (ret)
> > +             device_receive_wakeup_event(&target->dev);
> 
> And here?  What's the idea?
ditto
 
> > +                             device_receive_wakeup_event(&tmp->dev);
> 
> What exactly is the role of device_receive_wakeup_event() here?
ditto

> > +                     }
> > +             }
> > +     }
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(pci_handle_wakeup_event);
> > +
> > +static bool pci_pm_wakeup_event(struct device *dev)
> > +{
> > +     return pci_handle_wakeup_event(to_pci_dev(dev));
> > +}
> 
> What exactly is the point of introducing this function?
David said other archs (embedded system) might require it.

Thanks,
Shaohua