From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Vaibhav Gupta <vaibhavgupta40@gmail.com>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-spi <linux-spi@vger.kernel.org>,
Mark Brown <broonie@kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>,
linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH v1] spi: spi-topcliff-pch: use generic power management
Date: Sat, 25 Jul 2020 13:42:39 +0300 [thread overview]
Message-ID: <CAHp75VdSr1rguc9HJVh_rA1nBh1uyCdr18eyPosWPzCH1K2=zg@mail.gmail.com> (raw)
In-Reply-To: <20200724223746.GA1538991@bjorn-Precision-5520>
On Sat, Jul 25, 2020 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> 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
>
> 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@gmail.com> 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@gmail.com> 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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-07-25 10:43 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-20 15:57 [Linux-kernel-mentees] [PATCH v1] spi: spi-topcliff-pch: use generic power management Vaibhav Gupta
2020-07-22 13:45 ` Mark Brown
2020-07-22 20:01 ` Vaibhav Gupta
2020-07-24 10:51 ` Andy Shevchenko
2020-07-24 15:16 ` Vaibhav Gupta
2020-07-24 20:16 ` Andy Shevchenko
2020-07-24 22:37 ` Bjorn Helgaas
2020-07-25 10:42 ` Andy Shevchenko [this message]
2020-07-25 10:44 ` Andy Shevchenko
2020-07-27 7:06 ` Vaibhav Gupta
2020-07-27 11:12 ` Andy Shevchenko
2020-07-27 13:08 ` Vaibhav Gupta
2020-07-27 13:17 ` [Linux-kernel-mentees] [PATCH v2] spi: spi-topcliff-pch: drop call to wakeup-disable Vaibhav Gupta
2020-07-27 13:38 ` Andy Shevchenko
2020-07-27 13:46 ` Vaibhav Gupta
2020-07-27 14:08 ` Andy Shevchenko
2020-07-27 14:17 ` Joe Perches
2020-07-27 17:29 ` [Linux-kernel-mentees] [PATCH v3] " Vaibhav Gupta
2020-07-27 19:21 ` Andy Shevchenko
2020-07-28 16:31 ` Mark Brown
2020-07-28 16:31 ` [Linux-kernel-mentees] [PATCH v2] " Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHp75VdSr1rguc9HJVh_rA1nBh1uyCdr18eyPosWPzCH1K2=zg@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=bhelgaas@google.com \
--cc=broonie@kernel.org \
--cc=helgaas@kernel.org \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=vaibhavgupta40@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).