Re: [RFC 3/5] pci wakeup handler

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, 8 of September 2008, shaohua.li@intel.com wrote:
> pci subsystem wakeup handler.
> ---
>  drivers/pci/pci-driver.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> Index: linux/drivers/pci/pci-driver.c
> ===================================================================
> --- 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)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int pme_pos = pci_find_capability(pdev, PCI_CAP_ID_PM);

Pleae use dev->pm_cap instead.

> +	struct pci_driver *drv = pdev->driver;
> +	u16 reg16;
> +	int spurious = 0;

Please use 'bool' for boolean variables.

> +	int ret = -ENODEV;
> +
> +	if (pme_pos == 0) {
> +		/*
> +		 * Some USB devices haven't PME, but have specific registers to
> +		 * control wakeup
> +		 */
> +		goto out;
> +	}
> +
> +	/* clear PME status and disable PME to avoid interrupt flood */
> +	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &reg16);

The variable used for that is called 'pmcsr' in the other functions.  Perhaps
call it 'pmcsr' instead of 'reg16' here too?

> +	if (!(reg16 & PCI_PM_CTRL_PME_STATUS))
> +		return -ENODEV;
> +	/* I see spurious GPE here, just ignore it for now */
> +	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);

I think you can use pci_pm_active() for clearing PME# and status.  That'll
read the register once more, but that shouldn't be a problem.  Actually, you
can do:

	pci_read_config_word(pdev, pme_pos + PCI_PM_CTRL, &pmcsr);
	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
		/* No event (why have we been called, actually? */
		return -EINVAL;
	pci_pm_active(dev, false);
	if (!(pmcsr & PCI_PM_CTRL_PME_ENABLE))
		/* Spurious event */
		return -ENODEV;
> +
> +	if (spurious)
> +		return -ENODEV;
> +	ret = 0;
> +	/* This device invokes PME, gives driver a chance to do something */
> +out:
> +	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);

Hm, I'd do:

	if (drv && drv->pm && drv->pm->base.wakeup_event) {
		int dev_ret = drv->pm->base.wakeup_event(&pdev->dev);
		if (ret)
			ret = dev_ret;
	}

Also, why do you think we should ignore the returned value if ret is zero?

> +	}
> +	return ret;
> +}
>  #else /* !CONFIG_SUSPEND */
>  
>  #define pci_pm_suspend		NULL
>  #define pci_pm_suspend_noirq	NULL
>  #define pci_pm_resume		NULL
>  #define pci_pm_resume_noirq	NULL
> +#define pci_pm_wakeup_event	NULL
>  
>  #endif /* !CONFIG_SUSPEND */
>  
> @@ -651,6 +696,7 @@ struct pm_ext_ops pci_pm_ops = {
>  		.thaw = pci_pm_thaw,
>  		.poweroff = pci_pm_poweroff,
>  		.restore = pci_pm_restore,
> +		.wakeup_event = pci_pm_wakeup_event,
>  	},
>  	.suspend_noirq = pci_pm_suspend_noirq,
>  	.resume_noirq = pci_pm_resume_noirq,
> 

Thanks,
Rafael