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

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

[Vaibhav Gupta]

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(-)

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)
-- 
2.27.0

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

[Mark Brown]

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

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

[Vaibhav Gupta]

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

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

[Andy Shevchenko]

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;
>  }


-- 
With Best Regards,
Andy Shevchenko

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

[Vaibhav Gupta]

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

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

[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.

-- 
With Best Regards,
Andy Shevchenko

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

[Bjorn Helgaas]

[+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

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

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.


-- 
With Best Regards,
Andy Shevchenko

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

[Vaibhav Gupta]

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

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

[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.

-- 
With Best Regards,
Andy Shevchenko

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

[Vaibhav Gupta]

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

[PATCH v2] spi: spi-topcliff-pch: drop call to wakeup-disable

[Vaibhav Gupta]

Before generic upgrade, both .suspend() and .resume() were invoking
pci_enable_wake(pci_dev, PCI_D3hot, 0). Hence, disabling wakeup in both
states. (Normal trend is .suspend() enables and .resume() disables the
wakeup.)

This was ambiguous and may be buggy. Instead of replicating the legacy
behavior, drop the wakeup-disable call.

Fix: f185bcc77980("spi: spi-topcliff-pch: use generic power management")
Reported by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/spi/spi-topcliff-pch.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-topcliff-pch.c b/drivers/spi/spi-topcliff-pch.c
index 281a90f1b5d8..c73a03ddf5f3 100644
--- a/drivers/spi/spi-topcliff-pch.c
+++ b/drivers/spi/spi-topcliff-pch.c
@@ -1648,8 +1648,6 @@ static int __maybe_unused pch_spi_resume(struct device *dev)
 
 	dev_dbg(dev, "%s ENTRY\n", __func__);
 
-	device_wakeup_disable(dev);
-
 	/* set suspend status to false */
 	pd_dev_save->board_dat->suspend_sts = false;
 
-- 
2.27.0

Re: [PATCH v2] spi: spi-topcliff-pch: drop call to wakeup-disable

[Andy Shevchenko]

On Mon, Jul 27, 2020 at 4:21 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
>
> Before generic upgrade, both .suspend() and .resume() were invoking
> pci_enable_wake(pci_dev, PCI_D3hot, 0). Hence, disabling wakeup in both
> states. (Normal trend is .suspend() enables and .resume() disables the
> wakeup.)
>
> This was ambiguous and may be buggy. Instead of replicating the legacy
> behavior, drop the wakeup-disable call.
>

> Fix: f185bcc77980("spi: spi-topcliff-pch: use generic power management")

Fixes: and missed space.

Note:

% grep one ~/.gitconfig
       one = show -s --pretty='format:%h (\"%s\")'

% git one f185bcc77980
f185bcc77980 ("spi: spi-topcliff-pch: use generic power management")

> Reported by: Andy Shevchenko <andy.shevchenko@gmail.com>

Missed dash. Does checkpatch complain?

> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> ---
>  drivers/spi/spi-topcliff-pch.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/spi/spi-topcliff-pch.c b/drivers/spi/spi-topcliff-pch.c
> index 281a90f1b5d8..c73a03ddf5f3 100644
> --- a/drivers/spi/spi-topcliff-pch.c
> +++ b/drivers/spi/spi-topcliff-pch.c
> @@ -1648,8 +1648,6 @@ static int __maybe_unused pch_spi_resume(struct device *dev)
>
>         dev_dbg(dev, "%s ENTRY\n", __func__);
>
> -       device_wakeup_disable(dev);
> -
>         /* set suspend status to false */
>         pd_dev_save->board_dat->suspend_sts = false;
>
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] spi: spi-topcliff-pch: drop call to wakeup-disable

[Vaibhav Gupta]

On Mon, Jul 27, 2020 at 04:38:40PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 27, 2020 at 4:21 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> >
> > Before generic upgrade, both .suspend() and .resume() were invoking
> > pci_enable_wake(pci_dev, PCI_D3hot, 0). Hence, disabling wakeup in both
> > states. (Normal trend is .suspend() enables and .resume() disables the
> > wakeup.)
> >
> > This was ambiguous and may be buggy. Instead of replicating the legacy
> > behavior, drop the wakeup-disable call.
> >
> 
> > Fix: f185bcc77980("spi: spi-topcliff-pch: use generic power management")
> 
> Fixes: and missed space.
> 
> Note:
> 
> % grep one ~/.gitconfig
>        one = show -s --pretty='format:%h (\"%s\")'
> 
> % git one f185bcc77980
> f185bcc77980 ("spi: spi-topcliff-pch: use generic power management")
> 
> > Reported by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Missed dash.
> Does checkpatch complain?
No, I got this message:
"* .patch has no obvious style problems and is ready for submission"

I will fix the commit message.

Thanks!
Vaibhav Gupta
> 
> > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > ---
> >  drivers/spi/spi-topcliff-pch.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-topcliff-pch.c b/drivers/spi/spi-topcliff-pch.c
> > index 281a90f1b5d8..c73a03ddf5f3 100644
> > --- a/drivers/spi/spi-topcliff-pch.c
> > +++ b/drivers/spi/spi-topcliff-pch.c
> > @@ -1648,8 +1648,6 @@ static int __maybe_unused pch_spi_resume(struct device *dev)
> >
> >         dev_dbg(dev, "%s ENTRY\n", __func__);
> >
> > -       device_wakeup_disable(dev);
> > -
> >         /* set suspend status to false */
> >         pd_dev_save->board_dat->suspend_sts = false;
> >
> > --
> > 2.27.0
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] spi: spi-topcliff-pch: drop call to wakeup-disable

[Andy Shevchenko]

Joe, can we amend checkpatch to at least shout about simple typos in
the tag area?
See below for the context.

On Mon, Jul 27, 2020 at 4:48 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> On Mon, Jul 27, 2020 at 04:38:40PM +0300, Andy Shevchenko wrote:
> > On Mon, Jul 27, 2020 at 4:21 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:

...

> > > Fix: f185bcc77980("spi: spi-topcliff-pch: use generic power management")
> >
> > Fixes: and missed space.

(1)

> > > Reported by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> > Missed dash.
> > Does checkpatch complain?
> No, I got this message:
> "* .patch has no obvious style problems and is ready for submission"

(2)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] spi: spi-topcliff-pch: drop call to wakeup-disable

[Joe Perches]

On Mon, 2020-07-27 at 17:08 +0300, Andy Shevchenko wrote:
> Joe, can we amend checkpatch to at least shout about simple typos in
> the tag area?
> See below for the context.
> 
> On Mon, Jul 27, 2020 at 4:48 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> > On Mon, Jul 27, 2020 at 04:38:40PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jul 27, 2020 at 4:21 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
> 
> ...
> 
> > > > Fix: f185bcc77980("spi: spi-topcliff-pch: use generic power management")
> > > 
> > > Fixes: and missed space.
> 
> (1)
> 
> > > > Reported by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > 
> > > Missed dash.
> > > Does checkpatch complain?
> > No, I got this message:
> > "* .patch has no obvious style problems and is ready for submission"
> 
> (2)

Not reasonably so far as I can tell, no.

The test for a signature uses -by:

Fix: starting a line seems a reasonable thing
that someone might want to have in a commit
message.

[PATCH v3] spi: spi-topcliff-pch: drop call to wakeup-disable

[Vaibhav Gupta]

Before generic upgrade, both .suspend() and .resume() were invoking
pci_enable_wake(pci_dev, PCI_D3hot, 0). Hence, disabling wakeup in both
states. (Normal trend is .suspend() enables and .resume() disables the
wakeup.)

This was ambiguous and may be buggy. Instead of replicating the legacy
behavior, drop the wakeup-disable call.

Fixes: f185bcc77980 ("spi: spi-topcliff-pch: use generic power management")
Reported-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/spi/spi-topcliff-pch.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-topcliff-pch.c b/drivers/spi/spi-topcliff-pch.c
index 281a90f1b5d8..c73a03ddf5f3 100644
--- a/drivers/spi/spi-topcliff-pch.c
+++ b/drivers/spi/spi-topcliff-pch.c
@@ -1648,8 +1648,6 @@ static int __maybe_unused pch_spi_resume(struct device *dev)
 
 	dev_dbg(dev, "%s ENTRY\n", __func__);
 
-	device_wakeup_disable(dev);
-
 	/* set suspend status to false */
 	pd_dev_save->board_dat->suspend_sts = false;
 
-- 
2.27.0

Re: [PATCH v3] spi: spi-topcliff-pch: drop call to wakeup-disable

[Andy Shevchenko]

On Mon, Jul 27, 2020 at 8:32 PM Vaibhav Gupta <vaibhavgupta40@gmail.com> wrote:
>
> Before generic upgrade, both .suspend() and .resume() were invoking
> pci_enable_wake(pci_dev, PCI_D3hot, 0). Hence, disabling wakeup in both
> states. (Normal trend is .suspend() enables and .resume() disables the
> wakeup.)
>
> This was ambiguous and may be buggy. Instead of replicating the legacy
> behavior, drop the wakeup-disable call.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Thanks!

> Fixes: f185bcc77980 ("spi: spi-topcliff-pch: use generic power management")
> Reported-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> ---
>  drivers/spi/spi-topcliff-pch.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/spi/spi-topcliff-pch.c b/drivers/spi/spi-topcliff-pch.c
> index 281a90f1b5d8..c73a03ddf5f3 100644
> --- a/drivers/spi/spi-topcliff-pch.c
> +++ b/drivers/spi/spi-topcliff-pch.c
> @@ -1648,8 +1648,6 @@ static int __maybe_unused pch_spi_resume(struct device *dev)
>
>         dev_dbg(dev, "%s ENTRY\n", __func__);
>
> -       device_wakeup_disable(dev);
> -
>         /* set suspend status to false */
>         pd_dev_save->board_dat->suspend_sts = false;
>
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] spi: spi-topcliff-pch: drop call to wakeup-disable

[Mark Brown]

On Mon, 27 Jul 2020 22:59:37 +0530, Vaibhav Gupta wrote:
> Before generic upgrade, both .suspend() and .resume() were invoking
> pci_enable_wake(pci_dev, PCI_D3hot, 0). Hence, disabling wakeup in both
> states. (Normal trend is .suspend() enables and .resume() disables the
> wakeup.)
> 
> This was ambiguous and may be buggy. Instead of replicating the legacy
> behavior, drop the wakeup-disable call.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: spi-topcliff-pch: drop call to wakeup-disable
      commit: 15b413d93ccd0d26c29f005df82c299c8f14cbd6

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

Re: [PATCH v2] spi: spi-topcliff-pch: drop call to wakeup-disable

[Mark Brown]

On Mon, 27 Jul 2020 18:47:43 +0530, Vaibhav Gupta wrote:
> Before generic upgrade, both .suspend() and .resume() were invoking
> pci_enable_wake(pci_dev, PCI_D3hot, 0). Hence, disabling wakeup in both
> states. (Normal trend is .suspend() enables and .resume() disables the
> wakeup.)
> 
> This was ambiguous and may be buggy. Instead of replicating the legacy
> behavior, drop the wakeup-disable call.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: spi-topcliff-pch: drop call to wakeup-disable
      commit: 15b413d93ccd0d26c29f005df82c299c8f14cbd6

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