* Rafael J. Wysocki [off-list ref] [171228 17:33]:

On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:

quoted

Well Brian had a concern where we would have to implement PM runtime
for all device drivers for PCI devices.

Why would we?

Seems at least I was a bit confused. In the PCIe case the WAKE# is
owned by the PCIe slot, not the child PCIe device. So you're right,
there should be no need for the child PCIe device drivers to
implement runtime PM.

I was thinking the wakeirq case with WLAN on SDIO bus. Some WLAN
devices can have a hardwired OOB wakeirq wired to a GPIO controller.
In that case the wakeirq is owned by the child device driver
(WLAN controller) and not by the SDIO slot. I was earlier
thinking this is the same as the "Figure 5-4" case 1, but it's
not.

So in the PCIe WAKE# case for device tree, we must have the
wakeirq property for the PCIe slot for the struct device managing
that slot, and not for the child device driver. I think it's
already this way in the most recent set of patches, I need to
look again.

quoted

So isn't my option 1 above similar to the PCIe spec "Figure 5-4"
case 2?

No, it isn't, because in that case there is no practical difference
between WAKE# and an in-band PME message sent by the device (Beacon)
from the software perspective.

WAKE# causes the switch to send a PME message upstream and that is
handled by the Root Complex through the standard mechanism already
supported by our existing PME driver (drivers/pci/pcie/pme.c).

OK. So if "Figure 5-4" case 2 is already handled then and we need
to just deal with "Figure 5-4" case 1 :)

quoted

Yeah. FYI, for the dedicated wakeirq cases I have, we need to keep
them masked during runtime to avoid tons of interrupts as they
are often wired to the RX pins.

OK

BTW, enable_irq_wake() should take care of the sharing, shouldn't it?

That can be used to tell us which device has wakeirq enabled for
wake-up events, but only for resume not runtiem PM. We still have the
shared IRQ problem to deal with. And the PCIe subsystem still needs
to go through the child devices.

But the WAKE# thing is not just for waking up the system from sleep states,
it is for runtime PM's wakeup signaling too.

Yes my test cases have it working for runtime PM and for waking
up system from suspend.

quoted

quoted

quoted

Currently nothing happens with wakeirqs if there's no struct
wakeup_source. On device_wakeup_enable() we call device_wakeup_attach()
that just copies dev->power.wakeirq to ws->wakeirq. And when struct
wake_source is freed the device should be active and wakeirq
disabled. Or are you seeing other issues here?

I'm suspicious about one thing, but I need to look deeper into the code. :-)

So we are fine except for the race and we need the wakeirq field in wakeup
sources to automatically arm the wakeup IRQs during suspend.

OK.

If I'm not mistaken, we only need something like the patch below (untested).

Seems like it should fix the race, I'll do some testing next week.

Regards,

Tony