| Message ID | 20200720155714.714114-1-vaibhavgupta40@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f185bcc779808df5d31bc332b79b5f1455ee910b |
| Headers | show |
| Series | [v1] spi: spi-topcliff-pch: use generic power management | expand |
On Mon, 20 Jul 2020 21:27:15 +0530, Vaibhav Gupta wrote: > Drivers using legacy PM have to manage PCI states and device's PM states > themselves. They also need to take care of configuration registers. > > With improved and powerful support of generic PM, PCI Core takes care of > above mentioned, device-independent, jobs. > > This driver makes use of PCI helper functions like > pci_save/restore_state(), pci_enable/disable_device(), pci_enable_wake() > and pci_set_power_state() to do required operations. In generic mode, they > are no longer needed. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: spi-topcliff-pch: use generic power management commit: f185bcc779808df5d31bc332b79b5f1455ee910b All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Wed, Jul 22, 2020 at 02:45:17PM +0100, Mark Brown wrote: > On Mon, 20 Jul 2020 21:27:15 +0530, Vaibhav Gupta wrote: > > Drivers using legacy PM have to manage PCI states and device's PM states > > themselves. They also need to take care of configuration registers. > > > > With improved and powerful support of generic PM, PCI Core takes care of > > above mentioned, device-independent, jobs. > > > > This driver makes use of PCI helper functions like > > pci_save/restore_state(), pci_enable/disable_device(), pci_enable_wake() > > and pci_set_power_state() to do required operations. In generic mode, they > > are no longer needed. > > > > [...] > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next > > Thanks! > > [1/1] spi: spi-topcliff-pch: use generic power management > commit: f185bcc779808df5d31bc332b79b5f1455ee910b > > All being well this means that it will be integrated into the linux-next > tree (usually sometime in the next 24 hours) and sent to Linus during > the next merge window (or sooner if it is a bug fix), however if > problems are discovered then the patch may be dropped or reverted. > > You may get further e-mails resulting from automated or manual testing > and review of the tree, please engage with people reporting problems and > send followup patches addressing any issues that are reported if needed. > > If any updates are required or you are submitting further changes they > should be sent as incremental updates against current git, existing > patches will not be replaced. > > Please add any relevant lists and maintainers to the CCs when replying > to this mail. Thanks, --Vaibhav Gupta > > Thanks, > Mark
On Mon, Jul 20, 2020 at 7:31 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote: > > Drivers using legacy PM have to manage PCI states and device's PM states > themselves. They also need to take care of configuration registers. > > With improved and powerful support of generic PM, PCI Core takes care of > above mentioned, device-independent, jobs. > > This driver makes use of PCI helper functions like > pci_save/restore_state(), pci_enable/disable_device(), pci_enable_wake() > and pci_set_power_state() to do required operations. In generic mode, they > are no longer needed. > > Change function parameter in both .suspend() and .resume() to > "struct device*" type. Use dev_get_drvdata() to get drv data. > Compile-tested only. Yeah... ... > +static int __maybe_unused pch_spi_suspend(struct device *dev) > { > + struct pch_pd_dev_save *pd_dev_save = dev_get_drvdata(dev); > > + dev_dbg(dev, "%s ENTRY\n", __func__); > > pd_dev_save->board_dat->suspend_sts = true; > > + return 0; > } > > +static int __maybe_unused pch_spi_resume(struct device *dev) > { > + struct pch_pd_dev_save *pd_dev_save = dev_get_drvdata(dev); > > + dev_dbg(dev, "%s ENTRY\n", __func__); > > + 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? > + /* set suspend status to false */ > + pd_dev_save->board_dat->suspend_sts = false; > + return 0; > }
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: > > > > Drivers using legacy PM have to manage PCI states and device's PM states > > themselves. They also need to take care of configuration registers. > > > > With improved and powerful support of generic PM, PCI Core takes care of > > above mentioned, device-independent, jobs. > > > > This driver makes use of PCI helper functions like > > pci_save/restore_state(), pci_enable/disable_device(), pci_enable_wake() > > and pci_set_power_state() to do required operations. In generic mode, they > > are no longer needed. > > > > Change function parameter in both .suspend() and .resume() to > > "struct device*" type. Use dev_get_drvdata() to get drv data. > > > Compile-tested only. > > Yeah... > > ... > > > +static int __maybe_unused pch_spi_suspend(struct device *dev) > > { > > + struct pch_pd_dev_save *pd_dev_save = dev_get_drvdata(dev); > > > > + dev_dbg(dev, "%s ENTRY\n", __func__); > > > > pd_dev_save->board_dat->suspend_sts = true; > > > > + return 0; > > } > > > > +static int __maybe_unused pch_spi_resume(struct device *dev) > > { > > + struct pch_pd_dev_save *pd_dev_save = dev_get_drvdata(dev); > > > > + dev_dbg(dev, "%s ENTRY\n", __func__); > > > > > + 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? > Hello Andy, I didn't quite understand what you are trying to point at. And the result part. 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? Thanks Vaibhav Gupta > > + /* set suspend status to false */ > > + pd_dev_save->board_dat->suspend_sts = false; > > > + return 0; > > } > > > -- > With Best Regards, > Andy Shevchenko
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? > > > Hello Andy, > I didn't quite understand what you are trying to point at. And the result part. I emphasized the line by surrounding it with two blank lines followed by my comment. > 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.
[+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. 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: 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. 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. Bjorn
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.
On Sat, Jul 25, 2020 at 1:42 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Jul 25, 2020 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 24, 2020 at 11:16:55PM +0300, Andy Shevchenko wrote: ... > > 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. The only problem here, is that the 2nd is already in the Mark's tree and he doesn't do rebases. So, it will be the other way around.
On Sat, Jul 25, 2020 at 01:44:44PM +0300, Andy Shevchenko wrote: > On Sat, Jul 25, 2020 at 1:42 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sat, Jul 25, 2020 at 1:37 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Fri, Jul 24, 2020 at 11:16:55PM +0300, Andy Shevchenko wrote: > > ... > > > > 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. > > The only problem here, is that the 2nd is already in the Mark's tree > and he doesn't do rebases. > So, it will be the other way around. > Concluding from yours and Bjorn's suggestion, I will drop the device_wakeup_disable() call form .resume() and send the fix. I will also track the drivers who got similar upgrades and went un-noticed. As Bjorn mentioned, the problem is that I don't have hardware to test, so I just replicated the legacy behaviour in generic by replacing pci_enable_wake(....,false) with device_wakeup_disable(). So, from now, while upgrading drivers with generic PM, should I completely drop the pci_enable_wake(....,false) calls if both .suspend() and .resume() try to wakeup-disable the device? Thanks Vaibhav Gupta > > -- > With Best Regards, > Andy Shevchenko
On Mon, Jul 27, 2020 at 10:08 AM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote: > On Sat, Jul 25, 2020 at 01:44:44PM +0300, Andy Shevchenko wrote: > > On Sat, Jul 25, 2020 at 1:42 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: ... > > The only problem here, is that the 2nd is already in the Mark's tree > > and he doesn't do rebases. > > So, it will be the other way around. > > > Concluding from yours and Bjorn's suggestion, I will drop the > device_wakeup_disable() call form .resume() and send the fix. I will also track > the drivers who got similar upgrades and went un-noticed. Thanks for doing this! > As Bjorn mentioned, the problem is that I don't have hardware to test, so I just > replicated the legacy behaviour in generic by replacing > pci_enable_wake(....,false) with device_wakeup_disable(). > > So, from now, while upgrading drivers with generic PM, should I completely drop > the pci_enable_wake(....,false) calls if both .suspend() and .resume() try to > wakeup-disable the device? I guess the best approach is to rely on the PCI core to do the right thing. But mention this change in the commit message that we will have a track of the changes properly.
On Mon, Jul 27, 2020 at 02:12:16PM +0300, Andy Shevchenko wrote: > On Mon, Jul 27, 2020 at 10:08 AM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote: > > On Sat, Jul 25, 2020 at 01:44:44PM +0300, Andy Shevchenko wrote: > > > On Sat, Jul 25, 2020 at 1:42 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > ... > > > > The only problem here, is that the 2nd is already in the Mark's tree > > > and he doesn't do rebases. > > > So, it will be the other way around. > > > > > Concluding from yours and Bjorn's suggestion, I will drop the > > device_wakeup_disable() call form .resume() and send the fix. I will also track > > the drivers who got similar upgrades and went un-noticed. > > Thanks for doing this! > > > As Bjorn mentioned, the problem is that I don't have hardware to test, so I just > > replicated the legacy behaviour in generic by replacing > > pci_enable_wake(....,false) with device_wakeup_disable(). > > > > So, from now, while upgrading drivers with generic PM, should I completely drop > > the pci_enable_wake(....,false) calls if both .suspend() and .resume() try to > > wakeup-disable the device? > > I guess the best approach is to rely on the PCI core to do the right thing. > But mention this change in the commit message that we will have a > track of the changes properly. > Okay. Thanks ! Vaibhav Gupta > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/spi/spi-topcliff-pch.c b/drivers/spi/spi-topcliff-pch.c index d7ea6af74743..281a90f1b5d8 100644 --- a/drivers/spi/spi-topcliff-pch.c +++ b/drivers/spi/spi-topcliff-pch.c @@ -1631,64 +1631,39 @@ static void pch_spi_remove(struct pci_dev *pdev) kfree(pd_dev_save); } -#ifdef CONFIG_PM -static int pch_spi_suspend(struct pci_dev *pdev, pm_message_t state) +static int __maybe_unused pch_spi_suspend(struct device *dev) { - int retval; - struct pch_pd_dev_save *pd_dev_save = pci_get_drvdata(pdev); + struct pch_pd_dev_save *pd_dev_save = dev_get_drvdata(dev); - dev_dbg(&pdev->dev, "%s ENTRY\n", __func__); + dev_dbg(dev, "%s ENTRY\n", __func__); pd_dev_save->board_dat->suspend_sts = true; - /* save config space */ - retval = pci_save_state(pdev); - if (retval == 0) { - pci_enable_wake(pdev, PCI_D3hot, 0); - pci_disable_device(pdev); - pci_set_power_state(pdev, PCI_D3hot); - } else { - dev_err(&pdev->dev, "%s pci_save_state failed\n", __func__); - } - - return retval; + return 0; } -static int pch_spi_resume(struct pci_dev *pdev) +static int __maybe_unused pch_spi_resume(struct device *dev) { - int retval; - struct pch_pd_dev_save *pd_dev_save = pci_get_drvdata(pdev); - dev_dbg(&pdev->dev, "%s ENTRY\n", __func__); + struct pch_pd_dev_save *pd_dev_save = dev_get_drvdata(dev); - pci_set_power_state(pdev, PCI_D0); - pci_restore_state(pdev); + dev_dbg(dev, "%s ENTRY\n", __func__); - retval = pci_enable_device(pdev); - if (retval < 0) { - dev_err(&pdev->dev, - "%s pci_enable_device failed\n", __func__); - } else { - pci_enable_wake(pdev, PCI_D3hot, 0); + device_wakeup_disable(dev); - /* set suspend status to false */ - pd_dev_save->board_dat->suspend_sts = false; - } + /* set suspend status to false */ + pd_dev_save->board_dat->suspend_sts = false; - return retval; + return 0; } -#else -#define pch_spi_suspend NULL -#define pch_spi_resume NULL -#endif +static SIMPLE_DEV_PM_OPS(pch_spi_pm_ops, pch_spi_suspend, pch_spi_resume); static struct pci_driver pch_spi_pcidev_driver = { .name = "pch_spi", .id_table = pch_spi_pcidev_id, .probe = pch_spi_probe, .remove = pch_spi_remove, - .suspend = pch_spi_suspend, - .resume = pch_spi_resume, + .driver.pm = &pch_spi_pm_ops, }; static int __init pch_spi_init(void)
Drivers using legacy PM have to manage PCI states and device's PM states themselves. They also need to take care of configuration registers. With improved and powerful support of generic PM, PCI Core takes care of above mentioned, device-independent, jobs. This driver makes use of PCI helper functions like pci_save/restore_state(), pci_enable/disable_device(), pci_enable_wake() and pci_set_power_state() to do required operations. In generic mode, they are no longer needed. Change function parameter in both .suspend() and .resume() to "struct device*" type. Use dev_get_drvdata() to get drv data. Compile-tested only. Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> --- drivers/spi/spi-topcliff-pch.c | 51 +++++++++------------------------- 1 file changed, 13 insertions(+), 38 deletions(-)