* Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> [171228 17:33]:
> On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:
> >
> > 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.
> > 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 :)
> > 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.
> > > > 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
> ---
> drivers/base/power/wakeirq.c | 9 ++++-----
> drivers/base/power/wakeup.c | 2 +-
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/base/power/wakeirq.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/wakeirq.c
> +++ linux-pm/drivers/base/power/wakeirq.c
> @@ -33,7 +33,6 @@ static int dev_pm_attach_wake_irq(struct
> struct wake_irq *wirq)
> {
> unsigned long flags;
> - int err;
>
> if (!dev || !wirq)
> return -EINVAL;
> @@ -45,12 +44,12 @@ static int dev_pm_attach_wake_irq(struct
> return -EEXIST;
> }
>
> - err = device_wakeup_attach_irq(dev, wirq);
> - if (!err)
> - dev->power.wakeirq = wirq;
> + dev->power.wakeirq = wirq;
> + if (dev->power.wakeup)
> + device_wakeup_attach_irq(dev, wirq);
>
> spin_unlock_irqrestore(&dev->power.lock, flags);
> - return err;
> + return 0;
> }
>
> /**
> Index: linux-pm/drivers/base/power/wakeup.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/wakeup.c
> +++ linux-pm/drivers/base/power/wakeup.c
> @@ -303,7 +303,7 @@ int device_wakeup_attach_irq(struct devi
> }
>
> if (ws->wakeirq)
> - return -EEXIST;
> + dev_err(dev, "Leftover wakeup IRQ found, overriding\n");
>
> ws->wakeirq = wakeirq;
> return 0;
>
