On Sat, Jul 25, 2020 at 1:37 AM Bjorn Helgaas [off-list ref] wrote:

[+cc Rafael, in case you can clear up our wakeup confusion]
original patch:
https://lore.kernel.org/r/20200720155714.714114-1-vaibhavgupta40@gmail.com (local)

On Fri, Jul 24, 2020 at 11:16:55PM +0300, Andy Shevchenko wrote:

quoted

On Fri, Jul 24, 2020 at 6:17 PM Vaibhav Gupta [off-list ref] wrote:

quoted

On Fri, Jul 24, 2020 at 01:51:49PM +0300, Andy Shevchenko wrote:

quoted

On Mon, Jul 20, 2020 at 7:31 PM Vaibhav Gupta [off-list ref] wrote:

...

quoted

quoted

quoted

+       device_wakeup_disable(dev);

quoted

quoted

quoted

Here I left a result. Care to explain (and perhaps send a follow up
fix) where is the counterpart to this call?

The common pattern seems to be "enable wakeup in suspend, disable
wakeup in resume".

The confusion in spi-topcliff-pch.c is that it *disables* wakeup in
both the .suspend() and the .resume() methods and never seems to
enable wakeup at all.

Maybe there's something subtle we're missing, because all of the
following are the same way; they disable wakeup in suspend and also
disable wakeup in resume:

  pch_i2c_suspend    pci_enable_wake(pdev, PCI_D3hot, 0);
  pch_phub_suspend   pci_enable_wake(pdev, PCI_D3hot, 0);
  tifm_7xx1_suspend  pci_enable_wake(dev, pci_choose_state(dev, state), 0);
  pch_can_suspend    pci_enable_wake(pdev, PCI_D3hot, 0);
  atl1e_suspend      pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
  atl2_suspend       pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
  smsc9420_suspend   pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
  pch_suspend        pci_enable_wake(pdev, PCI_D3hot, 0);
  pch_spi_suspend    pci_enable_wake(pdev, PCI_D3hot, 0);

And the following are curious because they seem to disable wakeup in
suspend but don't do anything with wakeup in resume:

  jmb38x_ms_suspend  pci_enable_wake(dev, pci_choose_state(dev, state), 0);
  rtsx_pci_suspend   pci_enable_wake(pcidev, pci_choose_state(pcidev, state), 0);
  toshsd_pm_suspend  pci_enable_wake(pdev, PCI_D3hot, 0);
  via_sd_suspend     pci_enable_wake(pcidev, pci_choose_state(pcidev, state), 0);
  uli526x_suspend    pci_enable_wake(pdev, power_state, 0);

All of the above *look* buggy, but maybe we're missing something.

Agree!

My *guess* is that most PCI drivers using generic PM shouldn't do
anything at all with wakeup because these paths in the PCI core do it
for them:

That's why in the second message I proposed to drop this ambiguous
call. I think (guess) the same way you are.

  pci_pm_suspend_noirq             # pci_dev_pm_ops.suspend_noirq
    if (!pdev->state_saved)
      if (pci_power_manageable(pdev)
        pci_prepare_to_sleep(pdev)
          wakeup = device_may_wakeup(&pdev->dev)
          pci_enable_wake(pdev, ..., wakeup)

  pci_pm_resume                    # pci_dev_pm_ops.resume
    pci_pm_default_resume
      pci_enable_wake(pdev, ..., false)

quoted

quoted

Yes, it seem I forgot to put device_wakeup_disable() in .suspend()
when I removed pci_enable_wake(pdev, PCI_D3hot, 0); from there. It
doesn't seem that .suspend() wants to enable-wake the device as
the bool value passed to pci_enable_wake() is zero.

quoted

Am I missing something else?

At least above. Either you need to drop the current call, or explain
how it works.

quoted

Since you have no hardware to test, I would rather ask to drop an
extra call or revert the change.

I'm not quite sure what you mean here.  Vaibhav is converting dozens
of drivers from legacy PCI PM to generic PM, and of course doesn't
have any of that hardware, but it's still worth doing the conversion.

See above. Here I proposed two solutions and I'm in favour of the
former (drop the call) rather than latter (do not touch this code).

If it's a bug that spi-topcliff-pch.c disables but never enables
wakeup, I think this should turn into two patches:

  1) Fix the bug by enabling wakeup in suspend (or whatever the right
  fix is), and

  2) Convert to generic PM, which may involve removing the
  wakeup-related code completely.

Works for me.


-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees