Re: [PATCH 2/2] uswsusp: build only when configured

[Domenico Andreoli <domenico.andreoli@linux.com>]

On Sun, Apr 26, 2020 at 06:16:29PM +0200, Rafael J. Wysocki wrote:
> On Monday, April 13, 2020 9:08:45 PM CEST Domenico Andreoli wrote:
> > From: Domenico Andreoli <domenico.andreoli@linux.com>
> > 
> > uswsusp is no longer the preferred way to suspend/hibernate and the
> > userspace tools have not received any update in years.
> > 
> > Make it possible to enable the uswsusp support only if needed, prepare
> > for future phase out.
> > 
> > Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Linux PM <linux-pm@vger.kernel.org>
> > 
> > ---
> >  kernel/power/Kconfig  |    5 +++++
> >  kernel/power/Makefile |    3 ++-
> >  kernel/power/power.h  |    5 +++++
> >  3 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > Index: b/kernel/power/Kconfig
> > ===================================================================
> > --- a/kernel/power/Kconfig
> > +++ b/kernel/power/Kconfig
> > @@ -80,6 +80,11 @@ config HIBERNATION
> >  
> >  	  For more information take a look at <file:Documentation/power/swsusp.rst>.
> >  
> > +config HIBERNATION_USER
> > +	bool "Userspace software suspend interface (DEPRECATED)"
> > +	depends on HIBERNATION
> > +	default n
> 
> This needs to be "default y" for the time being.
> 
> Also, I would call the option HIBERNATION_SNAPSHOT_DEV, because it effectively
> controls whether or not the snapshot device is available.
> 
> > +
> >  config PM_STD_PARTITION
> >  	string "Default resume partition"
> >  	depends on HIBERNATION
> > Index: b/kernel/power/Makefile
> > ===================================================================
> > --- a/kernel/power/Makefile
> > +++ b/kernel/power/Makefile
> > @@ -10,7 +10,8 @@ obj-$(CONFIG_VT_CONSOLE_SLEEP)	+= consol
> >  obj-$(CONFIG_FREEZER)		+= process.o
> >  obj-$(CONFIG_SUSPEND)		+= suspend.o
> >  obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
> > -obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
> > +obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o
> > +obj-$(CONFIG_HIBERNATION_USER)	+= user.o
> >  obj-$(CONFIG_PM_AUTOSLEEP)	+= autosleep.o
> >  obj-$(CONFIG_PM_WAKELOCKS)	+= wakelock.o
> >  
> > Index: b/kernel/power/power.h
> > ===================================================================
> > --- a/kernel/power/power.h
> > +++ b/kernel/power/power.h
> > @@ -158,8 +158,13 @@ extern sector_t alloc_swapdev_block(int
> >  extern void free_all_swap_pages(int swap);
> >  extern int swsusp_swap_in_use(void);
> >  
> > +#ifdef CONFIG_HIBERNATION_USER
> >  bool swsusp_try_enter(void);
> >  void swsusp_leave(void);
> > +#else
> > +static inline bool swsusp_try_enter(void) { return 1; }
> > +static inline void swsusp_leave(void) {}
> > +#endif
> 
> It is possible in theory that two processes write "disk" to /sys/power/state
> concurrently.
> 
> Is there enough mutual exclusion in place to handle this gracefully after the
> above change?

No, indeed.

It looks like hibernate.c needs the mutual exclusion and user.c could
just use it. Should I move snapshot_device_available to hibernate.c
and rename it hibernate_available?

-- 
rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05