[RFC/RFT] PM: sleep: Ignore device driver suspend() callback return values

From: Len Brown <len.brown@intel.com>

Drivers commonly return non-zero values from their suspend()
callbacks due to transient errors, not realizing that doing so
aborts system-wide suspend.

Ignore those return values.

Both before and after this patch, the correct method for a
device driver to abort system-wide suspend is to invoke
pm_system_wakeup() during the suspend flow.

Legacy behaviour can be restored by adding this line to your .config:
CONFIG_PM_SLEEP_LEGACY_CALLBACK_ABORT=y

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/base/power/main.c |  4 ++++
 kernel/power/Kconfig      | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

On Wed, Nov 13, 2024 at 7:35 PM Len Brown <lenb@kernel.org> wrote:
>
> From: Len Brown <len.brown@intel.com>
>
> Drivers commonly return non-zero values from their suspend()
> callbacks due to transient errors, not realizing that doing so
> aborts system-wide suspend.
>
> Ignore those return values.
>
> Both before and after this patch, the correct method for a
> device driver to abort system-wide suspend is to invoke
> pm_system_wakeup() during the suspend flow.
>
> Legacy behaviour can be restored by adding this line to your .config:
> CONFIG_PM_SLEEP_LEGACY_CALLBACK_ABORT=y
>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/base/power/main.c |  4 ++++
>  kernel/power/Kconfig      | 14 ++++++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 4a67e83300e1..56b7c9c752b4 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1678,7 +1678,11 @@ static int device_suspend(struct device *dev, pm_message_t state, bool async)
>                 callback = pm_op(dev->driver->pm, state);
>         }
>
> +#if CONFIG_PM_SLEEP_LEGACY_CALLBACK_ABORT
>         error = dpm_run_callback(callback, dev, state, info);
> +#else
> +       dpm_run_callback(callback, dev, state, info);
> +#endif
>
>   End:
>         if (!error) {

I would prefer something like:

    error = dpm_run_callback(callback, dev, state, info);

 End:
    if (!IS_ENABLED(PM_SLEEP_LEGACY_CALLBACK_ABORT)
            error = 0;

    if (!error ) {
        dev->power.is_suspended = true;
        if (device_may_wakeup(dev))
            dev->power.wakeup_path = true;

        dpm_propagate_wakeup_to_parent(dev);
        dpm_clear_superiors_direct_complete(dev);
    }

and analogously in _noirq() and _late().

> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index afce8130d8b9..51b5d6c9bf1a 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -141,6 +141,20 @@ config PM_SLEEP
>         depends on SUSPEND || HIBERNATE_CALLBACKS
>         select PM
>
> +config PM_SLEEP_LEGACY_CALLBACK_ABORT
> +       def_bool n

This isn't needed.

> +       depends on PM_SLEEP
> +       help
> +       This option enables the legacy API for device .suspend() callbacks.
> +       That API empowered any driver to abort system-wide suspend
> +       by returning any non-zero value from its .suspend() callback.
> +       In practice, these aborts are almost always spurious and unwanted.
> +
> +       Disabling this option (default) ignores .suspend() callback return values.
> +
> +       In both cases, any driver can abort system wide suspend by invoking
> +       pm_system_wakeup() during the suspend flow.
> +
>  config PM_SLEEP_SMP
>         def_bool y
>         depends on SMP
> --

It would be good to update the PM documentation too to take this new
option into account.

On Wed, Nov 13, 2024 at 8:23 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Nov 13, 2024 at 7:35 PM Len Brown <lenb@kernel.org> wrote:
> >
> > From: Len Brown <len.brown@intel.com>
> >
> > Drivers commonly return non-zero values from their suspend()
> > callbacks due to transient errors, not realizing that doing so
> > aborts system-wide suspend.
> >
> > Ignore those return values.
> >
> > Both before and after this patch, the correct method for a
> > device driver to abort system-wide suspend is to invoke
> > pm_system_wakeup() during the suspend flow.
> >
> > Legacy behaviour can be restored by adding this line to your .config:
> > CONFIG_PM_SLEEP_LEGACY_CALLBACK_ABORT=y
> >
> > Signed-off-by: Len Brown <len.brown@intel.com>
> > ---
> >  drivers/base/power/main.c |  4 ++++
> >  kernel/power/Kconfig      | 14 ++++++++++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 4a67e83300e1..56b7c9c752b4 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1678,7 +1678,11 @@ static int device_suspend(struct device *dev, pm_message_t state, bool async)
> >                 callback = pm_op(dev->driver->pm, state);
> >         }
> >
> > +#if CONFIG_PM_SLEEP_LEGACY_CALLBACK_ABORT
> >         error = dpm_run_callback(callback, dev, state, info);
> > +#else
> > +       dpm_run_callback(callback, dev, state, info);
> > +#endif
> >
> >   End:
> >         if (!error) {
>
> I would prefer something like:
>
>     error = dpm_run_callback(callback, dev, state, info);
>
>  End:
>     if (!IS_ENABLED(PM_SLEEP_LEGACY_CALLBACK_ABORT)
>             error = 0;
>
>     if (!error ) {
>         dev->power.is_suspended = true;
>         if (device_may_wakeup(dev))
>             dev->power.wakeup_path = true;
>
>         dpm_propagate_wakeup_to_parent(dev);
>         dpm_clear_superiors_direct_complete(dev);
>     }
>
> and analogously in _noirq() and _late().

Or even

    error = dpm_run_callback(callback, dev, state, info);

 End:
    if (!error || !IS_ENABLED(PM_SLEEP_LEGACY_CALLBACK_ABORT)) {
        dev->power.is_suspended = true;
        if (device_may_wakeup(dev))
            dev->power.wakeup_path = true;

        dpm_propagate_wakeup_to_parent(dev);
        dpm_clear_superiors_direct_complete(dev);
    }

    device_unlock(dev);
    dpm_watchdog_clear(&wd);

 Complete:
    if (error) {
        dpm_save_failed_dev(dev_name(dev));
        pm_dev_err(dev, state, async ? " async" : "", error);
    }

    complete_all(&dev->power.completion);
    TRACE_SUSPEND(error);
    if (IS_ENABLED(PM_SLEEP_LEGACY_CALLBACK_ABORT) {
        async_error = error;
        return error;
    }

    return 0;

So I want the error messages to be printed and TRACE_SUSPEND() to
record the error, but to return 0 in the end if the new option is not
set.

Also please CC the next version to the LKML.