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 <[email protected]>
---
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
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 <[email protected]> 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
On Fri, Jul 24, 2020 at 01:51:49PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 20, 2020 at 7:31 PM Vaibhav Gupta <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
[+cc Rafael, in case you can clear up our wakeup confusion]
original patch:
https://lore.kernel.org/r/[email protected]
On Fri, Jul 24, 2020 at 11:16:55PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 24, 2020 at 6:17 PM Vaibhav Gupta <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>
> [+cc Rafael, in case you can clear up our wakeup confusion]
> original patch:
> https://lore.kernel.org/r/[email protected]
>
> On Fri, Jul 24, 2020 at 11:16:55PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 24, 2020 at 6:17 PM Vaibhav Gupta <[email protected]> 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 <[email protected]> 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
On Sat, Jul 25, 2020 at 1:42 PM Andy Shevchenko
<[email protected]> wrote:
> On Sat, Jul 25, 2020 at 1:37 AM Bjorn Helgaas <[email protected]> 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
On Sat, Jul 25, 2020 at 01:44:44PM +0300, Andy Shevchenko wrote:
> On Sat, Jul 25, 2020 at 1:42 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Sat, Jul 25, 2020 at 1:37 AM Bjorn Helgaas <[email protected]> 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 <[email protected]> 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
> > <[email protected]> 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
On Mon, Jul 27, 2020 at 02:12:16PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 27, 2020 at 10:08 AM Vaibhav Gupta <[email protected]> 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
> > > <[email protected]> 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
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 <[email protected]>
Signed-off-by: Vaibhav Gupta <[email protected]>
---
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
On Mon, Jul 27, 2020 at 4:21 PM Vaibhav Gupta <[email protected]> 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 <[email protected]>
Missed dash. Does checkpatch complain?
> Signed-off-by: Vaibhav Gupta <[email protected]>
> ---
> 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
On Mon, Jul 27, 2020 at 04:38:40PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 27, 2020 at 4:21 PM Vaibhav Gupta <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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
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 <[email protected]> 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 <[email protected]> wrote:
>
> ...
>
> > > > Fix: f185bcc77980("spi: spi-topcliff-pch: use generic power management")
> > >
> > > Fixes: and missed space.
>
> (1)
>
> > > > Reported by: Andy Shevchenko <[email protected]>
> > >
> > > 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.
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 <[email protected]> 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 <[email protected]> wrote:
...
> > > Fix: f185bcc77980("spi: spi-topcliff-pch: use generic power management")
> >
> > Fixes: and missed space.
(1)
> > > Reported by: Andy Shevchenko <[email protected]>
> >
> > 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
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 <[email protected]>
Signed-off-by: Vaibhav Gupta <[email protected]>
---
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
On Mon, Jul 27, 2020 at 8:32 PM Vaibhav Gupta <[email protected]> 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 <[email protected]>
Thanks!
> Fixes: f185bcc77980 ("spi: spi-topcliff-pch: use generic power management")
> Reported-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Vaibhav Gupta <[email protected]>
> ---
> 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
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
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