[PATCH 0/2] Fix a race condition between SPI domain validation and system suspend

[PATCH 0/2] Fix a race condition between SPI domain validation and system suspend

[Bart Van Assche]

Hello Martin,

Recently it was discovered that the v4.15 rework of suspend/resume handling
in the SCSI subsystem introduced a race condition between SPI domain validation
and system suspend. The two patches in this series fix that race. Please
consider these two patches for kernel v4.15.

Thanks,

Bart.

Bart Van Assche (2):
  PM / sleep: Make lock/unlock_system_sleep() available to kernel
    modules
  Fix a race condition between SPI domain validation and system suspend

 drivers/scsi/scsi_transport_spi.c | 16 ++++++++++++++--
 include/linux/suspend.h           | 28 ++--------------------------
 kernel/power/main.c               | 29 +++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 28 deletions(-)

-- 
2.15.1

[PATCH 1/2] PM / sleep: Make lock/unlock_system_sleep() available to kernel modules

[Bart Van Assche]

Since pm_mutex is not exported using lock/unlock_system_sleep() from
inside a kernel module causes a "pm_mutex undefined" linker error.
Hence move lock/unlock_system_sleep() into kernel/power/main.c and
export these.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
---
 include/linux/suspend.h | 28 ++--------------------------
 kernel/power/main.c     | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index d60b0f5c38d5..cc22a24516d6 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -443,32 +443,8 @@ extern bool pm_save_wakeup_count(unsigned int count);
 extern void pm_wakep_autosleep_enabled(bool set);
 extern void pm_print_active_wakeup_sources(void);
 
-static inline void lock_system_sleep(void)
-{
-	current->flags |= PF_FREEZER_SKIP;
-	mutex_lock(&pm_mutex);
-}
-
-static inline void unlock_system_sleep(void)
-{
-	/*
-	 * Don't use freezer_count() because we don't want the call to
-	 * try_to_freeze() here.
-	 *
-	 * Reason:
-	 * Fundamentally, we just don't need it, because freezing condition
-	 * doesn't come into effect until we release the pm_mutex lock,
-	 * since the freezer always works with pm_mutex held.
-	 *
-	 * More importantly, in the case of hibernation,
-	 * unlock_system_sleep() gets called in snapshot_read() and
-	 * snapshot_write() when the freezing condition is still in effect.
-	 * Which means, if we use try_to_freeze() here, it would make them
-	 * enter the refrigerator, thus causing hibernation to lockup.
-	 */
-	current->flags &= ~PF_FREEZER_SKIP;
-	mutex_unlock(&pm_mutex);
-}
+extern void lock_system_sleep(void);
+extern void unlock_system_sleep(void);
 
 #else /* !CONFIG_PM_SLEEP */
 
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 3a2ca9066583..705c2366dafe 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -22,6 +22,35 @@ DEFINE_MUTEX(pm_mutex);
 
 #ifdef CONFIG_PM_SLEEP
 
+void lock_system_sleep(void)
+{
+	current->flags |= PF_FREEZER_SKIP;
+	mutex_lock(&pm_mutex);
+}
+EXPORT_SYMBOL_GPL(lock_system_sleep);
+
+void unlock_system_sleep(void)
+{
+	/*
+	 * Don't use freezer_count() because we don't want the call to
+	 * try_to_freeze() here.
+	 *
+	 * Reason:
+	 * Fundamentally, we just don't need it, because freezing condition
+	 * doesn't come into effect until we release the pm_mutex lock,
+	 * since the freezer always works with pm_mutex held.
+	 *
+	 * More importantly, in the case of hibernation,
+	 * unlock_system_sleep() gets called in snapshot_read() and
+	 * snapshot_write() when the freezing condition is still in effect.
+	 * Which means, if we use try_to_freeze() here, it would make them
+	 * enter the refrigerator, thus causing hibernation to lockup.
+	 */
+	current->flags &= ~PF_FREEZER_SKIP;
+	mutex_unlock(&pm_mutex);
+}
+EXPORT_SYMBOL_GPL(unlock_system_sleep);
+
 /* Routines for PM-transition notifications */
 
 static BLOCKING_NOTIFIER_HEAD(pm_chain_head);
-- 
2.15.1

Re: [PATCH 1/2] PM / sleep: Make lock/unlock_system_sleep() available to kernel modules

[Rafael J. Wysocki]

On Fri, Jan 5, 2018 at 6:19 PM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> Since pm_mutex is not exported using lock/unlock_system_sleep() from
> inside a kernel module causes a "pm_mutex undefined" linker error.
> Hence move lock/unlock_system_sleep() into kernel/power/main.c and
> export these.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> ---
>  include/linux/suspend.h | 28 ++--------------------------
>  kernel/power/main.c     | 29 +++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index d60b0f5c38d5..cc22a24516d6 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -443,32 +443,8 @@ extern bool pm_save_wakeup_count(unsigned int count);
>  extern void pm_wakep_autosleep_enabled(bool set);
>  extern void pm_print_active_wakeup_sources(void);
>
> -static inline void lock_system_sleep(void)
> -{
> -       current->flags |= PF_FREEZER_SKIP;
> -       mutex_lock(&pm_mutex);
> -}
> -
> -static inline void unlock_system_sleep(void)
> -{
> -       /*
> -        * Don't use freezer_count() because we don't want the call to
> -        * try_to_freeze() here.
> -        *
> -        * Reason:
> -        * Fundamentally, we just don't need it, because freezing condition
> -        * doesn't come into effect until we release the pm_mutex lock,
> -        * since the freezer always works with pm_mutex held.
> -        *
> -        * More importantly, in the case of hibernation,
> -        * unlock_system_sleep() gets called in snapshot_read() and
> -        * snapshot_write() when the freezing condition is still in effect.
> -        * Which means, if we use try_to_freeze() here, it would make them
> -        * enter the refrigerator, thus causing hibernation to lockup.
> -        */
> -       current->flags &= ~PF_FREEZER_SKIP;
> -       mutex_unlock(&pm_mutex);
> -}
> +extern void lock_system_sleep(void);
> +extern void unlock_system_sleep(void);
>
>  #else /* !CONFIG_PM_SLEEP */

Don't you need to add stubs for this case too?

> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 3a2ca9066583..705c2366dafe 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -22,6 +22,35 @@ DEFINE_MUTEX(pm_mutex);
>
>  #ifdef CONFIG_PM_SLEEP
>
> +void lock_system_sleep(void)
> +{
> +       current->flags |= PF_FREEZER_SKIP;
> +       mutex_lock(&pm_mutex);
> +}
> +EXPORT_SYMBOL_GPL(lock_system_sleep);
> +
> +void unlock_system_sleep(void)
> +{
> +       /*
> +        * Don't use freezer_count() because we don't want the call to
> +        * try_to_freeze() here.
> +        *
> +        * Reason:
> +        * Fundamentally, we just don't need it, because freezing condition
> +        * doesn't come into effect until we release the pm_mutex lock,
> +        * since the freezer always works with pm_mutex held.
> +        *
> +        * More importantly, in the case of hibernation,
> +        * unlock_system_sleep() gets called in snapshot_read() and
> +        * snapshot_write() when the freezing condition is still in effect.
> +        * Which means, if we use try_to_freeze() here, it would make them
> +        * enter the refrigerator, thus causing hibernation to lockup.
> +        */
> +       current->flags &= ~PF_FREEZER_SKIP;
> +       mutex_unlock(&pm_mutex);
> +}
> +EXPORT_SYMBOL_GPL(unlock_system_sleep);
> +
>  /* Routines for PM-transition notifications */
>
>  static BLOCKING_NOTIFIER_HEAD(pm_chain_head);
> --

The changes are fine by me, but I really would prefer them to go in
via the PM tree which I guess means that the second patch would need
to go in this way too.

Thanks,
Rafael

Re: [PATCH 1/2] PM / sleep: Make lock/unlock_system_sleep() available to kernel modules

[Bart Van Assche]

On Sat, 2018-01-06 at 00:00 +0100, Rafael J. Wysocki wrote:
> The changes are fine by me, but I really would prefer them to go in
> via the PM tree which I guess means that the second patch would need
> to go in this way too.

Hello Rafael,

If I can get a Tested-by from Woody and an Acked-by from Martin then I can
do that.

Thanks,

Bart.

Re: [PATCH 1/2] PM / sleep: Make lock/unlock_system_sleep() available to kernel modules

[Rafael J. Wysocki]

On Sat, Jan 6, 2018 at 12:32 AM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Sat, 2018-01-06 at 00:00 +0100, Rafael J. Wysocki wrote:
>> The changes are fine by me, but I really would prefer them to go in
>> via the PM tree which I guess means that the second patch would need
>> to go in this way too.
>
> Hello Rafael,
>
> If I can get a Tested-by from Woody and an Acked-by from Martin then I can
> do that.

That would be an ACK from Marting for the second patch.

OK

[PATCH 2/2] Fix a race condition between SPI domain validation and system suspend

[Bart Van Assche]

Avoid that the following warning is reported when suspending a system
that is using the mptspi driver:

WARNING: CPU: 0 PID: 4187 at drivers/scsi/scsi_lib.c:2960 scsi_device_quiesce+0x20/0xb0
EIP: scsi_device_quiesce+0x20/0xb0
Call Trace:
 spi_dv_device+0x65/0x5f0 [scsi_transport_spi]
 mptspi_dv_device+0x4d/0x170 [mptspi]
 mptspi_dv_renegotiate_work+0x49/0xc0 [mptspi]
 process_one_work+0x190/0x2e0
 worker_thread+0x37/0x3f0
 kthread+0xcb/0x100
 ret_from_fork+0x19/0x24

Fixes: 3a0a529971ec ("block, scsi: Make SCSI quiesce and resume work reliably")
Reported-by: Woody Suwalski <terraluna977@gmail.com>
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: linux-pm@vger.kernel.org
---
 drivers/scsi/scsi_transport_spi.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index 10ebb213ddb3..871ea582029e 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -26,6 +26,7 @@
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 #include <scsi/scsi.h>
 #include "scsi_priv.h"
 #include <scsi/scsi_device.h>
@@ -1009,11 +1010,20 @@ spi_dv_device(struct scsi_device *sdev)
 	u8 *buffer;
 	const int len = SPI_MAX_ECHO_BUFFER_SIZE*2;
 
+	/*
+	 * Because this function and the power management code both call
+	 * scsi_device_quiesce(), it is not safe to perform domain validation
+	 * while suspend or resume is in progress. Hence the
+	 * lock/unlock_system_sleep() calls.
+	 */
+	lock_system_sleep();
+
 	if (unlikely(spi_dv_in_progress(starget)))
-		return;
+		goto unlock;
 
 	if (unlikely(scsi_device_get(sdev)))
-		return;
+		goto unlock;
+
 	spi_dv_in_progress(starget) = 1;
 
 	buffer = kzalloc(len, GFP_KERNEL);
@@ -1049,6 +1059,8 @@ spi_dv_device(struct scsi_device *sdev)
  out_put:
 	spi_dv_in_progress(starget) = 0;
 	scsi_device_put(sdev);
+unlock:
+	unlock_system_sleep();
 }
 EXPORT_SYMBOL(spi_dv_device);
 
-- 
2.15.1

Re: [PATCH 2/2] Fix a race condition between SPI domain validation and system suspend

[Martin K. Petersen]

Bart,

> Avoid that the following warning is reported when suspending a system
> that is using the mptspi driver:

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/2] Fix a race condition between SPI domain validation and system suspend

[Rafael J. Wysocki]

On Monday, January 8, 2018 5:02:53 PM CET Martin K. Petersen wrote:
> 
> Bart,
> 
> > Avoid that the following warning is reported when suspending a system
> > that is using the mptspi driver:
> 
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

Thanks!

Let me take both patches to the linux-pm tree then.

Thanks,
Rafael