[PATCH v4 1/2] PM: suspend: Define pm_suspend_target_state

[PATCH v4 1/2] PM: suspend: Define pm_suspend_target_state

[Kai-Heng Feng]

Define pm_suspend_target_state so it can still be used when
CONFIG_SUSPEND is not set.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v4:
 - Move pm_suspend_target_state to CONFIG_SUSPEND block.

v3:
 - New patch to resolve undefined pm_suspend_target_state.

 drivers/base/power/wakeup.c | 5 -----
 include/linux/suspend.h     | 4 +++-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 7cc0c0cf8eaa..a917219feea6 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -19,11 +19,6 @@
 
 #include "power.h"
 
-#ifndef CONFIG_SUSPEND
-suspend_state_t pm_suspend_target_state;
-#define pm_suspend_target_state	(PM_SUSPEND_ON)
-#endif
-
 #define list_for_each_entry_rcu_locked(pos, head, member) \
 	list_for_each_entry_rcu(pos, head, member, \
 		srcu_read_lock_held(&wakeup_srcu))
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index d0d4598a7b3f..474ecfbbaa62 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -202,6 +202,7 @@ struct platform_s2idle_ops {
 };
 
 #ifdef CONFIG_SUSPEND
+extern suspend_state_t pm_suspend_target_state;
 extern suspend_state_t mem_sleep_current;
 extern suspend_state_t mem_sleep_default;
 
@@ -337,6 +338,8 @@ extern bool sync_on_suspend_enabled;
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
 
+#define pm_suspend_target_state	(PM_SUSPEND_ON)
+
 static inline void pm_suspend_clear_flags(void) {}
 static inline void pm_set_suspend_via_firmware(void) {}
 static inline void pm_set_resume_via_firmware(void) {}
@@ -503,7 +506,6 @@ extern void pm_report_max_hw_sleep(u64 t);
 
 /* drivers/base/power/wakeup.c */
 extern bool events_check_enabled;
-extern suspend_state_t pm_suspend_target_state;
 
 extern bool pm_wakeup_pending(void);
 extern void pm_system_wakeup(void);
-- 
2.34.1

[PATCH v4 2/2] ata: libata: Defer rescan on suspended device

[Kai-Heng Feng]

During system resume, if an EH is schduled after ATA host is resumed
(i.e. ATA_PFLAG_PM_PENDING cleared), but before the disk device is
fully resumed, the device_lock hold by scsi_rescan_device() is never
released so the dpm_resume() of the disk is blocked forerver.

That's because scsi_attach_vpd() is expecting the disk device is in
operational state, as it doesn't work on suspended device.

To avoid such deadlock, defer rescan if the disk is still suspended so
the resume process of the disk device can proceed. At the end of the
resume process, use the complete() callback to schedule the rescan task.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v4: 
 - No change.

v3:
 - New patch to resolve undefined pm_suspend_target_state.

v2:
 - Schedule rescan task at the end of system resume phase.
 - Wording.

 drivers/ata/libata-core.c | 11 +++++++++++
 drivers/ata/libata-eh.c   | 11 +++++++++--
 include/linux/libata.h    |  1 +
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8bf612bdd61a..bdd244bdb8a2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5093,6 +5093,16 @@ static int ata_port_pm_poweroff(struct device *dev)
 	return 0;
 }
 
+static void ata_port_pm_complete(struct device *dev)
+{
+	struct ata_port *ap = to_ata_port(dev);
+
+	if (ap->pflags & ATA_PFLAG_DEFER_RESCAN)
+		schedule_work(&(ap->scsi_rescan_task));
+
+	ap->pflags &= ~ATA_PFLAG_DEFER_RESCAN;
+}
+
 static const unsigned int ata_port_resume_ehi = ATA_EHI_NO_AUTOPSY
 						| ATA_EHI_QUIET;
 
@@ -5158,6 +5168,7 @@ static const struct dev_pm_ops ata_port_pm_ops = {
 	.thaw = ata_port_pm_resume,
 	.poweroff = ata_port_pm_poweroff,
 	.restore = ata_port_pm_resume,
+	.complete = ata_port_pm_complete,
 
 	.runtime_suspend = ata_port_runtime_suspend,
 	.runtime_resume = ata_port_runtime_resume,
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a6c901811802..0881b590fb7e 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -15,6 +15,7 @@
 #include <linux/blkdev.h>
 #include <linux/export.h>
 #include <linux/pci.h>
+#include <linux/suspend.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_eh.h>
@@ -2983,8 +2984,14 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 			 */
 			ehc->i.flags |= ATA_EHI_SETMODE;
 
-			/* schedule the scsi_rescan_device() here */
-			schedule_work(&(ap->scsi_rescan_task));
+			/* Schedule the scsi_rescan_device() here.
+			 * Defer the rescan if it's in process of
+			 * suspending or resuming.
+			 */
+			if (pm_suspend_target_state != PM_SUSPEND_ON)
+				ap->pflags |= ATA_PFLAG_DEFER_RESCAN;
+			else
+				schedule_work(&(ap->scsi_rescan_task));
 		} else if (dev->class == ATA_DEV_UNKNOWN &&
 			   ehc->tries[dev->devno] &&
 			   ata_class_enabled(ehc->classes[dev->devno])) {
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 311cd93377c7..1696c9ebd168 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -189,6 +189,7 @@ enum {
 	ATA_PFLAG_UNLOADING	= (1 << 9), /* driver is being unloaded */
 	ATA_PFLAG_UNLOADED	= (1 << 10), /* driver is unloaded */
 
+	ATA_PFLAG_DEFER_RESCAN	= (1 << 16), /* peform deferred rescan on system resume */
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
 	ATA_PFLAG_INIT_GTM_VALID = (1 << 19), /* initial gtm data valid */
-- 
2.34.1

Re: [PATCH v4 2/2] ata: libata: Defer rescan on suspended device

[Damien Le Moal]

On 2023/05/03 0:04, Kai-Heng Feng wrote:
> During system resume, if an EH is schduled after ATA host is resumed
> (i.e. ATA_PFLAG_PM_PENDING cleared), but before the disk device is
> fully resumed, the device_lock hold by scsi_rescan_device() is never
> released so the dpm_resume() of the disk is blocked forerver.
> 
> That's because scsi_attach_vpd() is expecting the disk device is in
> operational state, as it doesn't work on suspended device.
> 
> To avoid such deadlock, defer rescan if the disk is still suspended so
> the resume process of the disk device can proceed. At the end of the
> resume process, use the complete() callback to schedule the rescan task.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v4: 
>  - No change.
> 
> v3:
>  - New patch to resolve undefined pm_suspend_target_state.
> 
> v2:
>  - Schedule rescan task at the end of system resume phase.
>  - Wording.
> 
>  drivers/ata/libata-core.c | 11 +++++++++++
>  drivers/ata/libata-eh.c   | 11 +++++++++--
>  include/linux/libata.h    |  1 +
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 8bf612bdd61a..bdd244bdb8a2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5093,6 +5093,16 @@ static int ata_port_pm_poweroff(struct device *dev)
>  	return 0;
>  }
>  
> +static void ata_port_pm_complete(struct device *dev)
> +{
> +	struct ata_port *ap = to_ata_port(dev);
> +
> +	if (ap->pflags & ATA_PFLAG_DEFER_RESCAN)
> +		schedule_work(&(ap->scsi_rescan_task));
> +
> +	ap->pflags &= ~ATA_PFLAG_DEFER_RESCAN;

Is this called with the port lock held ? Otherwise, there is a race with
ata_eh_revalidate_and_attach() and we may end up never actually revalidating the
drive. At the very least, I think that ATA_PFLAG_DEFER_RESCAN needs to be
cleared before calling schedule_work().

> +}
> +
>  static const unsigned int ata_port_resume_ehi = ATA_EHI_NO_AUTOPSY
>  						| ATA_EHI_QUIET;
>  
> @@ -5158,6 +5168,7 @@ static const struct dev_pm_ops ata_port_pm_ops = {
>  	.thaw = ata_port_pm_resume,
>  	.poweroff = ata_port_pm_poweroff,
>  	.restore = ata_port_pm_resume,
> +	.complete = ata_port_pm_complete,
>  
>  	.runtime_suspend = ata_port_runtime_suspend,
>  	.runtime_resume = ata_port_runtime_resume,
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index a6c901811802..0881b590fb7e 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -15,6 +15,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/export.h>
>  #include <linux/pci.h>
> +#include <linux/suspend.h>
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_eh.h>
> @@ -2983,8 +2984,14 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  			 */
>  			ehc->i.flags |= ATA_EHI_SETMODE;
>  
> -			/* schedule the scsi_rescan_device() here */
> -			schedule_work(&(ap->scsi_rescan_task));
> +			/* Schedule the scsi_rescan_device() here.

Code style: please start multi-line comment with a line starting with "/*"
without text after it.

> +			 * Defer the rescan if it's in process of
> +			 * suspending or resuming.
> +			 */
> +			if (pm_suspend_target_state != PM_SUSPEND_ON)

Why ? Shouldn't this be "pm_suspend_target_state == PM_SUSPEND_ON" ? Because if
the device is already resumed, why would we need to defer the rescan ?

> +				ap->pflags |= ATA_PFLAG_DEFER_RESCAN;
> +			else
> +				schedule_work(&(ap->scsi_rescan_task));
>  		} else if (dev->class == ATA_DEV_UNKNOWN &&
>  			   ehc->tries[dev->devno] &&
>  			   ata_class_enabled(ehc->classes[dev->devno])) {
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 311cd93377c7..1696c9ebd168 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -189,6 +189,7 @@ enum {
>  	ATA_PFLAG_UNLOADING	= (1 << 9), /* driver is being unloaded */
>  	ATA_PFLAG_UNLOADED	= (1 << 10), /* driver is unloaded */
>  
> +	ATA_PFLAG_DEFER_RESCAN	= (1 << 16), /* peform deferred rescan on system resume */

Do we really need a new flag ? Can't we use ATA_PFLAG_PM_PENDING correctly ?
From the rather sparse commit message description, it sounds like this flag is
being cleared too early. Not sure though. Need to dig further into this.

>  	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
>  	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
>  	ATA_PFLAG_INIT_GTM_VALID = (1 << 19), /* initial gtm data valid */

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 2/2] ata: libata: Defer rescan on suspended device

[Kai-Heng Feng]

On Sun, May 7, 2023 at 5:23 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> On 2023/05/03 0:04, Kai-Heng Feng wrote:
> > During system resume, if an EH is schduled after ATA host is resumed
> > (i.e. ATA_PFLAG_PM_PENDING cleared), but before the disk device is
> > fully resumed, the device_lock hold by scsi_rescan_device() is never
> > released so the dpm_resume() of the disk is blocked forerver.
> >
> > That's because scsi_attach_vpd() is expecting the disk device is in
> > operational state, as it doesn't work on suspended device.
> >
> > To avoid such deadlock, defer rescan if the disk is still suspended so
> > the resume process of the disk device can proceed. At the end of the
> > resume process, use the complete() callback to schedule the rescan task.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v4:
> >  - No change.
> >
> > v3:
> >  - New patch to resolve undefined pm_suspend_target_state.
> >
> > v2:
> >  - Schedule rescan task at the end of system resume phase.
> >  - Wording.
> >
> >  drivers/ata/libata-core.c | 11 +++++++++++
> >  drivers/ata/libata-eh.c   | 11 +++++++++--
> >  include/linux/libata.h    |  1 +
> >  3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 8bf612bdd61a..bdd244bdb8a2 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5093,6 +5093,16 @@ static int ata_port_pm_poweroff(struct device *dev)
> >       return 0;
> >  }
> >
> > +static void ata_port_pm_complete(struct device *dev)
> > +{
> > +     struct ata_port *ap = to_ata_port(dev);
> > +
> > +     if (ap->pflags & ATA_PFLAG_DEFER_RESCAN)
> > +             schedule_work(&(ap->scsi_rescan_task));
> > +
> > +     ap->pflags &= ~ATA_PFLAG_DEFER_RESCAN;
>
> Is this called with the port lock held ? Otherwise, there is a race with
> ata_eh_revalidate_and_attach() and we may end up never actually revalidating the
> drive. At the very least, I think that ATA_PFLAG_DEFER_RESCAN needs to be
> cleared before calling schedule_work().

OK. Maybe all of these are unnecessary if the rescanning can wait for
disk device to be resumed.

>
> > +}
> > +
> >  static const unsigned int ata_port_resume_ehi = ATA_EHI_NO_AUTOPSY
> >                                               | ATA_EHI_QUIET;
> >
> > @@ -5158,6 +5168,7 @@ static const struct dev_pm_ops ata_port_pm_ops = {
> >       .thaw = ata_port_pm_resume,
> >       .poweroff = ata_port_pm_poweroff,
> >       .restore = ata_port_pm_resume,
> > +     .complete = ata_port_pm_complete,
> >
> >       .runtime_suspend = ata_port_runtime_suspend,
> >       .runtime_resume = ata_port_runtime_resume,
> > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> > index a6c901811802..0881b590fb7e 100644
> > --- a/drivers/ata/libata-eh.c
> > +++ b/drivers/ata/libata-eh.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/blkdev.h>
> >  #include <linux/export.h>
> >  #include <linux/pci.h>
> > +#include <linux/suspend.h>
> >  #include <scsi/scsi.h>
> >  #include <scsi/scsi_host.h>
> >  #include <scsi/scsi_eh.h>
> > @@ -2983,8 +2984,14 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
> >                        */
> >                       ehc->i.flags |= ATA_EHI_SETMODE;
> >
> > -                     /* schedule the scsi_rescan_device() here */
> > -                     schedule_work(&(ap->scsi_rescan_task));
> > +                     /* Schedule the scsi_rescan_device() here.
>
> Code style: please start multi-line comment with a line starting with "/*"
> without text after it.

OK. Will do.

>
> > +                      * Defer the rescan if it's in process of
> > +                      * suspending or resuming.
> > +                      */
> > +                     if (pm_suspend_target_state != PM_SUSPEND_ON)
>
> Why ? Shouldn't this be "pm_suspend_target_state == PM_SUSPEND_ON" ? Because if
> the device is already resumed, why would we need to defer the rescan ?

"pm_suspend_target_state != PM_SUSPEND_ON" means the system is not
"ON" state. For this particular issue, it means the system is still in
resume process.

>
> > +                             ap->pflags |= ATA_PFLAG_DEFER_RESCAN;
> > +                     else
> > +                             schedule_work(&(ap->scsi_rescan_task));
> >               } else if (dev->class == ATA_DEV_UNKNOWN &&
> >                          ehc->tries[dev->devno] &&
> >                          ata_class_enabled(ehc->classes[dev->devno])) {
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 311cd93377c7..1696c9ebd168 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -189,6 +189,7 @@ enum {
> >       ATA_PFLAG_UNLOADING     = (1 << 9), /* driver is being unloaded */
> >       ATA_PFLAG_UNLOADED      = (1 << 10), /* driver is unloaded */
> >
> > +     ATA_PFLAG_DEFER_RESCAN  = (1 << 16), /* peform deferred rescan on system resume */
>
> Do we really need a new flag ? Can't we use ATA_PFLAG_PM_PENDING correctly ?
> From the rather sparse commit message description, it sounds like this flag is
> being cleared too early. Not sure though. Need to dig further into this.

Defer clearing ATA_PFLAG_PM_PENDING doesn't seem to be trivial.
I'll send a new version which IMO is a lot simpler.

Kai-Heng

>
> >       ATA_PFLAG_SUSPENDED     = (1 << 17), /* port is suspended (power) */
> >       ATA_PFLAG_PM_PENDING    = (1 << 18), /* PM operation pending */
> >       ATA_PFLAG_INIT_GTM_VALID = (1 << 19), /* initial gtm data valid */
>
> --
> Damien Le Moal
> Western Digital Research
>

Re: [PATCH v4 1/2] PM: suspend: Define pm_suspend_target_state

[Rafael J. Wysocki]

On Tue, May 2, 2023 at 5:05 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> Define pm_suspend_target_state so it can still be used when
> CONFIG_SUSPEND is not set.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v4:
>  - Move pm_suspend_target_state to CONFIG_SUSPEND block.
>
> v3:
>  - New patch to resolve undefined pm_suspend_target_state.
>
>  drivers/base/power/wakeup.c | 5 -----
>  include/linux/suspend.h     | 4 +++-
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 7cc0c0cf8eaa..a917219feea6 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -19,11 +19,6 @@
>
>  #include "power.h"
>
> -#ifndef CONFIG_SUSPEND
> -suspend_state_t pm_suspend_target_state;
> -#define pm_suspend_target_state        (PM_SUSPEND_ON)
> -#endif
> -
>  #define list_for_each_entry_rcu_locked(pos, head, member) \
>         list_for_each_entry_rcu(pos, head, member, \
>                 srcu_read_lock_held(&wakeup_srcu))
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index d0d4598a7b3f..474ecfbbaa62 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -202,6 +202,7 @@ struct platform_s2idle_ops {
>  };
>
>  #ifdef CONFIG_SUSPEND
> +extern suspend_state_t pm_suspend_target_state;
>  extern suspend_state_t mem_sleep_current;
>  extern suspend_state_t mem_sleep_default;
>
> @@ -337,6 +338,8 @@ extern bool sync_on_suspend_enabled;
>  #else /* !CONFIG_SUSPEND */
>  #define suspend_valid_only_mem NULL
>
> +#define pm_suspend_target_state        (PM_SUSPEND_ON)
> +
>  static inline void pm_suspend_clear_flags(void) {}
>  static inline void pm_set_suspend_via_firmware(void) {}
>  static inline void pm_set_resume_via_firmware(void) {}
> @@ -503,7 +506,6 @@ extern void pm_report_max_hw_sleep(u64 t);
>
>  /* drivers/base/power/wakeup.c */
>  extern bool events_check_enabled;
> -extern suspend_state_t pm_suspend_target_state;
>
>  extern bool pm_wakeup_pending(void);
>  extern void pm_system_wakeup(void);
> --

Applied as 6.5 material under an edited subject and with some edits in
the changelog.

Thanks!