mbox series

[v2,0/9] PM: sleep: core: Rearrange the handling of driver power management flags

Message ID 5673945.BT02kTCndr@kreacher
Headers show
Series PM: sleep: core: Rearrange the handling of driver power management flags | expand

Message

Rafael J. Wysocki April 18, 2020, 4:23 p.m. UTC
Hi,

This is an update including some fixes and extra patches based on the
continuation of the discussion [1].

On Friday, April 10, 2020 5:46:27 PM CEST Rafael J. Wysocki wrote:
> Hi Alan,
> 
> Following our recent discussion regarding the DPM_FLAG_* family of flags [1],
> I have decided to follow some of your recommendations and make changes to the
> core code handling those flags.
> 
> The purpose of this is basically to make the code more consistent internally,
> easier to follow and better documented.
> 
> First of all, patch [1/7] changes the PM core to skip driver-level "late"
> and "noirq" suspend callbacks for devices with SMART_SUSPEND set if they are
> still runtime-suspended during the "late" system-wide suspend phase (without
> the patch it does that only if subsystem-level late/noirq/early suspend/resume
> callbacks are not present for the device, which is demonstrably inconsistent)
> and updates the resume part of the code accordingly (it doesn't need to check
> whether or not the subsystem-level callbacks are present any more).
> 
> The next patch, [2/7], is purely cosmetic and its only purpose is to reduce
> the LOC number and move related pieces of code closer to each other.

The first two patches have not changed.

> Patch [3/7] changes the PM core so that it doesn't skip any subsystem-level
> callbacks during system-wide resume (without the patch they may be skipped in
> the "early resume" and "resume" phases due to LEAVE_SUSPENDED being set which
> may be problematic) and to always run the driver's ->resume callback if the
> corresponding subsystem-level callback is not present (without the patch it
> may be skipped if LEAVE_SUSPENDED is set) to let it reverse the changes made
> by the driver's ->suspend callback (which always runs too) if need be.

The difference between this one and patch [3/9] in the v2 is the fixed
definition of dev_pm_may_skip_resume(), renamed to dev_pm_skip_resume() by
one of the next patches.

Patch [4/9] changes the handling of the power.may_skip_resume flag to set it
to 'true' by default and updates the subsystems aware of it to clear it when
they don't want devices to stay in suspend.

> Patches [4-6/7] rename one function in the PM core and two driver PM flags to
> make their names better reflect their purpose.

These are patches [5/9] and [7-8/9] in the v2 and patch [6/9] renames
dev_pm_smart_suspend_and_suspended() to dev_pm_skip_suspend().

> Finally, patch [7/7] updates the documentation of the driver PM flags to
> reflect the new code flows.

This patch [9/9] now and it has been updated to reflect the new code changes.

The pm-sleep-core branch:

  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
  pm-sleep-core

contains the v2 now.

Cheers!


[1] https://lore.kernel.org/linux-pm/Pine.LNX.4.44L0.2003251631360.1724-100000@netrider.rowland.org/

Comments

Alan Stern April 18, 2020, 6 p.m. UTC | #1
On Sat, 18 Apr 2020, Rafael J. Wysocki wrote:

> Hi,
> 
> This is an update including some fixes and extra patches based on the
> continuation of the discussion [1].

I haven't checked the updates in detail yet.  However, it seems that 
dev_pm_skip_suspend() and dev_pm_skip_resume() should be EXPORTed, 
since they are intended to be used by subsystems, which may be in 
modules.

Alan Stern
Rafael J. Wysocki April 18, 2020, 6:08 p.m. UTC | #2
On Sat, Apr 18, 2020 at 8:00 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Sat, 18 Apr 2020, Rafael J. Wysocki wrote:
>
> > Hi,
> >
> > This is an update including some fixes and extra patches based on the
> > continuation of the discussion [1].
>
> I haven't checked the updates in detail yet.  However, it seems that
> dev_pm_skip_suspend() and dev_pm_skip_resume() should be EXPORTed,
> since they are intended to be used by subsystems, which may be in
> modules.

OK, so what about an extra patch to export them?

Currently there are no modular users of these functions.
Alan Stern April 18, 2020, 7:41 p.m. UTC | #3
On Sat, 18 Apr 2020, Rafael J. Wysocki wrote:

> On Sat, Apr 18, 2020 at 8:00 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Sat, 18 Apr 2020, Rafael J. Wysocki wrote:
> >
> > > Hi,
> > >
> > > This is an update including some fixes and extra patches based on the
> > > continuation of the discussion [1].
> >
> > I haven't checked the updates in detail yet.  However, it seems that
> > dev_pm_skip_suspend() and dev_pm_skip_resume() should be EXPORTed,
> > since they are intended to be used by subsystems, which may be in
> > modules.
> 
> OK, so what about an extra patch to export them?
> 
> Currently there are no modular users of these functions.

Ah, all right.  So when/if I want to use them, I will submit such a 
patch.

Alan Stern
Alan Stern April 19, 2020, 2:43 p.m. UTC | #4
On Sat, 18 Apr 2020, Rafael J. Wysocki wrote:

> Hi,
> 
> This is an update including some fixes and extra patches based on the
> continuation of the discussion [1].

The new code in pm.h and main.c all looks good.  Please add my
Acked-by: or Suggested-by: to the portions of the patches that affect
those files.

It's nice to see that, aside from the documentation, the patches ended
up removing more lines than they added.

Alan Stern
Rafael J. Wysocki April 19, 2020, 3:13 p.m. UTC | #5
On Sun, Apr 19, 2020 at 4:43 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Sat, 18 Apr 2020, Rafael J. Wysocki wrote:
>
> > Hi,
> >
> > This is an update including some fixes and extra patches based on the
> > continuation of the discussion [1].
>
> The new code in pm.h and main.c all looks good.  Please add my
> Acked-by: or Suggested-by: to the portions of the patches that affect
> those files.

I will, thank you!

> It's nice to see that, aside from the documentation, the patches ended
> up removing more lines than they added.

Indeed.

Cheers!
Ulf Hansson April 21, 2020, 10:30 a.m. UTC | #6
On Sat, 18 Apr 2020 at 19:11, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi,
>
> This is an update including some fixes and extra patches based on the
> continuation of the discussion [1].
>
> On Friday, April 10, 2020 5:46:27 PM CEST Rafael J. Wysocki wrote:
> > Hi Alan,
> >
> > Following our recent discussion regarding the DPM_FLAG_* family of flags [1],
> > I have decided to follow some of your recommendations and make changes to the
> > core code handling those flags.
> >
> > The purpose of this is basically to make the code more consistent internally,
> > easier to follow and better documented.
> >
> > First of all, patch [1/7] changes the PM core to skip driver-level "late"
> > and "noirq" suspend callbacks for devices with SMART_SUSPEND set if they are
> > still runtime-suspended during the "late" system-wide suspend phase (without
> > the patch it does that only if subsystem-level late/noirq/early suspend/resume
> > callbacks are not present for the device, which is demonstrably inconsistent)
> > and updates the resume part of the code accordingly (it doesn't need to check
> > whether or not the subsystem-level callbacks are present any more).
> >
> > The next patch, [2/7], is purely cosmetic and its only purpose is to reduce
> > the LOC number and move related pieces of code closer to each other.
>
> The first two patches have not changed.
>
> > Patch [3/7] changes the PM core so that it doesn't skip any subsystem-level
> > callbacks during system-wide resume (without the patch they may be skipped in
> > the "early resume" and "resume" phases due to LEAVE_SUSPENDED being set which
> > may be problematic) and to always run the driver's ->resume callback if the
> > corresponding subsystem-level callback is not present (without the patch it
> > may be skipped if LEAVE_SUSPENDED is set) to let it reverse the changes made
> > by the driver's ->suspend callback (which always runs too) if need be.
>
> The difference between this one and patch [3/9] in the v2 is the fixed
> definition of dev_pm_may_skip_resume(), renamed to dev_pm_skip_resume() by
> one of the next patches.
>
> Patch [4/9] changes the handling of the power.may_skip_resume flag to set it
> to 'true' by default and updates the subsystems aware of it to clear it when
> they don't want devices to stay in suspend.
>
> > Patches [4-6/7] rename one function in the PM core and two driver PM flags to
> > make their names better reflect their purpose.
>
> These are patches [5/9] and [7-8/9] in the v2 and patch [6/9] renames
> dev_pm_smart_suspend_and_suspended() to dev_pm_skip_suspend().
>
> > Finally, patch [7/7] updates the documentation of the driver PM flags to
> > reflect the new code flows.
>
> This patch [9/9] now and it has been updated to reflect the new code changes.
>
> The pm-sleep-core branch:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>   pm-sleep-core
>
> contains the v2 now.
>
> Cheers!

Rafael, apologize for taking some time to review and respond. I
noticed you have queued this up on your next branch by now, good.

In any case, I have looked through the series and I think it looks good, thanks!

Kind regards
Uffe

>
>
> [1] https://lore.kernel.org/linux-pm/Pine.LNX.4.44L0.2003251631360.1724-100000@netrider.rowland.org/
>
>
>
Rafael J. Wysocki April 21, 2020, 11:32 a.m. UTC | #7
On Tue, Apr 21, 2020 at 12:30 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Sat, 18 Apr 2020 at 19:11, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi,
> >
> > This is an update including some fixes and extra patches based on the
> > continuation of the discussion [1].
> >
> > On Friday, April 10, 2020 5:46:27 PM CEST Rafael J. Wysocki wrote:
> > > Hi Alan,
> > >
> > > Following our recent discussion regarding the DPM_FLAG_* family of flags [1],
> > > I have decided to follow some of your recommendations and make changes to the
> > > core code handling those flags.
> > >
> > > The purpose of this is basically to make the code more consistent internally,
> > > easier to follow and better documented.
> > >
> > > First of all, patch [1/7] changes the PM core to skip driver-level "late"
> > > and "noirq" suspend callbacks for devices with SMART_SUSPEND set if they are
> > > still runtime-suspended during the "late" system-wide suspend phase (without
> > > the patch it does that only if subsystem-level late/noirq/early suspend/resume
> > > callbacks are not present for the device, which is demonstrably inconsistent)
> > > and updates the resume part of the code accordingly (it doesn't need to check
> > > whether or not the subsystem-level callbacks are present any more).
> > >
> > > The next patch, [2/7], is purely cosmetic and its only purpose is to reduce
> > > the LOC number and move related pieces of code closer to each other.
> >
> > The first two patches have not changed.
> >
> > > Patch [3/7] changes the PM core so that it doesn't skip any subsystem-level
> > > callbacks during system-wide resume (without the patch they may be skipped in
> > > the "early resume" and "resume" phases due to LEAVE_SUSPENDED being set which
> > > may be problematic) and to always run the driver's ->resume callback if the
> > > corresponding subsystem-level callback is not present (without the patch it
> > > may be skipped if LEAVE_SUSPENDED is set) to let it reverse the changes made
> > > by the driver's ->suspend callback (which always runs too) if need be.
> >
> > The difference between this one and patch [3/9] in the v2 is the fixed
> > definition of dev_pm_may_skip_resume(), renamed to dev_pm_skip_resume() by
> > one of the next patches.
> >
> > Patch [4/9] changes the handling of the power.may_skip_resume flag to set it
> > to 'true' by default and updates the subsystems aware of it to clear it when
> > they don't want devices to stay in suspend.
> >
> > > Patches [4-6/7] rename one function in the PM core and two driver PM flags to
> > > make their names better reflect their purpose.
> >
> > These are patches [5/9] and [7-8/9] in the v2 and patch [6/9] renames
> > dev_pm_smart_suspend_and_suspended() to dev_pm_skip_suspend().
> >
> > > Finally, patch [7/7] updates the documentation of the driver PM flags to
> > > reflect the new code flows.
> >
> > This patch [9/9] now and it has been updated to reflect the new code changes.
> >
> > The pm-sleep-core branch:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> >   pm-sleep-core
> >
> > contains the v2 now.
> >
> > Cheers!
>
> Rafael, apologize for taking some time to review and respond. I
> noticed you have queued this up on your next branch by now, good.
>
> In any case, I have looked through the series and I think it looks good, thanks!

Thanks for letting me know!

Cheers!
Bjorn Helgaas April 23, 2020, 5:07 p.m. UTC | #8
On Sat, Apr 18, 2020 at 06:23:08PM +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> This is an update including some fixes and extra patches based on the
> continuation of the discussion [1].
> 
> On Friday, April 10, 2020 5:46:27 PM CEST Rafael J. Wysocki wrote:
> > Hi Alan,
> > 
> > Following our recent discussion regarding the DPM_FLAG_* family of flags [1],
> > I have decided to follow some of your recommendations and make changes to the
> > core code handling those flags.
> > 
> > The purpose of this is basically to make the code more consistent internally,
> > easier to follow and better documented.
> > 
> > First of all, patch [1/7] changes the PM core to skip driver-level "late"
> > and "noirq" suspend callbacks for devices with SMART_SUSPEND set if they are
> > still runtime-suspended during the "late" system-wide suspend phase (without
> > the patch it does that only if subsystem-level late/noirq/early suspend/resume
> > callbacks are not present for the device, which is demonstrably inconsistent)
> > and updates the resume part of the code accordingly (it doesn't need to check
> > whether or not the subsystem-level callbacks are present any more).
> > 
> > The next patch, [2/7], is purely cosmetic and its only purpose is to reduce
> > the LOC number and move related pieces of code closer to each other.
> 
> The first two patches have not changed.
> 
> > Patch [3/7] changes the PM core so that it doesn't skip any subsystem-level
> > callbacks during system-wide resume (without the patch they may be skipped in
> > the "early resume" and "resume" phases due to LEAVE_SUSPENDED being set which
> > may be problematic) and to always run the driver's ->resume callback if the
> > corresponding subsystem-level callback is not present (without the patch it
> > may be skipped if LEAVE_SUSPENDED is set) to let it reverse the changes made
> > by the driver's ->suspend callback (which always runs too) if need be.
> 
> The difference between this one and patch [3/9] in the v2 is the fixed
> definition of dev_pm_may_skip_resume(), renamed to dev_pm_skip_resume() by
> one of the next patches.
> 
> Patch [4/9] changes the handling of the power.may_skip_resume flag to set it
> to 'true' by default and updates the subsystems aware of it to clear it when
> they don't want devices to stay in suspend.
> 
> > Patches [4-6/7] rename one function in the PM core and two driver PM flags to
> > make their names better reflect their purpose.
> 
> These are patches [5/9] and [7-8/9] in the v2 and patch [6/9] renames
> dev_pm_smart_suspend_and_suspended() to dev_pm_skip_suspend().
> 
> > Finally, patch [7/7] updates the documentation of the driver PM flags to
> > reflect the new code flows.
> 
> This patch [9/9] now and it has been updated to reflect the new code changes.
> 
> The pm-sleep-core branch:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>   pm-sleep-core
> 
> contains the v2 now.

For the drivers/pci parts:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>