[PATCH 00/15] Add runtime PM support for libsas

[PATCH 00/15] Add runtime PM support for libsas

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

Right now hisi_sas driver has already supported runtime PM, and it works
well on base functions. But for some exception situations, there are some
issues related to libsas layer:
- Remove a directly attached disk when sas host is suspended, a hang will
occur in the resume process, patch 1~2 solve the issue;
- Insert a new disk (for expander) during suspended, and the disk is not
revalidated when resuming sas host, patch 4~7 solve the issue;
- SMP IOs from libsas may be sending when sas host is suspended, so resume
sas host when sending SMP IOs in patch 9;
- New phyup may occur during the process of resuming controller, then work
of DISCE_DISCOVER_DOMAIN of a new phy and work PORTE_BYTES_DMAED of suspended
phy are blocked by each other, so defer works of new phys during suspend
in patch 10~12;
- Work PORTE_BROADCAST_RCVD and PORTE_BYTES_DMAED are in the same 
workqueue, but it is possible that they are blocked by each other, 
so keep sas host active until finished some work in patch 14.

And patch 3 which is related to scsi/block PM is from Alan Stern 
(https://lore.kernel.org/linux-scsi/20210714161027.GC380727@rowland.harvard.edu/)

Alan Stern (1):
  scsi/block PM: Always set request queue runtime active in
    blk_post_runtime_resume()

John Garry (2):
  libsas: Don't always drain event workqueue for HA resume
  Revert "scsi: hisi_sas: Filter out new PHY up events during suspend"

Xiang Chen (12):
  scsi: libsas: Add spin_lock/unlock() to protect asd_sas_port->phy_list
  scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list
  scsi: mvsas: Add spin_lock/unlock() to protect asd_sas_port->phy_list
  scsi: libsas: Send event PORTE_BROADCAST_RCVD for valid ports
  scsi: hisi_sas: Add more prink for runtime suspend/resume
  scsi: libsas: Resume sas host before sending SMP IOs
  scsi: libsas: Add a flag SAS_HA_RESUMING of sas_ha
  scsi: libsas: Refactor out sas_queue_deferred_work()
  scsi: libsas: Defer works of new phys during suspend
  scsi: hisi_sas: Keep controller active between ISR of phyup and the
    event being processed
  scsi: libsas: Keep sas host active until finished some work
  scsi: hisi_sas: Use autosuspend for SAS controller

 block/blk-pm.c                         | 22 +++-----
 drivers/scsi/hisi_sas/hisi_sas.h       |  1 +
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 39 +++++++++----
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 25 +++++++--
 drivers/scsi/libsas/sas_event.c        | 77 +++++++++++++++++++++-----
 drivers/scsi/libsas/sas_expander.c     |  3 +
 drivers/scsi/libsas/sas_init.c         | 49 +++++++++++++++-
 drivers/scsi/libsas/sas_internal.h     |  2 +
 drivers/scsi/mvsas/mv_sas.c            |  5 ++
 drivers/scsi/scsi_pm.c                 |  2 +-
 include/linux/blk-pm.h                 |  2 +-
 include/scsi/libsas.h                  |  2 +
 12 files changed, 181 insertions(+), 48 deletions(-)

-- 
2.33.0

[PATCH 01/15] libsas: Don't always drain event workqueue for HA resume

[chenxiang]

From: John Garry <john.garry@huawei.com>

For the hisi_sas driver, if a directly attached disk is removed during
suspend, a hang will occur in the resume process:

The background is that in commit 16fd4a7c5917 ("scsi: hisi_sas: Add device
link between SCSI devices and hisi_hba"), it is ensured that the HBA
device cannot be runtime suspended when any SCSI device associated is
active.

Other drivers which use libsas don't worry about this as none support
runtime suspend.

The mentioned hang occurs when an disk is removed during suspend. In the
removal process - from PHYE_RESUME_TIMEOUT event processing - we call into
scsi_remove_device(), which is being processed in the HA event workqueue.
Here we wait for all suppliers of the SCSI device to resume, which
includes the HBA device (from the above commit). However the HBA device
cannot resume, as it is waiting for the PHYE_RESUME_TIMEOUT to be processed
(from calling sas_resume_ha() -> sas_drain_work()). This is the deadlock.

There does not appear to be any need for the sas_drain_work() to be called
at all in sas_resume_ha() as it is not syncing against anything, so allow
LLDDs to avoid this by providing a variant of sas_resume_ha() which does
"sync", i.e. doesn't drain the event workqueue.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 10 +++++++++-
 drivers/scsi/libsas/sas_init.c         | 17 +++++++++++++++--
 include/scsi/libsas.h                  |  1 +
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 9ae47d1d9897..5a83373ac0b1 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -4942,7 +4942,15 @@ static int _resume_v3_hw(struct device *device)
 		return rc;
 	}
 	phys_init_v3_hw(hisi_hba);
-	sas_resume_ha(sha);
+
+	/*
+	 * If a directly-attached disk is removed during suspend, a deadlock
+	 * may occur, as the PHYE_RESUME_TIMEOUT processing will require the
+	 * hisi_hba->device to be active, which can only happen when resume
+	 * completes. So don't wait for the HA event workqueue to drain upon
+	 * resume.
+	 */
+	sas_resume_ha_no_sync(sha);
 	clear_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags);
 
 	return 0;
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index b640e09af6a4..43509d139241 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -387,7 +387,7 @@ static int phys_suspended(struct sas_ha_struct *ha)
 	return rc;
 }
 
-void sas_resume_ha(struct sas_ha_struct *ha)
+static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
 {
 	const unsigned long tmo = msecs_to_jiffies(25000);
 	int i;
@@ -417,10 +417,23 @@ void sas_resume_ha(struct sas_ha_struct *ha)
 	 * flush out disks that did not return
 	 */
 	scsi_unblock_requests(ha->core.shost);
-	sas_drain_work(ha);
+	if (drain)
+		sas_drain_work(ha);
+}
+
+void sas_resume_ha(struct sas_ha_struct *ha)
+{
+	_sas_resume_ha(ha, true);
 }
 EXPORT_SYMBOL(sas_resume_ha);
 
+/* A no-sync variant, which does not call sas_drain_ha(). */
+void sas_resume_ha_no_sync(struct sas_ha_struct *ha)
+{
+	_sas_resume_ha(ha, false);
+}
+EXPORT_SYMBOL(sas_resume_ha_no_sync);
+
 void sas_suspend_ha(struct sas_ha_struct *ha)
 {
 	int i;
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 79e4903bd414..a795a2d9e5b1 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -660,6 +660,7 @@ extern int sas_register_ha(struct sas_ha_struct *);
 extern int sas_unregister_ha(struct sas_ha_struct *);
 extern void sas_prep_resume_ha(struct sas_ha_struct *sas_ha);
 extern void sas_resume_ha(struct sas_ha_struct *sas_ha);
+extern void sas_resume_ha_no_sync(struct sas_ha_struct *sas_ha);
 extern void sas_suspend_ha(struct sas_ha_struct *sas_ha);
 
 int sas_set_phy_speed(struct sas_phy *phy, struct sas_phy_linkrates *rates);
-- 
2.33.0

[PATCH 02/15] Revert "scsi: hisi_sas: Filter out new PHY up events during suspend"

[chenxiang]

From: John Garry <john.garry@huawei.com>

This reverts commit b14a37e011d829404c29a5ae17849d7efb034893.

In that commit, we had to filter out phy-up events during suspend, as it
work cause a deadlock between processing the phyup event and the resume
HA function try to drain the HA event workqueue to complete the resume
process.

Now that we no longer try to drain the HA event queue during the HA
resume processor, the deadlock would not occur, so remove the special
handling for it.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index f206c433de32..305d6282845a 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -619,12 +619,6 @@ static void hisi_sas_bytes_dmaed(struct hisi_hba *hisi_hba, int phy_no,
 	if (!phy->phy_attached)
 		return;
 
-	if (test_bit(HISI_SAS_PM_BIT, &hisi_hba->flags) &&
-	    !sas_phy->suspended) {
-		dev_warn(hisi_hba->dev, "phy%d during suspend filtered out\n", phy_no);
-		return;
-	}
-
 	sas_notify_phy_event(sas_phy, PHYE_OOB_DONE, gfp_flags);
 
 	if (sas_phy->phy) {
-- 
2.33.0

[PATCH 03/15] scsi/block PM: Always set request queue runtime active in blk_post_runtime_resume()

[chenxiang]

From: Alan Stern <stern@rowland.harvard.edu>

John Garry reported a deadlock that occurs when trying to access a
runtime-suspended SATA device.  For obscure reasons, the rescan
procedure causes the link to be hard-reset, which disconnects the
device.

The rescan tries to carry out a runtime resume when accessing the
device.  scsi_rescan_device() holds the SCSI device lock and won't
release it until it can put commands onto the device's block queue.
This can't happen until the queue is successfully runtime-resumed or
the device is unregistered.  But the runtime resume fails because the
device is disconnected, and __scsi_remove_device() can't do the
unregistration because it can't get the device lock.

The best way to resolve this deadlock appears to be to allow the block
queue to start running again even after an unsuccessful runtime
resume.  The idea is that the driver or the SCSI error handler will
need to be able to use the queue to resolve the runtime resume
failure.

This patch removes the err argument to blk_post_runtime_resume() and
makes the routine act as though the resume was successful always.
This fixes the deadlock.

Reported-and-tested-by: John Garry <john.garry@huawei.com>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Fixes: e27829dc92e5 ("scsi: serialize ->rescan against ->remove")
---
 block/blk-pm.c         | 22 +++++++---------------
 drivers/scsi/scsi_pm.c |  2 +-
 include/linux/blk-pm.h |  2 +-
 3 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/block/blk-pm.c b/block/blk-pm.c
index 17bd020268d4..2dad62cc1572 100644
--- a/block/blk-pm.c
+++ b/block/blk-pm.c
@@ -163,27 +163,19 @@ EXPORT_SYMBOL(blk_pre_runtime_resume);
 /**
  * blk_post_runtime_resume - Post runtime resume processing
  * @q: the queue of the device
- * @err: return value of the device's runtime_resume function
  *
  * Description:
- *    Update the queue's runtime status according to the return value of the
- *    device's runtime_resume function. If the resume was successful, call
- *    blk_set_runtime_active() to do the real work of restarting the queue.
+ *    For historical reasons, this routine merely calls blk_set_runtime_active()
+ *    to do the real work of restarting the queue.  It does this regardless of
+ *    whether the device's runtime-resume succeeded; even if it failed the
+ *    driver or error handler will need to communicate with the device.
  *
  *    This function should be called near the end of the device's
  *    runtime_resume callback.
  */
-void blk_post_runtime_resume(struct request_queue *q, int err)
+void blk_post_runtime_resume(struct request_queue *q)
 {
-	if (!q->dev)
-		return;
-	if (!err) {
-		blk_set_runtime_active(q);
-	} else {
-		spin_lock_irq(&q->queue_lock);
-		q->rpm_status = RPM_SUSPENDED;
-		spin_unlock_irq(&q->queue_lock);
-	}
+	blk_set_runtime_active(q);
 }
 EXPORT_SYMBOL(blk_post_runtime_resume);
 
@@ -201,7 +193,7 @@ EXPORT_SYMBOL(blk_post_runtime_resume);
  * runtime PM status and re-enable peeking requests from the queue. It
  * should be called before first request is added to the queue.
  *
- * This function is also called by blk_post_runtime_resume() for successful
+ * This function is also called by blk_post_runtime_resume() for
  * runtime resumes.  It does everything necessary to restart the queue.
  */
 void blk_set_runtime_active(struct request_queue *q)
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index b5a858c29488..f06ca9d2a597 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -181,7 +181,7 @@ static int sdev_runtime_resume(struct device *dev)
 	blk_pre_runtime_resume(sdev->request_queue);
 	if (pm && pm->runtime_resume)
 		err = pm->runtime_resume(dev);
-	blk_post_runtime_resume(sdev->request_queue, err);
+	blk_post_runtime_resume(sdev->request_queue);
 
 	return err;
 }
diff --git a/include/linux/blk-pm.h b/include/linux/blk-pm.h
index b80c65aba249..2580e05a8ab6 100644
--- a/include/linux/blk-pm.h
+++ b/include/linux/blk-pm.h
@@ -14,7 +14,7 @@ extern void blk_pm_runtime_init(struct request_queue *q, struct device *dev);
 extern int blk_pre_runtime_suspend(struct request_queue *q);
 extern void blk_post_runtime_suspend(struct request_queue *q, int err);
 extern void blk_pre_runtime_resume(struct request_queue *q);
-extern void blk_post_runtime_resume(struct request_queue *q, int err);
+extern void blk_post_runtime_resume(struct request_queue *q);
 extern void blk_set_runtime_active(struct request_queue *q);
 #else
 static inline void blk_pm_runtime_init(struct request_queue *q,
-- 
2.33.0

[PATCH 04/15] scsi: libsas: Add spin_lock/unlock() to protect asd_sas_port->phy_list

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

Most places that use asd_sas_port->phy_list in libsas are protected by
spinlock asd_sas_port->phy_list_lock, but there are still some places
which lack of it. So add it in those places.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_event.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index f703115e7a25..af605620ea13 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -104,11 +104,15 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
 		if (!test_and_clear_bit(ev, &d->pending))
 			continue;
 
-		if (list_empty(&port->phy_list))
+		spin_lock(&port->phy_list_lock);
+		if (list_empty(&port->phy_list)) {
+			spin_unlock(&port->phy_list_lock);
 			continue;
+		}
 
 		sas_phy = container_of(port->phy_list.next, struct asd_sas_phy,
 				port_phy_el);
+		spin_unlock(&port->phy_list_lock);
 		sas_notify_port_event(sas_phy,
 				PORTE_BROADCAST_RCVD, GFP_KERNEL);
 	}
-- 
2.33.0

[PATCH 05/15] scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

Most places that use asd_sas_port->phy_list are protected by spinlock
asd_sas_port->phy_list_lock, but there are some places which lack of it
in hisi_sas driver, so add it in function hisi_sas_refresh_port_id() when
accessing asd_sas_port->phy_list. But it has a risk that list mutates while
dropping the lock at the same time in function
hisi_sas_send_ata_reset_each_phy(), so read asd_sas_port->phy_mask
instead of accessing asd_sas_port->phy_list to avoid the risk.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Acked-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 305d6282845a..44c888e0afd7 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1427,11 +1427,13 @@ static void hisi_sas_refresh_port_id(struct hisi_hba *hisi_hba)
 		sas_port = device->port;
 		port = to_hisi_sas_port(sas_port);
 
+		spin_lock(&sas_port->phy_list_lock);
 		list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el)
 			if (state & BIT(sas_phy->id)) {
 				phy = sas_phy->lldd_phy;
 				break;
 			}
+		spin_unlock(&sas_port->phy_list_lock);
 
 		if (phy) {
 			port->id = phy->port_id;
@@ -1508,22 +1510,25 @@ static void hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba,
 	struct ata_link *link;
 	u8 fis[20] = {0};
 	u32 state;
+	int i;
 
 	state = hisi_hba->hw->get_phys_state(hisi_hba);
-	list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el) {
+	for (i = 0; i < hisi_hba->n_phy; i++) {
 		if (!(state & BIT(sas_phy->id)))
 			continue;
+		if (!(sas_port->phy_mask & BIT(i)))
+			continue;
 
 		ata_for_each_link(link, ap, EDGE) {
 			int pmp = sata_srst_pmp(link);
 
-			tmf_task.phy_id = sas_phy->id;
+			tmf_task.phy_id = i;
 			hisi_sas_fill_ata_reset_cmd(link->device, 1, pmp, fis);
 			rc = hisi_sas_exec_internal_tmf_task(device, fis, s,
 							     &tmf_task);
 			if (rc != TMF_RESP_FUNC_COMPLETE) {
 				dev_err(dev, "phy%d ata reset failed rc=%d\n",
-					sas_phy->id, rc);
+					i, rc);
 				break;
 			}
 		}
-- 
2.33.0

[PATCH 06/15] scsi: mvsas: Add spin_lock/unlock() to protect asd_sas_port->phy_list

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

It lacks of spinlock phy_list_lock when using asd_sas_port->phy_list in
mvsas driver, so add spin_lock/unlock in those places.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
---
 drivers/scsi/mvsas/mv_sas.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 31d1ea5a5dd2..1e52bc7febfa 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -67,8 +67,10 @@ static struct mvs_info *mvs_find_dev_mvi(struct domain_device *dev)
 
 	while (sha->sas_port[i]) {
 		if (sha->sas_port[i] == dev->port) {
+			spin_lock(&sha->sas_port[i]->phy_list_lock);
 			phy =  container_of(sha->sas_port[i]->phy_list.next,
 				struct asd_sas_phy, port_phy_el);
+			spin_unlock(&sha->sas_port[i]->phy_list_lock);
 			j = 0;
 			while (sha->sas_phy[j]) {
 				if (sha->sas_phy[j] == phy)
@@ -96,6 +98,8 @@ static int mvs_find_dev_phyno(struct domain_device *dev, int *phyno)
 	while (sha->sas_port[i]) {
 		if (sha->sas_port[i] == dev->port) {
 			struct asd_sas_phy *phy;
+
+			spin_lock(&sha->sas_port[i]->phy_list_lock);
 			list_for_each_entry(phy,
 				&sha->sas_port[i]->phy_list, port_phy_el) {
 				j = 0;
@@ -109,6 +113,7 @@ static int mvs_find_dev_phyno(struct domain_device *dev, int *phyno)
 				num++;
 				n++;
 			}
+			spin_unlock(&sha->sas_port[i]->phy_list_lock);
 			break;
 		}
 		i++;
-- 
2.33.0

[PATCH 07/15] scsi: libsas: Send event PORTE_BROADCAST_RCVD for valid ports

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

If inserting a new disk for expander, the disk will not be revalidated
as the topology is not re-scanned during the resume process. So send event
PORTE_BROADCAST_RCVD to identify some new inserted disks for expander.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_init.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 43509d139241..974c4a305ece 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -387,6 +387,30 @@ static int phys_suspended(struct sas_ha_struct *ha)
 	return rc;
 }
 
+static void sas_resume_insert_broadcast_ha(struct sas_ha_struct *ha)
+{
+	int i;
+
+	for (i = 0; i < ha->num_phys; i++) {
+		struct asd_sas_port *port = ha->sas_port[i];
+		struct domain_device *dev = port->port_dev;
+
+		if (dev && dev_is_expander(dev->dev_type)) {
+			struct asd_sas_phy *first_phy;
+
+			spin_lock(&port->phy_list_lock);
+			first_phy = list_first_entry_or_null(
+				&port->phy_list, struct asd_sas_phy,
+				port_phy_el);
+			spin_unlock(&port->phy_list_lock);
+
+			if (first_phy)
+				sas_notify_port_event(first_phy,
+					PORTE_BROADCAST_RCVD, GFP_KERNEL);
+		}
+	}
+}
+
 static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
 {
 	const unsigned long tmo = msecs_to_jiffies(25000);
@@ -419,6 +443,11 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
 	scsi_unblock_requests(ha->core.shost);
 	if (drain)
 		sas_drain_work(ha);
+
+	/* send event PORTE_BROADCAST_RCVD to identify some new inserted
+	 * disks for expander
+	 */
+	sas_resume_insert_broadcast_ha(ha);
 }
 
 void sas_resume_ha(struct sas_ha_struct *ha)
-- 
2.33.0

[PATCH 08/15] scsi: hisi_sas: Add more prink for runtime suspend/resume

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

Add some prink at the beginning and end of suspend/resume.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 5a83373ac0b1..4e9dfdb69757 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -4896,6 +4896,8 @@ static int _suspend_v3_hw(struct device *device)
 	if (test_and_set_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags))
 		return -1;
 
+	dev_warn(dev, "entering suspend state\n");
+
 	scsi_block_requests(shost);
 	set_bit(HISI_SAS_REJECT_CMD_BIT, &hisi_hba->flags);
 	flush_workqueue(hisi_hba->wq);
@@ -4911,11 +4913,11 @@ static int _suspend_v3_hw(struct device *device)
 
 	hisi_sas_init_mem(hisi_hba);
 
-	dev_warn(dev, "entering suspend state\n");
-
 	hisi_sas_release_tasks(hisi_hba);
 
 	sas_suspend_ha(sha);
+
+	dev_warn(dev, "end of suspending controller\n");
 	return 0;
 }
 
@@ -4953,6 +4955,8 @@ static int _resume_v3_hw(struct device *device)
 	sas_resume_ha_no_sync(sha);
 	clear_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags);
 
+	dev_warn(dev, "end of resuming controller\n");
+
 	return 0;
 }
 
-- 
2.33.0

[PATCH 09/15] scsi: libsas: Resume sas host before sending SMP IOs

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

Need to resume sas host before sending SMP IOs to ensure that
SMP IOs are sent sucessfully.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 3 +++
 drivers/scsi/libsas/sas_internal.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index c2150a818423..6abce9dfc17b 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -58,7 +58,9 @@ static int smp_execute_task_sg(struct domain_device *dev,
 	struct sas_task *task = NULL;
 	struct sas_internal *i =
 		to_sas_internal(dev->port->ha->core.shost->transportt);
+	struct sas_ha_struct *ha = dev->port->ha;
 
+	pm_runtime_get_sync(ha->dev);
 	mutex_lock(&dev->ex_dev.cmd_mutex);
 	for (retry = 0; retry < 3; retry++) {
 		if (test_bit(SAS_DEV_GONE, &dev->state)) {
@@ -131,6 +133,7 @@ static int smp_execute_task_sg(struct domain_device *dev,
 		}
 	}
 	mutex_unlock(&dev->ex_dev.cmd_mutex);
+	pm_runtime_put_sync(ha->dev);
 
 	BUG_ON(retry == 3 && task != NULL);
 	sas_free_task(task);
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index d7a1fb5c10c6..ad9764a976c3 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -14,6 +14,7 @@
 #include <scsi/scsi_transport_sas.h>
 #include <scsi/libsas.h>
 #include <scsi/sas_ata.h>
+#include <linux/pm_runtime.h>
 
 #ifdef pr_fmt
 #undef pr_fmt
-- 
2.33.0

[PATCH 10/15] scsi: libsas: Add a flag SAS_HA_RESUMING of sas_ha

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

Add a flag SAS_HA_RESUMING and use it to indicate the process of resuming
sas_ha.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_init.c | 2 ++
 include/scsi/libsas.h          | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 974c4a305ece..069e40fc8411 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -362,6 +362,7 @@ void sas_prep_resume_ha(struct sas_ha_struct *ha)
 	int i;
 
 	set_bit(SAS_HA_REGISTERED, &ha->state);
+	set_bit(SAS_HA_RESUMING, &ha->state);
 
 	/* clear out any stale link events/data from the suspension path */
 	for (i = 0; i < ha->num_phys; i++) {
@@ -443,6 +444,7 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
 	scsi_unblock_requests(ha->core.shost);
 	if (drain)
 		sas_drain_work(ha);
+	clear_bit(SAS_HA_RESUMING, &ha->state);
 
 	/* send event PORTE_BROADCAST_RCVD to identify some new inserted
 	 * disks for expander
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index a795a2d9e5b1..698f2032807b 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -356,6 +356,7 @@ enum sas_ha_state {
 	SAS_HA_DRAINING,
 	SAS_HA_ATA_EH_ACTIVE,
 	SAS_HA_FROZEN,
+	SAS_HA_RESUMING,
 };
 
 struct sas_ha_struct {
-- 
2.33.0

[PATCH 11/15] scsi: libsas: Refactor out sas_queue_deferred_work()

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

In the 2rd part of function __sas_drain_work, it queues defer work. And it
will be used in other places, so refactor out sas_queue_deferred_work().

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_event.c    | 25 ++++++++++++++-----------
 drivers/scsi/libsas/sas_internal.h |  1 +
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index af605620ea13..01e544ca518a 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -41,12 +41,23 @@ static int sas_queue_event(int event, struct sas_work *work,
 	return rc;
 }
 
-
-void __sas_drain_work(struct sas_ha_struct *ha)
+void sas_queue_deferred_work(struct sas_ha_struct *ha)
 {
 	struct sas_work *sw, *_sw;
 	int ret;
 
+	spin_lock_irq(&ha->lock);
+	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
+		list_del_init(&sw->drain_node);
+		ret = sas_queue_work(ha, sw);
+		if (ret != 1)
+			sas_free_event(to_asd_sas_event(&sw->work));
+	}
+	spin_unlock_irq(&ha->lock);
+}
+
+void __sas_drain_work(struct sas_ha_struct *ha)
+{
 	set_bit(SAS_HA_DRAINING, &ha->state);
 	/* flush submitters */
 	spin_lock_irq(&ha->lock);
@@ -55,16 +66,8 @@ void __sas_drain_work(struct sas_ha_struct *ha)
 	drain_workqueue(ha->event_q);
 	drain_workqueue(ha->disco_q);
 
-	spin_lock_irq(&ha->lock);
 	clear_bit(SAS_HA_DRAINING, &ha->state);
-	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
-		list_del_init(&sw->drain_node);
-		ret = sas_queue_work(ha, sw);
-		if (ret != 1)
-			sas_free_event(to_asd_sas_event(&sw->work));
-
-	}
-	spin_unlock_irq(&ha->lock);
+	sas_queue_deferred_work(ha);
 }
 
 int sas_drain_work(struct sas_ha_struct *ha)
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index ad9764a976c3..acd515c01861 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -57,6 +57,7 @@ void sas_unregister_ports(struct sas_ha_struct *sas_ha);
 
 void sas_disable_revalidation(struct sas_ha_struct *ha);
 void sas_enable_revalidation(struct sas_ha_struct *ha);
+void sas_queue_deferred_work(struct sas_ha_struct *ha);
 void __sas_drain_work(struct sas_ha_struct *ha);
 
 void sas_deform_port(struct asd_sas_phy *phy, int gone);
-- 
2.33.0

[PATCH 12/15] scsi: libsas: Defer works of new phys during suspend

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

During the process of event PORT_BYTES_DMAED, it queues work
DISCE_DISCOVER_DOMAIN and then flush workqueue ha->disco_q.
If new phyup occurs during resming SAS controller, the work
PORTE_BYTES_DMAED of new phys occurs before suspended phys', then work
DISCE_DISCOVER_DOMAIN of new phy requires the active of SAS controller
(It requires to resume SAS controller by function scsi_sysfs_add_sdev()
and some other functions such as fucntion add_device_link()).
But the active of SAS controller requires the complete of work
PORTE_BYTES_DMAED of suspended phys while it is blocked by new phy's work
on ha->event_q. So there is a deadlock and it is released only after
resume timeout.

To solve the issue, defer works of new phys during suspend and queue those
defer works after SAS controller becomes active.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_event.c | 24 ++++++++++++++++++++++++
 drivers/scsi/libsas/sas_init.c  |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 01e544ca518a..626ef96b9348 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -139,6 +139,24 @@ static void sas_phy_event_worker(struct work_struct *work)
 	sas_free_event(ev);
 }
 
+/* defer works of new phys during suspend */
+static bool sas_defer_event(struct asd_sas_phy *phy, struct asd_sas_event *ev)
+{
+	struct sas_ha_struct *ha = phy->ha;
+	unsigned long flags;
+	bool deferred = false;
+
+	spin_lock_irqsave(&ha->lock, flags);
+	if (test_bit(SAS_HA_RESUMING, &ha->state) && !phy->suspended) {
+		struct sas_work *sw = &ev->work;
+
+		list_add_tail(&sw->drain_node, &ha->defer_q);
+		deferred = true;
+	}
+	spin_unlock_irqrestore(&ha->lock, flags);
+	return deferred;
+}
+
 int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
 			  gfp_t gfp_flags)
 {
@@ -154,6 +172,9 @@ int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
 
 	INIT_SAS_EVENT(ev, sas_port_event_worker, phy, event);
 
+	if (sas_defer_event(phy, ev))
+		return 0;
+
 	ret = sas_queue_event(event, &ev->work, ha);
 	if (ret != 1)
 		sas_free_event(ev);
@@ -177,6 +198,9 @@ int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event,
 
 	INIT_SAS_EVENT(ev, sas_phy_event_worker, phy, event);
 
+	if (sas_defer_event(phy, ev))
+		return 0;
+
 	ret = sas_queue_event(event, &ev->work, ha);
 	if (ret != 1)
 		sas_free_event(ev);
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 069e40fc8411..dc35f0f8eae3 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -446,6 +446,7 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
 		sas_drain_work(ha);
 	clear_bit(SAS_HA_RESUMING, &ha->state);
 
+	sas_queue_deferred_work(ha);
 	/* send event PORTE_BROADCAST_RCVD to identify some new inserted
 	 * disks for expander
 	 */
-- 
2.33.0

[PATCH 13/15] scsi: hisi_sas: Keep controller active between ISR of phyup and the event being processed

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

It is possible that controller may be suspended between ISR of phyup
and the event being processed, then it can't ensure controller is
active when processing the phyup event which will cause issues.
To avoid the issue, add pm_runtime_get_noresume() in ISR of phyup
and pm_runtime_put_sync() in the work handler exit of a new event
HISI_PHYE_PHY_UP_PM which is called in v3 hw as runtime PM is
only supported for v3 hw.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Acked-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas.h       |  1 +
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 22 ++++++++++++++++++++--
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  5 ++++-
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 2213a91923a5..8269703b978d 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -154,6 +154,7 @@ enum hisi_sas_bit_err_type {
 enum hisi_sas_phy_event {
 	HISI_PHYE_PHY_UP   = 0U,
 	HISI_PHYE_LINK_RESET,
+	HISI_PHYE_PHY_UP_PM,
 	HISI_PHYES_NUM,
 };
 
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 44c888e0afd7..9255790a8568 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -854,10 +854,11 @@ int hisi_sas_scan_finished(struct Scsi_Host *shost, unsigned long time)
 }
 EXPORT_SYMBOL_GPL(hisi_sas_scan_finished);
 
-static void hisi_sas_phyup_work(struct work_struct *work)
+static void hisi_sas_phyup_work_common(struct work_struct *work,
+		enum hisi_sas_phy_event event)
 {
 	struct hisi_sas_phy *phy =
-		container_of(work, typeof(*phy), works[HISI_PHYE_PHY_UP]);
+		container_of(work, typeof(*phy), works[event]);
 	struct hisi_hba *hisi_hba = phy->hisi_hba;
 	struct asd_sas_phy *sas_phy = &phy->sas_phy;
 	int phy_no = sas_phy->id;
@@ -868,6 +869,11 @@ static void hisi_sas_phyup_work(struct work_struct *work)
 	hisi_sas_bytes_dmaed(hisi_hba, phy_no, GFP_KERNEL);
 }
 
+static void hisi_sas_phyup_work(struct work_struct *work)
+{
+	hisi_sas_phyup_work_common(work, HISI_PHYE_PHY_UP);
+}
+
 static void hisi_sas_linkreset_work(struct work_struct *work)
 {
 	struct hisi_sas_phy *phy =
@@ -877,9 +883,21 @@ static void hisi_sas_linkreset_work(struct work_struct *work)
 	hisi_sas_control_phy(sas_phy, PHY_FUNC_LINK_RESET, NULL);
 }
 
+static void hisi_sas_phyup_pm_work(struct work_struct *work)
+{
+	struct hisi_sas_phy *phy =
+		container_of(work, typeof(*phy), works[HISI_PHYE_PHY_UP_PM]);
+	struct hisi_hba *hisi_hba = phy->hisi_hba;
+	struct device *dev = hisi_hba->dev;
+
+	hisi_sas_phyup_work_common(work, HISI_PHYE_PHY_UP_PM);
+	pm_runtime_put_sync(dev);
+}
+
 static const work_func_t hisi_sas_phye_fns[HISI_PHYES_NUM] = {
 	[HISI_PHYE_PHY_UP] = hisi_sas_phyup_work,
 	[HISI_PHYE_LINK_RESET] = hisi_sas_linkreset_work,
+	[HISI_PHYE_PHY_UP_PM] = hisi_sas_phyup_pm_work,
 };
 
 bool hisi_sas_notify_phy_event(struct hisi_sas_phy *phy,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 4e9dfdb69757..a5695ae8b73b 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -1561,7 +1561,10 @@ static irqreturn_t phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
 
 	phy->port_id = port_id;
 	phy->phy_attached = 1;
-	hisi_sas_notify_phy_event(phy, HISI_PHYE_PHY_UP);
+
+	/* Call pm_runtime_put_sync() with pairs in hisi_sas_phyup_pm_work() */
+	pm_runtime_get_noresume(dev);
+	hisi_sas_notify_phy_event(phy, HISI_PHYE_PHY_UP_PM);
 	res = IRQ_HANDLED;
 end:
 	if (phy->reset_completion)
-- 
2.33.0

[PATCH 14/15] scsi: libsas: Keep sas host active until finished some work

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

For those works from event queue, if executing them such as
PORTE_BROADCAST_RCVD when sas host is suspended, it will resume sas host
firstly as SMP IOs are sent in the process. So phyup will occur and it
will call work PORTE_BYTES_DMAED. But the original work (such as
PORTE_BROADCAST_RCVD) and the work PORTE_BYTES_DMAED are in the same
singlethread workqueue, so the complete of original work waits for
the complete of work PORTE_BYTES_DMAED while the complete of work
PORTE_BYTES_DMAED wait for the complete of original and it is blocked.

So call pm_runtime_get_noresume() to keep sas host active until
finished those works.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_event.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 626ef96b9348..3613b9b315bc 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -50,8 +50,10 @@ void sas_queue_deferred_work(struct sas_ha_struct *ha)
 	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
 		list_del_init(&sw->drain_node);
 		ret = sas_queue_work(ha, sw);
-		if (ret != 1)
+		if (ret != 1) {
+			pm_runtime_put(ha->dev);
 			sas_free_event(to_asd_sas_event(&sw->work));
+		}
 	}
 	spin_unlock_irq(&ha->lock);
 }
@@ -126,16 +128,22 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
 static void sas_port_event_worker(struct work_struct *work)
 {
 	struct asd_sas_event *ev = to_asd_sas_event(work);
+	struct asd_sas_phy *phy = ev->phy;
+	struct sas_ha_struct *ha = phy->ha;
 
 	sas_port_event_fns[ev->event](work);
+	pm_runtime_put(ha->dev);
 	sas_free_event(ev);
 }
 
 static void sas_phy_event_worker(struct work_struct *work)
 {
 	struct asd_sas_event *ev = to_asd_sas_event(work);
+	struct asd_sas_phy *phy = ev->phy;
+	struct sas_ha_struct *ha = phy->ha;
 
 	sas_phy_event_fns[ev->event](work);
+	pm_runtime_put(ha->dev);
 	sas_free_event(ev);
 }
 
@@ -170,14 +178,19 @@ int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
 	if (!ev)
 		return -ENOMEM;
 
+	/* Call pm_runtime_put() with pairs in sas_port_event_worker() */
+	pm_runtime_get_noresume(ha->dev);
+
 	INIT_SAS_EVENT(ev, sas_port_event_worker, phy, event);
 
 	if (sas_defer_event(phy, ev))
 		return 0;
 
 	ret = sas_queue_event(event, &ev->work, ha);
-	if (ret != 1)
+	if (ret != 1) {
+		pm_runtime_put(ha->dev);
 		sas_free_event(ev);
+	}
 
 	return ret;
 }
@@ -196,14 +209,19 @@ int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event,
 	if (!ev)
 		return -ENOMEM;
 
+	/* Call pm_runtime_put() with pairs in sas_phy_event_worker() */
+	pm_runtime_get_noresume(ha->dev);
+
 	INIT_SAS_EVENT(ev, sas_phy_event_worker, phy, event);
 
 	if (sas_defer_event(phy, ev))
 		return 0;
 
 	ret = sas_queue_event(event, &ev->work, ha);
-	if (ret != 1)
+	if (ret != 1) {
+		pm_runtime_put(ha->dev);
 		sas_free_event(ev);
+	}
 
 	return ret;
 }
-- 
2.33.0

[PATCH 15/15] scsi: hisi_sas: Use autosuspend for SAS controller

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

For some scenarios, it may send many IOs in a short time, then SAS controller
will enter suspend and resume frequently which is invalid.
To avoid it, use autosuspend mode for SAS controller and set default
autosuspend delay time to 5s.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index a5695ae8b73b..d96fe636e984 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -4777,6 +4777,8 @@ hisi_sas_v3_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	scsi_scan_host(shost);
 
+	pm_runtime_set_autosuspend_delay(dev, 5000);
+	pm_runtime_use_autosuspend(dev);
 	/*
 	 * For the situation that there are ATA disks connected with SAS
 	 * controller, it additionally creates ata_port which will affect the
-- 
2.33.0

Re: [PATCH 01/15] libsas: Don't always drain event workqueue for HA resume

[Bart Van Assche]

On 11/16/21 18:44, chenxiang wrote:
> [ ... ]

Where is the cover letter of this patch series? Please always
include a cover letter with a patch series.

Thanks,

Bart.

Re: [PATCH 03/15] scsi/block PM: Always set request queue runtime active in blk_post_runtime_resume()

[Bart Van Assche]

On 11/16/21 6:44 PM, chenxiang wrote:
> From: Alan Stern <stern@rowland.harvard.edu>
> 
> John Garry reported a deadlock that occurs when trying to access a
> runtime-suspended SATA device.  For obscure reasons, the rescan
> procedure causes the link to be hard-reset, which disconnects the
> device.
> 
> The rescan tries to carry out a runtime resume when accessing the
> device.  scsi_rescan_device() holds the SCSI device lock and won't
> release it until it can put commands onto the device's block queue.
> This can't happen until the queue is successfully runtime-resumed or
> the device is unregistered.  But the runtime resume fails because the
> device is disconnected, and __scsi_remove_device() can't do the
> unregistration because it can't get the device lock.
> 
> The best way to resolve this deadlock appears to be to allow the block
> queue to start running again even after an unsuccessful runtime
> resume.  The idea is that the driver or the SCSI error handler will
> need to be able to use the queue to resolve the runtime resume
> failure.
> 
> This patch removes the err argument to blk_post_runtime_resume() and
> makes the routine act as though the resume was successful always.
> This fixes the deadlock.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 01/15] libsas: Don't always drain event workqueue for HA resume

[Bart Van Assche]

On 11/16/21 8:14 PM, Bart Van Assche wrote:
> On 11/16/21 18:44, chenxiang wrote:
>> [ ... ]
> 
> Where is the cover letter of this patch series? Please always
> include a cover letter with a patch series.

Oops, I was too fast. Patches 01/15..15/15 arrived in my mailbox before the cover
letter. I just received the cover letter.

Thanks,

Bart.

Re: [PATCH 00/15] Add runtime PM support for libsas

[Martin K. Petersen]

> Right now hisi_sas driver has already supported runtime PM, and it
> works well on base functions.

The commit messages for this series need work.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 00/15] Add runtime PM support for libsas

[chenxiang (M)]

在 2021/11/19 12:04, Martin K. Petersen 写道:
>> Right now hisi_sas driver has already supported runtime PM, and it
>> works well on base functions.
> The commit messages for this series need work.
>
Ok, thanks.

Re: [PATCH 02/15] Revert "scsi: hisi_sas: Filter out new PHY up events during suspend"

[John Garry]

On 17/11/2021 02:44, chenxiang wrote:
> From: John Garry<john.garry@huawei.com>
> 
> This reverts commit b14a37e011d829404c29a5ae17849d7efb034893.
> 
> In that commit, we had to filter out phy-up events during suspend, as it
> work cause a deadlock between processing the phyup event and the resume
> HA function try to drain the HA event workqueue to complete the resume
> process.
> 
> Now that we no longer try to drain the HA event queue during the HA
> resume processor, the deadlock would not occur, so remove the special
> handling for it.
> 
> Signed-off-by: John Garry<john.garry@huawei.com>

This is missing your Signed-off-by

Thanks,
John

Re: [PATCH 08/15] scsi: hisi_sas: Add more prink for runtime suspend/resume

[John Garry]

On 17/11/2021 02:45, chenxiang wrote:
> From: Xiang Chen<chenxiang66@hisilicon.com>
> 
> Add some prink at the beginning and end of suspend/resume.
>

Same as subject, it should be /s/prink/logs or prints/. I assume.


> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
> Signed-off-by: John Garry<john.garry@huawei.com>

Re: [PATCH 08/15] scsi: hisi_sas: Add more prink for runtime suspend/resume

[John Garry]

On 17/11/2021 02:45, chenxiang wrote:
> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
> Signed-off-by: John Garry<john.garry@huawei.com>

And please change this to Acked-by for my tag

Thanks,
John

Re: [PATCH 13/15] scsi: hisi_sas: Keep controller active between ISR of phyup and the event being processed

[John Garry]

On 17/11/2021 02:45, chenxiang wrote:
> From: Xiang Chen<chenxiang66@hisilicon.com>
> 
> It is possible that controller may be suspended between ISR of phyup
> and the event being processed, then it can't ensure controller is
> active when processing the phyup event which will cause issues.
> To avoid the issue, add pm_runtime_get_noresume() in ISR of phyup
> and pm_runtime_put_sync() in the work handler exit of a new event
> HISI_PHYE_PHY_UP_PM which is called in v3 hw as runtime PM is
> only supported for v3 hw.

Please consider this rewrite:

It is possible that the controller may become suspended between 
processing a phyup interrupt and the event being processed by libsas. As 
such, we can't ensure that the controller is active when processing the 
phyup event - this may cause the phyup event to be lost or other issues.
To avoid any possible issues, add pm_runtime_get_noresume() in the phyup 
interrupt handler and pm_runtime_put_sync() in the work handler exit to 
ensure that we stay always active. Since we only want 
pm_runtime_get_noresume() called for v3 hw, signal this will a new 
event, HISI_PHYE_PHY_UP_PM.

Thanks,
John

Re: [PATCH 15/15] scsi: hisi_sas: Use autosuspend for SAS controller

[John Garry]

Please consider this rewrite:

On 17/11/2021 02:45, chenxiang wrote:

scsi: hisi_sas: Use autosuspend for the host controller

> From: Xiang Chen<chenxiang66@hisilicon.com>
> 
> For some scenarios, it may send many IOs in a short time, then SAS controller
> will enter suspend and resume frequently which is invalid.
> To avoid it, use autosuspend mode for SAS controller and set default
> autosuspend delay time to 5s.

The controller may frequently enter and exit suspend for each IO which 
we need to deal with. This is inefficient and may cause too much 
suspend+resume activity for the controller.

To avoid this, use a default 5s autosuspend for the controller to stop 
frequently suspending and resuming. This value may still be still 
modified via sysfs.

> 
> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
> Signed-off-by: John Garry<john.garry@huawei.com>

Acked-by please

Thanks,
John

Re: [PATCH 07/15] scsi: libsas: Send event PORTE_BROADCAST_RCVD for valid ports

[John Garry]

Please consider these points:

About "Send event PORTE_BROADCAST_RCVD for valid ports":

Maybe say "Insert PORTE_BROADCAST_RCVD event for resuming host", as 
"valid ports" is a little vague. And the point is that we can to rescan 
when we resume.

On 17/11/2021 02:45, chenxiang wrote:
> From: Xiang Chen<chenxiang66@hisilicon.com>
> 
> If inserting a new disk for expander, the disk will not be revalidated
> as the topology is not re-scanned during the resume process. So send event
> PORTE_BROADCAST_RCVD to identify some new inserted disks for expander.

If a new disk is inserted through an expander when the host was 
suspended, they will not necessarily be detected as the topology is not 
re-scanned during resume.

To detect possible changes in topology during suspension, insert a 
PORTE_BROADCAST_RCVD event per port when resuming to trigger a revalidation.

> 
> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
> Reviewed-by: John Garry<john.garry@huawei.com>

Re: [PATCH 09/15] scsi: libsas: Resume sas host before sending SMP IOs

[John Garry]

Please consider these points:

About "scsi: libsas: Resume sas host before sending SMP IOs", just have 
"Resume host while sending SMP IOs"

On 17/11/2021 02:45, chenxiang wrote:
> From: Xiang Chen<chenxiang66@hisilicon.com>
> 
> Need to resume sas host before sending SMP IOs to ensure that
> SMP IOs are sent sucessfully.

successfully

 >

When sending SMP IOs to the host we need to ensure that that host is not 
suspended and may handle the commands. This is a better approach than 
relying on the host to resume itself to handle such commands. So use 
pm_runtime_get_sync() and pm_runtime_get_sync() calls for the host when 
executing SMP tasks.

> 
> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
> Reviewed-by: John Garry<john.garry@huawei.com>

Re: [PATCH 10/15] scsi: libsas: Add a flag SAS_HA_RESUMING of sas_ha

[John Garry]

On 17/11/2021 02:45, chenxiang wrote:

I'd just have "scsi: libsas: Add flag SAS_HA_RESUMING"

> From: Xiang Chen<chenxiang66@hisilicon.com>
> 
> Add a flag SAS_HA_RESUMING and use it to indicate the process of resuming
> sas_ha.

Add a flag SAS_HA_RESUMING and use it to indicate the state of resuming 
the host controller.

> 
> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
> Reviewed-by: John Garry<john.garry@huawei.com>

Re: [PATCH 11/15] scsi: libsas: Refactor out sas_queue_deferred_work()

[John Garry]

Please consider these:

On 17/11/2021 02:45, chenxiang wrote:

I'd have "scsi: libsas: Refactor sas_queue_deferred_work()"

> From: Xiang Chen <chenxiang66@hisilicon.com>
> 
> In the 2rd part of function __sas_drain_work, it queues defer work. And it
> will be used in other places, so refactor out sas_queue_deferred_work().

The second part of the function __sas_drain_work() queues the deferred 
work. This functionality would be duplicated in other places, so factor 
out into sas_queue_deferred_work().

Thanks,
John

> 
> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/libsas/sas_event.c    | 25 ++++++++++++++-----------
>   drivers/scsi/libsas/sas_internal.h |  1 +
>   2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
> index af605620ea13..01e544ca518a 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -41,12 +41,23 @@ static int sas_queue_event(int event, struct sas_work *work,
>   	return rc;
>   }
>   
> -
> -void __sas_drain_work(struct sas_ha_struct *ha)
> +void sas_queue_deferred_work(struct sas_ha_struct *ha)
>   {
>   	struct sas_work *sw, *_sw;
>   	int ret;
>   
> +	spin_lock_irq(&ha->lock);
> +	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
> +		list_del_init(&sw->drain_node);
> +		ret = sas_queue_work(ha, sw);
> +		if (ret != 1)
> +			sas_free_event(to_asd_sas_event(&sw->work));
> +	}
> +	spin_unlock_irq(&ha->lock);
> +}
> +
> +void __sas_drain_work(struct sas_ha_struct *ha)
> +{
>   	set_bit(SAS_HA_DRAINING, &ha->state);
>   	/* flush submitters */
>   	spin_lock_irq(&ha->lock);
> @@ -55,16 +66,8 @@ void __sas_drain_work(struct sas_ha_struct *ha)
>   	drain_workqueue(ha->event_q);
>   	drain_workqueue(ha->disco_q);
>   
> -	spin_lock_irq(&ha->lock);
>   	clear_bit(SAS_HA_DRAINING, &ha->state);
> -	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
> -		list_del_init(&sw->drain_node);
> -		ret = sas_queue_work(ha, sw);
> -		if (ret != 1)
> -			sas_free_event(to_asd_sas_event(&sw->work));
> -
> -	}
> -	spin_unlock_irq(&ha->lock);
> +	sas_queue_deferred_work(ha);
>   }
>   
>   int sas_drain_work(struct sas_ha_struct *ha)
> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
> index ad9764a976c3..acd515c01861 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -57,6 +57,7 @@ void sas_unregister_ports(struct sas_ha_struct *sas_ha);
>   
>   void sas_disable_revalidation(struct sas_ha_struct *ha);
>   void sas_enable_revalidation(struct sas_ha_struct *ha);
> +void sas_queue_deferred_work(struct sas_ha_struct *ha);
>   void __sas_drain_work(struct sas_ha_struct *ha);
>   
>   void sas_deform_port(struct asd_sas_phy *phy, int gone);
> 

Re: [PATCH 00/15] Add runtime PM support for libsas

[John Garry]

On 17/11/2021 02:44, chenxiang wrote:
> From: Xiang Chen<chenxiang66@hisilicon.com>
> 

Please consider this rewrite:

Currently the HiSilicon SAS controller v3 hw driver supports runtime PM. 
However a number of corner-case bugs have been reported for this 
feature. These include:
a. If a directly-attached disk is removed when the host is suspended a 
system hang may occur during resume. libsas drains events after resuming 
the host. Draining the events causes a deadlock as we need to ensure 
that the host is resumed for some libsas events processing, however the 
resume process will not complete until all events are processed.
b. If a disk is attached to an expander when the host is suspended then 
this new disk will not be detected when active again.
c. The host controller may not resume from suspension when sending SMP IOs.
d. If a phy comes up when resuming the host controller then we may get a 
deadlock from processing of events DISCE_DISCOVER_DOMAIN and 
PORTE_BYTES_DMAED.
e. Similar to d., the work of PORTE_BROADCAST_RCVD and PORTE_BYTES_DMAED 
may deadlock.

This series addresses those issues, briefly described as follows:
a. As far as we can see, this drain is unneeded, so conditionally remove.
b. Just insert broadcast events to revalidate the topology.
c. and e. When processing any events from the LLD, make libsas keep the 
host active until finished processing all work related to that original 
event.
d. Defer phyup event processing in case described.


> Right now hisi_sas driver has already supported runtime PM, and it works
> well on base functions. But for some exception situations, there are some
> issues related to libsas layer:
> - Remove a directly attached disk when sas host is suspended, a hang will
> occur in the resume process, patch 1~2 solve the issue;
> - Insert a new disk (for expander) during suspended, and the disk is not
> revalidated when resuming sas host, patch 4~7 solve the issue;
> - SMP IOs from libsas may be sending when sas host is suspended, so resume
> sas host when sending SMP IOs in patch 9;
> - New phyup may occur during the process of resuming controller, then work
> of DISCE_DISCOVER_DOMAIN of a new phy and work PORTE_BYTES_DMAED of suspended
> phy are blocked by each other, so defer works of new phys during suspend
> in patch 10~12;
> - Work PORTE_BROADCAST_RCVD and PORTE_BYTES_DMAED are in the same
> workqueue, but it is possible that they are blocked by each other,
> so keep sas host active until finished some work in patch 14.
> 
> And patch 3 which is related to scsi/block PM is from Alan Stern
> (https://lore.kernel.org/linux-scsi/20210714161027.GC380727@rowland.harvard.edu/)

Re: [PATCH 14/15] scsi: libsas: Keep sas host active until finished some work

[John Garry]

Please consider som rewrite:

On 17/11/2021 02:45, chenxiang wrote:

"scsi: libsas: Keep sas host active until finished some work" -> "scsi: 
libsas: Keep host active while processing events"

> From: Xiang Chen <chenxiang66@hisilicon.com>
> 
> For those works from event queue, if executing them such as
> PORTE_BROADCAST_RCVD when sas host is suspended, it will resume sas host
> firstly as SMP IOs are sent in the process. So phyup will occur and it
> will call work PORTE_BYTES_DMAED. But the original work (such as
> PORTE_BROADCAST_RCVD) and the work PORTE_BYTES_DMAED are in the same
> singlethread workqueue, so the complete of original work waits for
> the complete of work PORTE_BYTES_DMAED while the complete of work
> PORTE_BYTES_DMAED wait for the complete of original and it is blocked.
> 
> So call pm_runtime_get_noresume() to keep sas host active until
> finished those works.
Processing events such as PORTE_BROADCAST_RCVD may cause dependency 
issues for runtime power management support.

Such a problem would be that handling a PORTE_BROADCAST_RCVD event 
requires that the host is resumed to send SMP commands. However, in 
resuming the host, the phyup events generated from re-enabling the phys 
are processed in the same workqueue as the original PORTE_BROADCAST_RCVD 
event. As such, the host will never finish resuming (as it waits for the 
phyup event processing), and then the PORTE_BROADCAST_RCVD event cannot 
be processed as the SMP commands are blocked, and so we have a deadlock.

Solve this problem by ensuring that libsas keeps the host active until 
completely finished processing phy or port events, such as 
PORTE_BYTES_DMAED. As such, we don't have to worry about resuming the 
host for processing individual SMP commands in this example.

> 
> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/libsas/sas_event.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
> index 626ef96b9348..3613b9b315bc 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -50,8 +50,10 @@ void sas_queue_deferred_work(struct sas_ha_struct *ha)
>   	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
>   		list_del_init(&sw->drain_node);
>   		ret = sas_queue_work(ha, sw);
> -		if (ret != 1)
> +		if (ret != 1) {
> +			pm_runtime_put(ha->dev);
>   			sas_free_event(to_asd_sas_event(&sw->work));
> +		}
>   	}
>   	spin_unlock_irq(&ha->lock);
>   }
> @@ -126,16 +128,22 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
>   static void sas_port_event_worker(struct work_struct *work)
>   {
>   	struct asd_sas_event *ev = to_asd_sas_event(work);
> +	struct asd_sas_phy *phy = ev->phy;
> +	struct sas_ha_struct *ha = phy->ha;
>   
>   	sas_port_event_fns[ev->event](work);
> +	pm_runtime_put(ha->dev);
>   	sas_free_event(ev);
>   }
>   
>   static void sas_phy_event_worker(struct work_struct *work)
>   {
>   	struct asd_sas_event *ev = to_asd_sas_event(work);
> +	struct asd_sas_phy *phy = ev->phy;
> +	struct sas_ha_struct *ha = phy->ha;
>   
>   	sas_phy_event_fns[ev->event](work);
> +	pm_runtime_put(ha->dev);
>   	sas_free_event(ev);
>   }
>   
> @@ -170,14 +178,19 @@ int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
>   	if (!ev)
>   		return -ENOMEM;
>   
> +	/* Call pm_runtime_put() with pairs in sas_port_event_worker() */
> +	pm_runtime_get_noresume(ha->dev);
> +
>   	INIT_SAS_EVENT(ev, sas_port_event_worker, phy, event);
>   
>   	if (sas_defer_event(phy, ev))
>   		return 0;
>   
>   	ret = sas_queue_event(event, &ev->work, ha);
> -	if (ret != 1)
> +	if (ret != 1) {
> +		pm_runtime_put(ha->dev);
>   		sas_free_event(ev);
> +	}
>   
>   	return ret;
>   }
> @@ -196,14 +209,19 @@ int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event,
>   	if (!ev)
>   		return -ENOMEM;
>   
> +	/* Call pm_runtime_put() with pairs in sas_phy_event_worker() */
> +	pm_runtime_get_noresume(ha->dev);
> +
>   	INIT_SAS_EVENT(ev, sas_phy_event_worker, phy, event);
>   
>   	if (sas_defer_event(phy, ev))
>   		return 0;
>   
>   	ret = sas_queue_event(event, &ev->work, ha);
> -	if (ret != 1)
> +	if (ret != 1) {
> +		pm_runtime_put(ha->dev);
>   		sas_free_event(ev);
> +	}
>   
>   	return ret;
>   }
> 

Re: [PATCH 02/15] Revert "scsi: hisi_sas: Filter out new PHY up events during suspend"

[chenxiang (M)]

在 2021/12/13 18:31, John Garry 写道:
> On 17/11/2021 02:44, chenxiang wrote:
>> From: John Garry<john.garry@huawei.com>
>>
>> This reverts commit b14a37e011d829404c29a5ae17849d7efb034893.
>>
>> In that commit, we had to filter out phy-up events during suspend, as it
>> work cause a deadlock between processing the phyup event and the resume
>> HA function try to drain the HA event workqueue to complete the resume
>> process.
>>
>> Now that we no longer try to drain the HA event queue during the HA
>> resume processor, the deadlock would not occur, so remove the special
>> handling for it.
>>
>> Signed-off-by: John Garry<john.garry@huawei.com>
>
> This is missing your Signed-off-by

Ok, i will add it in next version.

>
> Thanks,
> John
>
> .
>

Re: [PATCH 07/15] scsi: libsas: Send event PORTE_BROADCAST_RCVD for valid ports

[chenxiang (M)]

在 2021/12/13 19:02, John Garry 写道:
> Please consider these points:
>
> About "Send event PORTE_BROADCAST_RCVD for valid ports":
>
> Maybe say "Insert PORTE_BROADCAST_RCVD event for resuming host", as 
> "valid ports" is a little vague. And the point is that we can to 
> rescan when we resume.
ok

>
> On 17/11/2021 02:45, chenxiang wrote:
>> From: Xiang Chen<chenxiang66@hisilicon.com>
>>
>> If inserting a new disk for expander, the disk will not be revalidated
>> as the topology is not re-scanned during the resume process. So send 
>> event
>> PORTE_BROADCAST_RCVD to identify some new inserted disks for expander.
>
> If a new disk is inserted through an expander when the host was 
> suspended, they will not necessarily be detected as the topology is 
> not re-scanned during resume.
>
> To detect possible changes in topology during suspension, insert a 
> PORTE_BROADCAST_RCVD event per port when resuming to trigger a 
> revalidation.

Thanks, i will change it in next version.

>
>>
>> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
>> Reviewed-by: John Garry<john.garry@huawei.com>
>
>
> .
>

Re: [PATCH 08/15] scsi: hisi_sas: Add more prink for runtime suspend/resume

[chenxiang (M)]

在 2021/12/13 18:34, John Garry 写道:
> On 17/11/2021 02:45, chenxiang wrote:
>> From: Xiang Chen<chenxiang66@hisilicon.com>
>>
>> Add some prink at the beginning and end of suspend/resume.
>>
>
> Same as subject, it should be /s/prink/logs or prints/. I assume.

ok, i will use logs instead

>
>
>> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
>> Signed-off-by: John Garry<john.garry@huawei.com>
>
>
> .
>

Re: [PATCH 08/15] scsi: hisi_sas: Add more prink for runtime suspend/resume

[chenxiang (M)]

在 2021/12/13 18:35, John Garry 写道:
> On 17/11/2021 02:45, chenxiang wrote:
>> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
>> Signed-off-by: John Garry<john.garry@huawei.com>
>
> And please change this to Acked-by for my tag
>

Ok, thanks, i will change it in next version.


> Thanks,
> John
>
> .
>

Re: [PATCH 10/15] scsi: libsas: Add a flag SAS_HA_RESUMING of sas_ha

[chenxiang (M)]

在 2021/12/13 19:17, John Garry 写道:
> On 17/11/2021 02:45, chenxiang wrote:
>
> I'd just have "scsi: libsas: Add flag SAS_HA_RESUMING"

Ok

>
>> From: Xiang Chen<chenxiang66@hisilicon.com>
>>
>> Add a flag SAS_HA_RESUMING and use it to indicate the process of 
>> resuming
>> sas_ha.
>
> Add a flag SAS_HA_RESUMING and use it to indicate the state of 
> resuming the host controller.
Ok
>
>>
>> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
>> Reviewed-by: John Garry<john.garry@huawei.com>
>
>
> .
>

Re: [PATCH 14/15] scsi: libsas: Keep sas host active until finished some work

[chenxiang (M)]

在 2021/12/14 20:34, John Garry 写道:
> Please consider som rewrite:
>
> On 17/11/2021 02:45, chenxiang wrote:
>
> "scsi: libsas: Keep sas host active until finished some work" -> 
> "scsi: libsas: Keep host active while processing events"
>
>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>
>> For those works from event queue, if executing them such as
>> PORTE_BROADCAST_RCVD when sas host is suspended, it will resume sas host
>> firstly as SMP IOs are sent in the process. So phyup will occur and it
>> will call work PORTE_BYTES_DMAED. But the original work (such as
>> PORTE_BROADCAST_RCVD) and the work PORTE_BYTES_DMAED are in the same
>> singlethread workqueue, so the complete of original work waits for
>> the complete of work PORTE_BYTES_DMAED while the complete of work
>> PORTE_BYTES_DMAED wait for the complete of original and it is blocked.
>>
>> So call pm_runtime_get_noresume() to keep sas host active until
>> finished those works.
> Processing events such as PORTE_BROADCAST_RCVD may cause dependency 
> issues for runtime power management support.
>
> Such a problem would be that handling a PORTE_BROADCAST_RCVD event 
> requires that the host is resumed to send SMP commands. However, in 
> resuming the host, the phyup events generated from re-enabling the 
> phys are processed in the same workqueue as the original 
> PORTE_BROADCAST_RCVD event. As such, the host will never finish 
> resuming (as it waits for the phyup event processing), and then the 
> PORTE_BROADCAST_RCVD event cannot be processed as the SMP commands are 
> blocked, and so we have a deadlock.
>
> Solve this problem by ensuring that libsas keeps the host active until 
> completely finished processing phy or port events, such as 
> PORTE_BYTES_DMAED. As such, we don't have to worry about resuming the 
> host for processing individual SMP commands in this example.
>

ok, i will consider to rewrite them.

>>
>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> Reviewed-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_event.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_event.c 
>> b/drivers/scsi/libsas/sas_event.c
>> index 626ef96b9348..3613b9b315bc 100644
>> --- a/drivers/scsi/libsas/sas_event.c
>> +++ b/drivers/scsi/libsas/sas_event.c
>> @@ -50,8 +50,10 @@ void sas_queue_deferred_work(struct sas_ha_struct 
>> *ha)
>>       list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
>>           list_del_init(&sw->drain_node);
>>           ret = sas_queue_work(ha, sw);
>> -        if (ret != 1)
>> +        if (ret != 1) {
>> +            pm_runtime_put(ha->dev);
>> sas_free_event(to_asd_sas_event(&sw->work));
>> +        }
>>       }
>>       spin_unlock_irq(&ha->lock);
>>   }
>> @@ -126,16 +128,22 @@ void sas_enable_revalidation(struct 
>> sas_ha_struct *ha)
>>   static void sas_port_event_worker(struct work_struct *work)
>>   {
>>       struct asd_sas_event *ev = to_asd_sas_event(work);
>> +    struct asd_sas_phy *phy = ev->phy;
>> +    struct sas_ha_struct *ha = phy->ha;
>>         sas_port_event_fns[ev->event](work);
>> +    pm_runtime_put(ha->dev);
>>       sas_free_event(ev);
>>   }
>>     static void sas_phy_event_worker(struct work_struct *work)
>>   {
>>       struct asd_sas_event *ev = to_asd_sas_event(work);
>> +    struct asd_sas_phy *phy = ev->phy;
>> +    struct sas_ha_struct *ha = phy->ha;
>>         sas_phy_event_fns[ev->event](work);
>> +    pm_runtime_put(ha->dev);
>>       sas_free_event(ev);
>>   }
>>   @@ -170,14 +178,19 @@ int sas_notify_port_event(struct asd_sas_phy 
>> *phy, enum port_event event,
>>       if (!ev)
>>           return -ENOMEM;
>>   +    /* Call pm_runtime_put() with pairs in sas_port_event_worker() */
>> +    pm_runtime_get_noresume(ha->dev);
>> +
>>       INIT_SAS_EVENT(ev, sas_port_event_worker, phy, event);
>>         if (sas_defer_event(phy, ev))
>>           return 0;
>>         ret = sas_queue_event(event, &ev->work, ha);
>> -    if (ret != 1)
>> +    if (ret != 1) {
>> +        pm_runtime_put(ha->dev);
>>           sas_free_event(ev);
>> +    }
>>         return ret;
>>   }
>> @@ -196,14 +209,19 @@ int sas_notify_phy_event(struct asd_sas_phy 
>> *phy, enum phy_event event,
>>       if (!ev)
>>           return -ENOMEM;
>>   +    /* Call pm_runtime_put() with pairs in sas_phy_event_worker() */
>> +    pm_runtime_get_noresume(ha->dev);
>> +
>>       INIT_SAS_EVENT(ev, sas_phy_event_worker, phy, event);
>>         if (sas_defer_event(phy, ev))
>>           return 0;
>>         ret = sas_queue_event(event, &ev->work, ha);
>> -    if (ret != 1)
>> +    if (ret != 1) {
>> +        pm_runtime_put(ha->dev);
>>           sas_free_event(ev);
>> +    }
>>         return ret;
>>   }
>>
>
>
> .
>

Re: [PATCH 15/15] scsi: hisi_sas: Use autosuspend for SAS controller

[chenxiang (M)]

在 2021/12/13 18:50, John Garry 写道:
> Please consider this rewrite:

ok, i will consider to rewrite it in next version

>
> On 17/11/2021 02:45, chenxiang wrote:
>
> scsi: hisi_sas: Use autosuspend for the host controller
>
>> From: Xiang Chen<chenxiang66@hisilicon.com>
>>
>> For some scenarios, it may send many IOs in a short time, then SAS 
>> controller
>> will enter suspend and resume frequently which is invalid.
>> To avoid it, use autosuspend mode for SAS controller and set default
>> autosuspend delay time to 5s.
>
> The controller may frequently enter and exit suspend for each IO which 
> we need to deal with. This is inefficient and may cause too much 
> suspend+resume activity for the controller.
>
> To avoid this, use a default 5s autosuspend for the controller to stop 
> frequently suspending and resuming. This value may still be still 
> modified via sysfs.
>
>>
>> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
>> Signed-off-by: John Garry<john.garry@huawei.com>
>
> Acked-by please

ok

>
> Thanks,
> John
>
> .
>

Re: [PATCH 00/15] Add runtime PM support for libsas

[chenxiang (M)]

Hi John,


在 2021/12/14 19:52, John Garry 写道:
> On 17/11/2021 02:44, chenxiang wrote:
>> From: Xiang Chen<chenxiang66@hisilicon.com>
>>
>
> Please consider this rewrite:
>
> Currently the HiSilicon SAS controller v3 hw driver supports runtime 
> PM. However a number of corner-case bugs have been reported for this 
> feature. These include:
> a. If a directly-attached disk is removed when the host is suspended a 
> system hang may occur during resume. libsas drains events after 
> resuming the host. Draining the events causes a deadlock as we need to 
> ensure that the host is resumed for some libsas events processing, 
> however the resume process will not complete until all events are 
> processed.
> b. If a disk is attached to an expander when the host is suspended 
> then this new disk will not be detected when active again.
> c. The host controller may not resume from suspension when sending SMP 
> IOs.
> d. If a phy comes up when resuming the host controller then we may get 
> a deadlock from processing of events DISCE_DISCOVER_DOMAIN and 
> PORTE_BYTES_DMAED.
> e. Similar to d., the work of PORTE_BROADCAST_RCVD and 
> PORTE_BYTES_DMAED may deadlock.
>
> This series addresses those issues, briefly described as follows:
> a. As far as we can see, this drain is unneeded, so conditionally remove.
> b. Just insert broadcast events to revalidate the topology.
> c. and e. When processing any events from the LLD, make libsas keep 
> the host active until finished processing all work related to that 
> original event.
> d. Defer phyup event processing in case described.

Thanks for your detailed comments.
I will improve those commit message according to your suggestions.

>
>
>> Right now hisi_sas driver has already supported runtime PM, and it works
>> well on base functions. But for some exception situations, there are 
>> some
>> issues related to libsas layer:
>> - Remove a directly attached disk when sas host is suspended, a hang 
>> will
>> occur in the resume process, patch 1~2 solve the issue;
>> - Insert a new disk (for expander) during suspended, and the disk is not
>> revalidated when resuming sas host, patch 4~7 solve the issue;
>> - SMP IOs from libsas may be sending when sas host is suspended, so 
>> resume
>> sas host when sending SMP IOs in patch 9;
>> - New phyup may occur during the process of resuming controller, then 
>> work
>> of DISCE_DISCOVER_DOMAIN of a new phy and work PORTE_BYTES_DMAED of 
>> suspended
>> phy are blocked by each other, so defer works of new phys during suspend
>> in patch 10~12;
>> - Work PORTE_BROADCAST_RCVD and PORTE_BYTES_DMAED are in the same
>> workqueue, but it is possible that they are blocked by each other,
>> so keep sas host active until finished some work in patch 14.
>>
>> And patch 3 which is related to scsi/block PM is from Alan Stern
>> (https://lore.kernel.org/linux-scsi/20210714161027.GC380727@rowland.harvard.edu/) 
>>
>
>
> .
>

Re: [PATCH 13/15] scsi: hisi_sas: Keep controller active between ISR of phyup and the event being processed

[chenxiang (M)]

在 2021/12/13 18:44, John Garry 写道:
> On 17/11/2021 02:45, chenxiang wrote:
>> From: Xiang Chen<chenxiang66@hisilicon.com>
>>
>> It is possible that controller may be suspended between ISR of phyup
>> and the event being processed, then it can't ensure controller is
>> active when processing the phyup event which will cause issues.
>> To avoid the issue, add pm_runtime_get_noresume() in ISR of phyup
>> and pm_runtime_put_sync() in the work handler exit of a new event
>> HISI_PHYE_PHY_UP_PM which is called in v3 hw as runtime PM is
>> only supported for v3 hw.
>
> Please consider this rewrite:
>
> It is possible that the controller may become suspended between 
> processing a phyup interrupt and the event being processed by libsas. 
> As such, we can't ensure that the controller is active when processing 
> the phyup event - this may cause the phyup event to be lost or other 
> issues.
> To avoid any possible issues, add pm_runtime_get_noresume() in the 
> phyup interrupt handler and pm_runtime_put_sync() in the work handler 
> exit to ensure that we stay always active. Since we only want 
> pm_runtime_get_noresume() called for v3 hw, signal this will a new 
> event, HISI_PHYE_PHY_UP_PM.

Ok, i will rewrite it in next version.

>
> Thanks,
> John
>
> .
>

Re: [PATCH 11/15] scsi: libsas: Refactor out sas_queue_deferred_work()

[chenxiang (M)]

在 2021/12/13 19:20, John Garry 写道:
> Please consider these:
>
> On 17/11/2021 02:45, chenxiang wrote:
>
> I'd have "scsi: libsas: Refactor sas_queue_deferred_work()"

Ok, i will change it.

>
>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>
>> In the 2rd part of function __sas_drain_work, it queues defer work. 
>> And it
>> will be used in other places, so refactor out sas_queue_deferred_work().
>
> The second part of the function __sas_drain_work() queues the deferred 
> work. This functionality would be duplicated in other places, so 
> factor out into sas_queue_deferred_work().

I will rewrite them in next verison.

>
> Thanks,
> John
>
>>
>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> Reviewed-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_event.c    | 25 ++++++++++++++-----------
>>   drivers/scsi/libsas/sas_internal.h |  1 +
>>   2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_event.c 
>> b/drivers/scsi/libsas/sas_event.c
>> index af605620ea13..01e544ca518a 100644
>> --- a/drivers/scsi/libsas/sas_event.c
>> +++ b/drivers/scsi/libsas/sas_event.c
>> @@ -41,12 +41,23 @@ static int sas_queue_event(int event, struct 
>> sas_work *work,
>>       return rc;
>>   }
>>   -
>> -void __sas_drain_work(struct sas_ha_struct *ha)
>> +void sas_queue_deferred_work(struct sas_ha_struct *ha)
>>   {
>>       struct sas_work *sw, *_sw;
>>       int ret;
>>   +    spin_lock_irq(&ha->lock);
>> +    list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
>> +        list_del_init(&sw->drain_node);
>> +        ret = sas_queue_work(ha, sw);
>> +        if (ret != 1)
>> +            sas_free_event(to_asd_sas_event(&sw->work));
>> +    }
>> +    spin_unlock_irq(&ha->lock);
>> +}
>> +
>> +void __sas_drain_work(struct sas_ha_struct *ha)
>> +{
>>       set_bit(SAS_HA_DRAINING, &ha->state);
>>       /* flush submitters */
>>       spin_lock_irq(&ha->lock);
>> @@ -55,16 +66,8 @@ void __sas_drain_work(struct sas_ha_struct *ha)
>>       drain_workqueue(ha->event_q);
>>       drain_workqueue(ha->disco_q);
>>   -    spin_lock_irq(&ha->lock);
>>       clear_bit(SAS_HA_DRAINING, &ha->state);
>> -    list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
>> -        list_del_init(&sw->drain_node);
>> -        ret = sas_queue_work(ha, sw);
>> -        if (ret != 1)
>> -            sas_free_event(to_asd_sas_event(&sw->work));
>> -
>> -    }
>> -    spin_unlock_irq(&ha->lock);
>> +    sas_queue_deferred_work(ha);
>>   }
>>     int sas_drain_work(struct sas_ha_struct *ha)
>> diff --git a/drivers/scsi/libsas/sas_internal.h 
>> b/drivers/scsi/libsas/sas_internal.h
>> index ad9764a976c3..acd515c01861 100644
>> --- a/drivers/scsi/libsas/sas_internal.h
>> +++ b/drivers/scsi/libsas/sas_internal.h
>> @@ -57,6 +57,7 @@ void sas_unregister_ports(struct sas_ha_struct 
>> *sas_ha);
>>     void sas_disable_revalidation(struct sas_ha_struct *ha);
>>   void sas_enable_revalidation(struct sas_ha_struct *ha);
>> +void sas_queue_deferred_work(struct sas_ha_struct *ha);
>>   void __sas_drain_work(struct sas_ha_struct *ha);
>>     void sas_deform_port(struct asd_sas_phy *phy, int gone);
>>
>
>
> .
>

Re: [PATCH 09/15] scsi: libsas: Resume sas host before sending SMP IOs

[chenxiang (M)]

在 2021/12/13 19:12, John Garry 写道:
> Please consider these points:
>

> About "scsi: libsas: Resume sas host before sending SMP IOs", just 
> have "Resume host while sending SMP IOs"
>
> On 17/11/2021 02:45, chenxiang wrote:
>> From: Xiang Chen<chenxiang66@hisilicon.com>
>>
>> Need to resume sas host before sending SMP IOs to ensure that
>> SMP IOs are sent sucessfully.
>
> successfully

ok

>
> >
>
> When sending SMP IOs to the host we need to ensure that that host is 
> not suspended and may handle the commands. This is a better approach 
> than relying on the host to resume itself to handle such commands. So 
> use pm_runtime_get_sync() and pm_runtime_get_sync() calls for the host 
> when executing SMP tasks.

Ok, i will rewrite it.

>
>>
>> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com>
>> Reviewed-by: John Garry<john.garry@huawei.com>
>
>
> .
>