linux-kernel - Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5A41A0A7.5070605@rock-chips.com>
Date:   Tue, 26 Dec 2017 09:06:47 +0800
From:   JeffyChen <jeffy.chen@...k-chips.com>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
CC:     linux-kernel@...r.kernel.org, bhelgaas@...gle.com,
        linux-pm@...r.kernel.org, tony@...mide.com,
        shawn.lin@...k-chips.com, briannorris@...omium.org,
        dianders@...omium.org, devicetree@...r.kernel.org,
        linux-pci@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>
Subject: Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE#
 signal for OF

Hi Rafael,

Thanks for your reply :)

On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
>> >+
>> >+	dn = pci_device_to_OF_node(ppdev);
>> >+	if (!dn)
>> >+		return 0;
>> >+
>> >+	irq = of_irq_get_byname(dn, "wakeup");
> Why is this a property of the bridge and not of the device itself?
That is suggested by Brian, because in that way, the wakeup pin would 
not "tied to what exact device is installed (or no device, if it's a slot)."

>
>> >+	if (irq == -EPROBE_DEFER)
> Braces here, please.
ok, will fix in the next version.

>
>> >+		return irq;
>> >+	/* Ignore other errors, since a missing wakeup is non-fatal. */
>> >+	else if (irq < 0) {
>> >+		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
>> >+		return 0;
>> >+	}
>> >+
>> >+	device_init_wakeup(&pdev->dev, true);
> Why do you call this before dev_pm_set_dedicated_wake_irq()?
hmmm, i thought so too, but it turns out the dedicated wake irq 
framework requires device_init_wakeup(dev, true) before attach the wake irq:

int device_wakeup_attach_irq(struct device *dev,
                              struct wake_irq *wakeirq)
{
         struct wakeup_source *ws;

         ws = dev->power.wakeup;
         if (!ws) {
                 dev_err(dev, "forgot to call device_init_wakeup?\n");
                 return -EINVAL;


>
>> >+
>> >+	ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, irq);
>> >+	if (ret < 0) {
>> >+		dev_err(&pdev->dev, "failed to set wake IRQ: %d\n", ret);
>> >+		device_init_wakeup(&pdev->dev, false);
>> >+		return ret;
>> >+	}
>> >+
> It would be more straightforward to call device_init_wakeup() here.
>
>> >+	return 0;
>> >+}
>> >+EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
>> >+
>> >+void of_pci_teardown_wake_irq(struct pci_dev *pdev)
>> >+{
>> >+	if (!pdev->dev.power.wakeirq)
>> >+		return;
>> >+
>> >+	dev_pm_clear_wake_irq(&pdev->dev);
>> >+	device_init_wakeup(&pdev->dev, false);
>> >+}
>> >+EXPORT_SYMBOL_GPL(of_pci_teardown_wake_irq);
>> >+
>> >  /**
>> >   * of_irq_parse_pci - Resolve the interrupt for a PCI device
>> >   * @pdev:       the device whose interrupt is to be resolved
> Thanks,
> Rafael
>
>
>
>