Re: [RFC PATCH v10 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Brian Norris <briannorris@chromium.org>]

Hi Jeffy,

On Fri, Oct 27, 2017 at 03:26:12PM +0800, Jeffy Chen wrote:
> Add pci-of.c to handle the PCIe WAKE# interrupt.
> 
> Also use the dedicated wakeirq infrastructure to simplify it.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v10:
> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
> since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
> 
> Changes in v9:
> Fix check error in .cleanup().
> Move dedicated wakeirq setup to setup() callback and use
> device_set_wakeup_enable() to enable/disable.
> 
> Changes in v8:
> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
> 
> Changes in v7:
> Move PCIE_WAKE handling into pci core.
> 
> Changes in v6:
> Fix device_init_wake error handling, and add some comments.
> 
> Changes in v5:
> Rebase.
> 
> Changes in v3:
> Fix error handling.
> 
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq.
> 
>  drivers/pci/Makefile |   2 +-
>  drivers/pci/pci-of.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 128 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pci-of.c
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 66a21acad952..4f76dbdb024c 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
>  
>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
>  
> -obj-$(CONFIG_OF) += of.o
> +obj-$(CONFIG_OF) += of.o pci-of.o
>  
>  ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>  
> diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
> new file mode 100644
> index 000000000000..28f3c4a0eec8
> --- /dev/null
> +++ b/drivers/pci/pci-of.c
> @@ -0,0 +1,127 @@
> +/*
> + * OF PCI PM support
> + *
> + * Copyright (c) 2017 Rockchip, Inc.
> + *
> + * Author: Jeffy Chen <jeffy.chen@rock-chips.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/pm_wakeirq.h>
> +#include "pci.h"
> +
> +struct of_pci_pm_data {
> +	struct device	*dev;
> +	unsigned int	wakeup_irq;
> +	atomic_t	wakeup_cnt;
> +};
> +
> +static void *of_pci_setup(struct device *dev)
> +{
> +	struct of_pci_pm_data *data;
> +	int irq, ret;
> +
> +	if (!dev->of_node)
> +		return NULL;
> +
> +	data = devm_kzalloc(dev, sizeof(struct of_pci_pm_data), GFP_KERNEL);
> +	if (!data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	irq = of_irq_get_byname(dev->of_node, "wakeup");
> +	if (irq < 0) {
> +		if (irq == -EPROBE_DEFER)
> +			return ERR_PTR(irq);
> +
> +		return NULL;
> +	}
> +
> +	data->wakeup_irq = irq;
> +	data->dev = dev;
> +
> +	device_init_wakeup(dev, true);
> +	ret = dev_pm_set_dedicated_wake_irq(dev, irq);

I'm seeing this WARNING during system resume when I enable wake-on-Wifi
with this series:

[  135.259920] Unbalanced IRQ 64 wake disable
[  135.259929] ------------[ cut here ]------------
[  135.259942] WARNING: CPU: 0 PID: 3233 at kernel/irq/manage.c:606 irq_set_irq_wake+0xac/0x12c
[  135.259944] Modules linked in: btusb btrtl btbcm btintel bluetooth ecdh_generic cdc_ether usbnet uvcvideo mwifiex_pcie videobuf2_vmalloc videobuf2_memops mwifiex videobuf2_v4l2 videobuf2_core cfg80211 ip6table_filter r8152 mii joydev
[  135.259986] CPU: 0 PID: 3233 Comm: bash Not tainted 4.14.0-rc3+ #40
[  135.259988] Hardware name: Google Kevin (DT)
[  135.259991] task: ffffffc0f133c880 task.stack: ffffff8010520000
[  135.259994] PC is at irq_set_irq_wake+0xac/0x12c
[  135.259998] LR is at irq_set_irq_wake+0xac/0x12c
...
[  135.260121] [<ffffff80080ff1a4>] irq_set_irq_wake+0xac/0x12c
[  135.260127] [<ffffff8008494a7c>] dev_pm_disarm_wake_irq+0x3c/0x58
[  135.260133] [<ffffff800849989c>] device_wakeup_disarm_wake_irqs+0x50/0x78
[  135.260137] [<ffffff800849667c>] dpm_noirq_end+0x18/0x24
[  135.260140] [<ffffff80084966ac>] dpm_resume_noirq+0x24/0x30
[  135.260146] [<ffffff80080f85cc>] suspend_devices_and_enter+0x474/0x970
[  135.260150] [<ffffff80080f9150>] pm_suspend+0x688/0x6cc
[  135.260154] [<ffffff80080f7388>] state_store+0xd4/0xf8
[  135.260160] [<ffffff80087c3f3c>] kobj_attr_store+0x18/0x28
[  135.260165] [<ffffff80082818f8>] sysfs_kf_write+0x5c/0x68
[  135.260169] [<ffffff80082808c0>] kernfs_fop_write+0x15c/0x198
[  135.260174] [<ffffff80082081a8>] __vfs_write+0x58/0x160
[  135.260178] [<ffffff8008208484>] vfs_write+0xc4/0x15c
[  135.260181] [<ffffff80082086cc>] SyS_write+0x64/0xb4

I'm not yet sure if this is your series' fault, or if the dedicated wake IRQ
infrastructure did something wrong.

> +	if (ret < 0) {
> +		device_init_wakeup(dev, false);
> +		return NULL;
> +	}
> +	device_set_wakeup_capable(dev, false);
> +
> +	dev_info(dev, "Wakeup IRQ %d\n", irq);

Do you actually need to print this out? It'll be available in things
like /proc/interrupts now, so this seems overkill.

> +	return data;
> +}
> +
> +static void *of_pci_setup_dev(struct pci_dev *pci_dev)
> +{
> +	return of_pci_setup(&pci_dev->dev);
> +}
> +
> +static void *of_pci_setup_host_bridge(struct pci_host_bridge *bridge)
> +{
> +	return of_pci_setup(bridge->dev.parent);
> +}
> +
> +static void of_pci_cleanup(void *pmdata)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (!IS_ERR_OR_NULL(data)) {
> +		device_init_wakeup(data->dev, false);
> +		dev_pm_clear_wake_irq(data->dev);
> +	}
> +}
> +
> +static bool of_pci_can_wakeup(void *pmdata)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (IS_ERR_OR_NULL(data))
> +		return false;
> +
> +	return data->wakeup_irq > 0;
> +}
> +
> +static int of_pci_dev_wakeup(void *pmdata, bool enable)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +	struct device *dev = data->dev;
> +
> +	device_set_wakeup_capable(dev, enable);
> +	return 0;
> +}
> +
> +static int of_pci_bridge_wakeup(void *pmdata, bool enable)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (enable && atomic_inc_return(&data->wakeup_cnt) != 1)
> +		return 0;
> +
> +	if (!enable && atomic_dec_return(&data->wakeup_cnt) != 0)
> +		return 0;
> +
> +	return of_pci_dev_wakeup(pmdata, enable);
> +}
> +
> +static const struct pci_platform_pm_ops of_pci_platform_pm = {
> +	.setup_dev		= of_pci_setup_dev,
> +	.setup_host_bridge	= of_pci_setup_host_bridge,
> +	.cleanup		= of_pci_cleanup,
> +	.can_wakeup		= of_pci_can_wakeup,
> +	.dev_wakeup		= of_pci_dev_wakeup,
> +	.bridge_wakeup		= of_pci_bridge_wakeup,
> +};
> +
> +static int __init of_pci_init(void)
> +{

I still don't think you've worked out the potential conflict between
ACPI and OF on (e.g.) ARM64 systems with both enabled in the kernel. On
such kernels, both acpi_pci_init() and of_pci_init() are built in as
arch_initcalls, and which one wins will be based on implementation
details like link order.

Seems like acpi_pci_init() could still use something like this:

	if (acpi_disabled)
		return;

And do the opposite here in of_pci_init().

It also feels like we could use something like this in pci.c:

 void pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
 {
+	WARN(pci_platform_pm, "PCI platform PM ops already registered\n");
 	pci_platform_pm = ops;
 }

And I wonder what happens with pci-mid.c -- does this currently win the
collision because pci-mid.o is linked after pci-acpi.o?

Brian

> +	pci_set_platform_pm(&of_pci_platform_pm);
> +	return 0;
> +}
> +arch_initcall(of_pci_init);
> -- 
> 2.11.0
> 
>