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]

Date:   Wed, 27 Dec 2017 01:57:07 +0100
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     JeffyChen <jeffy.chen@...k-chips.com>, tony@...mide.com
Cc:     linux-kernel@...r.kernel.org, bhelgaas@...gle.com,
        linux-pm@...r.kernel.org, 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

On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> 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)."

But I don't think it works when there are two devices using different WAKE#
interrupt lines under the same bridge.  Or how does it work then?

> >> >+	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;
> 

Well, that's a framework issue, fair enough.

That said, what if user space removes the wakeup source from under you
concurrently via sysfs?  Tony?

Thanks,
Rafael