Re: [PATCH v1] spi: spi-topcliff-pch: use generic power management

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Jul 25, 2020 at 1:37 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> [+cc Rafael, in case you can clear up our wakeup confusion]
> original patch:
> https://lore.kernel.org/r/20200720155714.714114-1-vaibhavgupta40@xxxxxxxxx
>
> On Fri, Jul 24, 2020 at 11:16:55PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 24, 2020 at 6:17 PM Vaibhav Gupta <vaibhavgupta40@xxxxxxxxx> wrote:
> > > On Fri, Jul 24, 2020 at 01:51:49PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jul 20, 2020 at 7:31 PM Vaibhav Gupta <vaibhavgupta40@xxxxxxxxx> wrote:
> >
> > ...
> > > > > +       device_wakeup_disable(dev);
>
> > > >
> > > > 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)
>
> > > 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.
> >
> > > Am I missing something else?
> >
> > At least above. Either you need to drop the current call, or explain
> > how it works.
>
> > 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