[{"id":1795151,"web_url":"http://patchwork.ozlabs.org/comment/1795151/","msgid":"<20171027230340.GC105121@google.com>","list_archive_url":null,"date":"2017-10-27T23:03:41","subject":"Re: [RFC PATCH v10 7/7] PCI / PM: Add support for the PCIe WAKE#\n\tsignal for OF","submitter":{"id":67074,"url":"http://patchwork.ozlabs.org/api/people/67074/","name":"Brian Norris","email":"briannorris@chromium.org"},"content":"Hi Jeffy,\n\nOn Fri, Oct 27, 2017 at 03:26:12PM +0800, Jeffy Chen wrote:\n> Add pci-of.c to handle the PCIe WAKE# interrupt.\n> \n> Also use the dedicated wakeirq infrastructure to simplify it.\n> \n> Signed-off-by: Jeffy Chen \n> ---\n> \n> Changes in v10:\n> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),\n> since dedicated wakeirq will be lost in device_set_wakeup_enable(false).\n> \n> Changes in v9:\n> Fix check error in .cleanup().\n> Move dedicated wakeirq setup to setup() callback and use\n> device_set_wakeup_enable() to enable/disable.\n> \n> Changes in v8:\n> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.\n> \n> Changes in v7:\n> Move PCIE_WAKE handling into pci core.\n> \n> Changes in v6:\n> Fix device_init_wake error handling, and add some comments.\n> \n> Changes in v5:\n> Rebase.\n> \n> Changes in v3:\n> Fix error handling.\n> \n> Changes in v2:\n> Use dev_pm_set_dedicated_wake_irq.\n> \n> drivers/pci/Makefile | 2 +-\n> drivers/pci/pci-of.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 128 insertions(+), 1 deletion(-)\n> create mode 100644 drivers/pci/pci-of.c\n> \n> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile\n> index 66a21acad952..4f76dbdb024c 100644\n> --- a/drivers/pci/Makefile\n> +++ b/drivers/pci/Makefile\n> @@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o\n> \n> obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o\n> \n> -obj-$(CONFIG_OF) += of.o\n> +obj-$(CONFIG_OF) += of.o pci-of.o\n> \n> ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG\n> \n> diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c\n> new file mode 100644\n> index 000000000000..28f3c4a0eec8\n> --- /dev/null\n> +++ b/drivers/pci/pci-of.c\n> @@ -0,0 +1,127 @@\n> +/*\n> + * OF PCI PM support\n> + *\n> + * Copyright (c) 2017 Rockchip, Inc.\n> + *\n> + * Author: Jeffy Chen \n> + *\n> + * This program is free software: you can redistribute it and/or modify\n> + * it under the terms of the GNU General Public License as published by\n> + * the Free Software Foundation, either version 2 of the License, or\n> + * (at your option) any later version.\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \"pci.h\"\n> +\n> +struct of_pci_pm_data {\n> +\tstruct device\t*dev;\n> +\tunsigned int\twakeup_irq;\n> +\tatomic_t\twakeup_cnt;\n> +};\n> +\n> +static void *of_pci_setup(struct device *dev)\n> +{\n> +\tstruct of_pci_pm_data *data;\n> +\tint irq, ret;\n> +\n> +\tif (!dev->of_node)\n> +\t\treturn NULL;\n> +\n> +\tdata = devm_kzalloc(dev, sizeof(struct of_pci_pm_data), GFP_KERNEL);\n> +\tif (!data)\n> +\t\treturn ERR_PTR(-ENOMEM);\n> +\n> +\tirq = of_irq_get_byname(dev->of_node, \"wakeup\");\n> +\tif (irq < 0) {\n> +\t\tif (irq == -EPROBE_DEFER)\n> +\t\t\treturn ERR_PTR(irq);\n> +\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\tdata->wakeup_irq = irq;\n> +\tdata->dev = dev;\n> +\n> +\tdevice_init_wakeup(dev, true);\n> +\tret = dev_pm_set_dedicated_wake_irq(dev, irq);\n\nI'm seeing this WARNING during system resume when I enable wake-on-Wifi\nwith this series:\n\n[ 135.259920] Unbalanced IRQ 64 wake disable\n[ 135.259929] ------------[ cut here ]------------\n[ 135.259942] WARNING: CPU: 0 PID: 3233 at kernel/irq/manage.c:606 irq_set_irq_wake+0xac/0x12c\n[ 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\n[ 135.259986] CPU: 0 PID: 3233 Comm: bash Not tainted 4.14.0-rc3+ #40\n[ 135.259988] Hardware name: Google Kevin (DT)\n[ 135.259991] task: ffffffc0f133c880 task.stack: ffffff8010520000\n[ 135.259994] PC is at irq_set_irq_wake+0xac/0x12c\n[ 135.259998] LR is at irq_set_irq_wake+0xac/0x12c\n...\n[ 135.260121] [] irq_set_irq_wake+0xac/0x12c\n[ 135.260127] [] dev_pm_disarm_wake_irq+0x3c/0x58\n[ 135.260133] [] device_wakeup_disarm_wake_irqs+0x50/0x78\n[ 135.260137] [] dpm_noirq_end+0x18/0x24\n[ 135.260140] [] dpm_resume_noirq+0x24/0x30\n[ 135.260146] [] suspend_devices_and_enter+0x474/0x970\n[ 135.260150] [] pm_suspend+0x688/0x6cc\n[ 135.260154] [] state_store+0xd4/0xf8\n[ 135.260160] [] kobj_attr_store+0x18/0x28\n[ 135.260165] [] sysfs_kf_write+0x5c/0x68\n[ 135.260169] [] kernfs_fop_write+0x15c/0x198\n[ 135.260174] [] __vfs_write+0x58/0x160\n[ 135.260178] [] vfs_write+0xc4/0x15c\n[ 135.260181] [] SyS_write+0x64/0xb4\n\nI'm not yet sure if this is your series' fault, or if the dedicated wake IRQ\ninfrastructure did something wrong.\n\n> +\tif (ret < 0) {\n> +\t\tdevice_init_wakeup(dev, false);\n> +\t\treturn NULL;\n> +\t}\n> +\tdevice_set_wakeup_capable(dev, false);\n> +\n> +\tdev_info(dev, \"Wakeup IRQ %d\\n\", irq);\n\nDo you actually need to print this out? It'll be available in things\nlike /proc/interrupts now, so this seems overkill.\n\n> +\treturn data;\n> +}\n> +\n> +static void *of_pci_setup_dev(struct pci_dev *pci_dev)\n> +{\n> +\treturn of_pci_setup(&pci_dev->dev);\n> +}\n> +\n> +static void *of_pci_setup_host_bridge(struct pci_host_bridge *bridge)\n> +{\n> +\treturn of_pci_setup(bridge->dev.parent);\n> +}\n> +\n> +static void of_pci_cleanup(void *pmdata)\n> +{\n> +\tstruct of_pci_pm_data *data = pmdata;\n> +\n> +\tif (!IS_ERR_OR_NULL(data)) {\n> +\t\tdevice_init_wakeup(data->dev, false);\n> +\t\tdev_pm_clear_wake_irq(data->dev);\n> +\t}\n> +}\n> +\n> +static bool of_pci_can_wakeup(void *pmdata)\n> +{\n> +\tstruct of_pci_pm_data *data = pmdata;\n> +\n> +\tif (IS_ERR_OR_NULL(data))\n> +\t\treturn false;\n> +\n> +\treturn data->wakeup_irq > 0;\n> +}\n> +\n> +static int of_pci_dev_wakeup(void *pmdata, bool enable)\n> +{\n> +\tstruct of_pci_pm_data *data = pmdata;\n> +\tstruct device *dev = data->dev;\n> +\n> +\tdevice_set_wakeup_capable(dev, enable);\n> +\treturn 0;\n> +}\n> +\n> +static int of_pci_bridge_wakeup(void *pmdata, bool enable)\n> +{\n> +\tstruct of_pci_pm_data *data = pmdata;\n> +\n> +\tif (enable && atomic_inc_return(&data->wakeup_cnt) != 1)\n> +\t\treturn 0;\n> +\n> +\tif (!enable && atomic_dec_return(&data->wakeup_cnt) != 0)\n> +\t\treturn 0;\n> +\n> +\treturn of_pci_dev_wakeup(pmdata, enable);\n> +}\n> +\n> +static const struct pci_platform_pm_ops of_pci_platform_pm = {\n> +\t.setup_dev\t\t= of_pci_setup_dev,\n> +\t.setup_host_bridge\t= of_pci_setup_host_bridge,\n> +\t.cleanup\t\t= of_pci_cleanup,\n> +\t.can_wakeup\t\t= of_pci_can_wakeup,\n> +\t.dev_wakeup\t\t= of_pci_dev_wakeup,\n> +\t.bridge_wakeup\t\t= of_pci_bridge_wakeup,\n> +};\n> +\n> +static int __init of_pci_init(void)\n> +{\n\nI still don't think you've worked out the potential conflict between\nACPI and OF on (e.g.) ARM64 systems with both enabled in the kernel. On\nsuch kernels, both acpi_pci_init() and of_pci_init() are built in as\narch_initcalls, and which one wins will be based on implementation\ndetails like link order.\n\nSeems like acpi_pci_init() could still use something like this:\n\n\tif (acpi_disabled)\n\t\treturn;\n\nAnd do the opposite here in of_pci_init().\n\nIt also feels like we could use something like this in pci.c:\n\n void pci_set_platform_pm(const struct pci_platform_pm_ops *ops)\n {\n+\tWARN(pci_platform_pm, \"PCI platform PM ops already registered\\n\");\n \tpci_platform_pm = ops;\n }\n\nAnd I wonder what happens with pci-mid.c -- does this currently win the\ncollision because pci-mid.o is linked after pci-acpi.o?\n\nBrian\n\n> +\tpci_set_platform_pm(&of_pci_platform_pm);\n> +\treturn 0;\n> +}\n> +arch_initcall(of_pci_init);\n> -- \n> 2.11.0\n> \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"GJFJw4AT\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yNzvV0z1qz9t4X\n\tfor ;\n\tSat, 28 Oct 2017 10:03:50 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751251AbdJ0XDr (ORCPT );\n\tFri, 27 Oct 2017 19:03:47 -0400","from mail-io0-f194.google.com ([209.85.223.194]:45521 \"EHLO\n\tmail-io0-f194.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751326AbdJ0XDp (ORCPT\n\t); Fri, 27 Oct 2017 19:03:45 -0400","by mail-io0-f194.google.com with SMTP id i38so15866595iod.2\n\tfor ; Fri, 27 Oct 2017 16:03:45 -0700 (PDT)","from google.com ([2620:0:1000:1600:822:58a4:e85c:5b35])\n\tby smtp.gmail.com with ESMTPSA id\n\tz65sm3951285ioe.52.2017.10.27.16.03.43\n\t(version=TLS1_2 cipher=AES128-SHA bits=128/128);\n\tFri, 27 Oct 2017 16:03:44 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=uPuiK3SZk0TdY15F2O+YKmg0x6WlgALEZxFYc8sp1/8=;\n\tb=GJFJw4AT/WzdB3/n3cdJbMBffg+NwCZXBzSxoGnZz83vCv8o+d+W1GNRuHn/MH/c6q\n\tg3MswM/ANgGznjOJVwORqZ6+g7JGYmQyzibnHCYPRvarN3jiUTuuv+5FZ/WnDvvLtq9m\n\t/e1A4FyT/qKGWnXOCzDlvzOaLbXM4d0YMbfsA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=uPuiK3SZk0TdY15F2O+YKmg0x6WlgALEZxFYc8sp1/8=;\n\tb=GFK3JV+um5hXGi1TrmBUb8FEyGa5etLihQ8lYYT/TGP69wiez+YeiKDzE87QjUDunX\n\t/Uh9V3lpj7bcsS2a9eoV1aCXj2aFs5HhwegEPQATx6xQonLoiccRyKyoRU5Banag7OF6\n\tUchSRmT52V09oy1QOHGWkW9PYQjqwa/GSbbPTmCNRbhiHjIhacFy+U37Kq3i1Fs46k0B\n\tcX7/FbENC9vhlyQirLZS6hCcTwSn1LFYTM//64aGq+sCJt/prkEwQ2arvT817XVgjFnQ\n\tLClyDBzFi0oooWcla5d6bhcoGuPN2K6zzgKqHBHTJAOVPRTeV6OtPRE1pXrzz4xuJyxr\n\tdEuQ==","X-Gm-Message-State":"AMCzsaXPKhXoSZPfSzFZ9nr1aIBS626x8sogNEUICT4cqwbpnb/Asq0X\n\typ4ypaJSFyKLJD5r5Qw9eO+6Vg==","X-Google-Smtp-Source":"ABhQp+Skb9yr/A2EgziNjr3E2I61iZR2iCGLGVtPYKlEBv/u6fyx5dE3ABZyrafv/0QhFwi/3k+xsA==","X-Received":"by 10.36.254.130 with SMTP id w124mr2559705ith.19.1509145424689; \n\tFri, 27 Oct 2017 16:03:44 -0700 (PDT)","Date":"Fri, 27 Oct 2017 16:03:41 -0700","From":"Brian Norris ","To":"Jeffy Chen ","Cc":"linux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, tony@atomide.com,\n\tshawn.lin@rock-chips.com, rjw@rjwysocki.net, dianders@chromium.org,\n\tlinux-pci@vger.kernel.org","Subject":"Re: [RFC PATCH v10 7/7] PCI / PM: Add support for the PCIe WAKE#\n\tsignal for OF","Message-ID":"<20171027230340.GC105121@google.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<20171027072612.26565-8-jeffy.chen@rock-chips.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171027072612.26565-8-jeffy.chen@rock-chips.com>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}}]