From: Andy Shevchenko <andy.shevchenko@gmail.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: Vaibhav Gupta <vaibhavgupta40@gmail.com>, Bjorn Helgaas <bhelgaas@google.com>, Bjorn Helgaas <bjorn@helgaas.com>, Vaibhav Gupta <vaibhav.varodek@gmail.com>, Mark Brown <broonie@kernel.org>, linux-spi <linux-spi@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-kernel-mentees@lists.linuxfoundation.org, Shuah Khan <skhan@linuxfoundation.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net> Subject: Re: [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
WARNING: multiple messages have this Message-ID (diff)
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:42 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-20 15:57 [PATCH v1] spi: spi-topcliff-pch: use generic power management Vaibhav Gupta 2020-07-20 15:57 ` [Linux-kernel-mentees] " Vaibhav Gupta 2020-07-22 13:45 ` Mark Brown 2020-07-22 13:45 ` [Linux-kernel-mentees] " Mark Brown 2020-07-22 20:01 ` Vaibhav Gupta 2020-07-22 20:01 ` [Linux-kernel-mentees] " Vaibhav Gupta 2020-07-24 10:51 ` Andy Shevchenko 2020-07-24 10:51 ` [Linux-kernel-mentees] " Andy Shevchenko 2020-07-24 15:16 ` Vaibhav Gupta 2020-07-24 15:16 ` [Linux-kernel-mentees] " Vaibhav Gupta 2020-07-24 20:16 ` Andy Shevchenko 2020-07-24 20:16 ` [Linux-kernel-mentees] " Andy Shevchenko 2020-07-24 22:37 ` Bjorn Helgaas 2020-07-24 22:37 ` [Linux-kernel-mentees] " Bjorn Helgaas 2020-07-25 10:42 ` Andy Shevchenko [this message] 2020-07-25 10:42 ` Andy Shevchenko 2020-07-25 10:44 ` Andy Shevchenko 2020-07-25 10:44 ` [Linux-kernel-mentees] " Andy Shevchenko 2020-07-27 7:06 ` Vaibhav Gupta 2020-07-27 7:06 ` [Linux-kernel-mentees] " Vaibhav Gupta 2020-07-27 11:12 ` Andy Shevchenko 2020-07-27 11:12 ` [Linux-kernel-mentees] " Andy Shevchenko 2020-07-27 13:08 ` Vaibhav Gupta 2020-07-27 13:08 ` [Linux-kernel-mentees] " Vaibhav Gupta 2020-07-27 13:17 ` [PATCH v2] spi: spi-topcliff-pch: drop call to wakeup-disable Vaibhav Gupta 2020-07-27 13:17 ` [Linux-kernel-mentees] " Vaibhav Gupta 2020-07-27 13:38 ` Andy Shevchenko 2020-07-27 13:38 ` [Linux-kernel-mentees] " Andy Shevchenko 2020-07-27 13:46 ` Vaibhav Gupta 2020-07-27 13:46 ` [Linux-kernel-mentees] " Vaibhav Gupta 2020-07-27 14:08 ` Andy Shevchenko 2020-07-27 14:08 ` [Linux-kernel-mentees] " Andy Shevchenko 2020-07-27 14:17 ` Joe Perches 2020-07-27 14:17 ` [Linux-kernel-mentees] " Joe Perches 2020-07-27 17:29 ` [PATCH v3] " Vaibhav Gupta 2020-07-27 17:29 ` [Linux-kernel-mentees] " Vaibhav Gupta 2020-07-27 19:21 ` Andy Shevchenko 2020-07-27 19:21 ` [Linux-kernel-mentees] " Andy Shevchenko 2020-07-28 16:31 ` Mark Brown 2020-07-28 16:31 ` [Linux-kernel-mentees] " Mark Brown 2020-07-28 16:31 ` [PATCH v2] " Mark Brown 2020-07-28 16:31 ` [Linux-kernel-mentees] " 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=bjorn@helgaas.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=skhan@linuxfoundation.org \ --cc=vaibhav.varodek@gmail.com \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.