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

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

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

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 includes:
a. If a directly-attached disk is removed when the host is suspended, a
system hang may occur during resuming. 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 be resumed 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 it
in patch 1~2;
b. Just insert broadcast events to revalidate the topology in patch 4~7;
c. and e. When processing any events from the LLD, make libsas keep the
host active until finished processing all works related to the original
events in patch 9 and 14;
d. Defer phyup event processing in case described in patch 10~12;


Change Log:
v1 -> v2:
- Rewrite those commit messages according to John Garry's suggestions;
- Add my SOB for patch 2 and change SOB of John Garry's to Acked-by 
in patch 8 and 15; 

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: Insert PORTE_BROADCAST_RCVD events for resuming host
  scsi: hisi_sas: Add more logs for runtime suspend/resume
  scsi: libsas: Resume host while sending SMP IOs
  scsi: libsas: Add flag SAS_HA_RESUMING
  scsi: libsas: Refactor 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 host active while processing events
  scsi: hisi_sas: Use autosuspend for the host 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 | 24 ++++++--
 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, 180 insertions(+), 48 deletions(-)

-- 
2.33.0

[PATCH v2 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>
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.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 0239e2b4b84f..63059fb6d9ec 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -4950,7 +4950,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 v2 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>
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.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 66e63a336770..ad64ccd41420 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -611,12 +611,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 v2 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
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 0e841e8761c5..d581613d87c7 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -180,7 +180,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 v2 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 v2 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 ad64ccd41420..051092e294f7 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1428,11 +1428,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;
@@ -1509,22 +1511,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 v2 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 v2 07/15] scsi: libsas: Insert PORTE_BROADCAST_RCVD event for resuming host

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

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>
---
 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 v2 08/15] scsi: hisi_sas: Add more logs for runtime suspend/resume

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

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

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Acked-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 63059fb6d9ec..6d7fde38fe02 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -4904,6 +4904,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);
@@ -4919,11 +4921,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;
 }
 
@@ -4961,6 +4963,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 v2 09/15] scsi: libsas: Resume host while sending SMP IOs

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

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

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 v2 10/15] scsi: libsas: Add flag SAS_HA_RESUMING

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

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>
---
 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 v2 11/15] scsi: libsas: Refactor sas_queue_deferred_work()

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

In the second part of function __sas_drain_work(), it queues deferred work.
This functionality would be duplicated in other places, so refactor out
into 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 v2 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 function 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 v2 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 become suspended between processing a
phyup interrupt and the event being processed by libsas. As such, we can't
ensure 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 phyup
interrupt handler and pm_runtime_put_sync() in the work handler exit to
ensure that we stay always active. Since we only want to call
pm_runtime_get_noresume() for v3 hw, signal this will a new event,
HISI_PHYE_PHY_UP_PM.

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 |  4 +++-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 07b473de9136..15a58c955516 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -158,6 +158,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 051092e294f7..f46f679fe825 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -846,10 +846,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;
@@ -860,6 +861,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 =
@@ -869,9 +875,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 6d7fde38fe02..94eb48c93ab1 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -1561,7 +1561,9 @@ static irqreturn_t phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
 
 	phy->port_id = port_id;
 
-	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;
 
-- 
2.33.0

[PATCH v2 14/15] scsi: libsas: Keep host active while processing events

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

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 can't 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 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;
 }
-- 
2.33.0

[PATCH v2 15/15] scsi: hisi_sas: Use autosuspend for the host controller

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

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 and
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 modified via
sysfs interfaces.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Acked-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 94eb48c93ab1..a45ef9a5e12e 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -4783,6 +4783,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 v2 00/15] Add runtime PM support for libsas

[Martin K. Petersen]

chenxiang,

> This series addresses those issues, briefly described as follows:
> a. As far as we can see, this drain is unneeded, so conditionally remove it
> in patch 1~2;
> b. Just insert broadcast events to revalidate the topology in patch 4~7;
> c. and e. When processing any events from the LLD, make libsas keep the
> host active until finished processing all works related to the original
> events in patch 9 and 14;
> d. Defer phyup event processing in case described in patch 10~12;

Applied to 5.17/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Nathan Chancellor]

Hi Xiang,

On Mon, Dec 20, 2021 at 07:21:28PM +0800, chenxiang wrote:
> 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 ad64ccd41420..051092e294f7 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -1428,11 +1428,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;
> @@ -1509,22 +1511,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
> 
> 

Please ignore this if it was already reported, I do not see any reports
of it on lore.kernel.org nor a commit fixing it in Martin's tree.

This commit as commit 29e2bac87421 ("scsi: hisi_sas: Fix some issues
related to asd_sas_port->phy_list") in -next causes the following clang
warning, which will break the build under -Werror:

drivers/scsi/hisi_sas/hisi_sas_main.c:1536:21: error: variable 'sas_phy' is uninitialized when used here [-Werror,-Wuninitialized]
                if (!(state & BIT(sas_phy->id)))
                                  ^~~~~~~
./include/vdso/bits.h:7:30: note: expanded from macro 'BIT'
#define BIT(nr)                 (UL(1) << (nr))
                                           ^~
drivers/scsi/hisi_sas/hisi_sas_main.c:1528:29: note: initialize the variable 'sas_phy' to silence this warning
        struct asd_sas_phy *sas_phy;
                                   ^
                                    = NULL
1 error generated.

It seems like this variable is entirely unused now, should it be removed
along with this check?

Cheers,
Nathan

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

[chenxiang (M)]

Hi Nathan and Colin,
Thank you for your report.

在 2021/12/28 1:25, Nathan Chancellor 写道:
> Hi Xiang,
>
> On Mon, Dec 20, 2021 at 07:21:28PM +0800, chenxiang wrote:
>> 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 ad64ccd41420..051092e294f7 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> @@ -1428,11 +1428,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;
>> @@ -1509,22 +1511,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
>>
>>
> Please ignore this if it was already reported, I do not see any reports
> of it on lore.kernel.org nor a commit fixing it in Martin's tree.
>
> This commit as commit 29e2bac87421 ("scsi: hisi_sas: Fix some issues
> related to asd_sas_port->phy_list") in -next causes the following clang
> warning, which will break the build under -Werror:
>
> drivers/scsi/hisi_sas/hisi_sas_main.c:1536:21: error: variable 'sas_phy' is uninitialized when used here [-Werror,-Wuninitialized]
>                  if (!(state & BIT(sas_phy->id)))
>                                    ^~~~~~~
> ./include/vdso/bits.h:7:30: note: expanded from macro 'BIT'
> #define BIT(nr)                 (UL(1) << (nr))
>                                             ^~
> drivers/scsi/hisi_sas/hisi_sas_main.c:1528:29: note: initialize the variable 'sas_phy' to silence this warning
>          struct asd_sas_phy *sas_phy;
>                                     ^
>                                      = NULL
> 1 error generated.
>
> It seems like this variable is entirely unused now, should it be removed
> along with this check?
>

Right, it needs to be removed as the additional check is enough.

@Martin and @John Garry, could you have a review and consider to merge 
following patch ?

From: Xiang Chen <chenxiang66@hisilicon.com>
Date: Tue, 28 Dec 2021 09:40:01 +0800
Subject: [PATCH] scsi: libsas: Remove unused variable and check in function
  hisi_sas_send_ata_reset_each_phy()

In commit 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to
asd_sas_port->phy_list"), we use asd_sas_port->phy_mask instead of
accessing asd_sas_port->phy_list, and it is enough to use
asd_sas_port->phy_mask to check the state of phy, so removing the
old and unused check.

Fixes: 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to 
asd_sas_port->phy_list")
Reported-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
---
  drivers/scsi/hisi_sas/hisi_sas_main.c | 5 -----
  1 file changed, 5 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c 
b/drivers/scsi/hisi_sas/hisi_sas_main.c
index f46f679fe825..a05ec7aece5a 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1525,16 +1525,11 @@ static void 
hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba,
         struct device *dev = hisi_hba->dev;
         int s = sizeof(struct host_to_dev_fis);
         int rc = TMF_RESP_FUNC_FAILED;
-       struct asd_sas_phy *sas_phy;
         struct ata_link *link;
         u8 fis[20] = {0};
-       u32 state;
         int i;

-       state = hisi_hba->hw->get_phys_state(hisi_hba);
         for (i = 0; i < hisi_hba->n_phy; i++) {
-               if (!(state & BIT(sas_phy->id)))
-                       continue;
                 if (!(sas_port->phy_mask & BIT(i)))
                         continue;

--
2.33.0

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

[John Garry]

> 
> @Martin and @John Garry, could you have a review and consider to merge
> following patch ?

This series has not made it to Martin's 5.17 queue, but I suggest that 
you send it as a patch in case it does.

> 
> From: Xiang Chen <chenxiang66@hisilicon.com>
> Date: Tue, 28 Dec 2021 09:40:01 +0800
> Subject: [PATCH] scsi: libsas: Remove unused variable and check in function
>    hisi_sas_send_ata_reset_each_phy()

please try to condense these subjects, like just "Remove broken legacy 
code in hisi_sas_send_ata_reset_each_phy()"

Or at least remove "function", as this is obvious

> 
> In commit 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to
> asd_sas_port->phy_list"), we use asd_sas_port->phy_mask instead of
> accessing asd_sas_port->phy_list, and it is enough to use
> asd_sas_port->phy_mask to check the state of phy, so removing the

/s/removing/remove/

> old and unused check.
> 
> Fixes: 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to
> asd_sas_port->phy_list")
> Reported-by: Nathan Chancellor <nathan@kernel.org>

Colin King also reported this, so please add him.

> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> ---
>    drivers/scsi/hisi_sas/hisi_sas_main.c | 5 -----
>    1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
> b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index f46f679fe825..a05ec7aece5a 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -1525,16 +1525,11 @@ static void
> hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba,
>           struct device *dev = hisi_hba->dev;
>           int s = sizeof(struct host_to_dev_fis);
>           int rc = TMF_RESP_FUNC_FAILED;
> -       struct asd_sas_phy *sas_phy;
>           struct ata_link *link;
>           u8 fis[20] = {0};
> -       u32 state;
>           int i;
> 
> -       state = hisi_hba->hw->get_phys_state(hisi_hba);
>           for (i = 0; i < hisi_hba->n_phy; i++) {
> -               if (!(state & BIT(sas_phy->id)))
> -                       continue;
>                   if (!(sas_port->phy_mask & BIT(i)))
>                           continue;
> 
> --
> 2.33.0
> 
> 
> 

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

[chenxiang (M)]

Hi John,


在 2022/1/4 18:02, John Garry 写道:
>>
>> @Martin and @John Garry, could you have a review and consider to merge
>> following patch ?
>
> This series has not made it to Martin's 5.17 queue, but I suggest that 
> you send it as a patch in case it does.

Ok, i will send it as a patch.

>
>>
>> From: Xiang Chen <chenxiang66@hisilicon.com>
>> Date: Tue, 28 Dec 2021 09:40:01 +0800
>> Subject: [PATCH] scsi: libsas: Remove unused variable and check in 
>> function
>>    hisi_sas_send_ata_reset_each_phy()
>
> please try to condense these subjects, like just "Remove broken legacy 
> code in hisi_sas_send_ata_reset_each_phy()"
>
> Or at least remove "function", as this is obvious

I will remove "function".

>
>>
>> In commit 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to
>> asd_sas_port->phy_list"), we use asd_sas_port->phy_mask instead of
>> accessing asd_sas_port->phy_list, and it is enough to use
>> asd_sas_port->phy_mask to check the state of phy, so removing the
>
> /s/removing/remove/

Ok

>
>> old and unused check.
>>
>> Fixes: 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to
>> asd_sas_port->phy_list")
>> Reported-by: Nathan Chancellor <nathan@kernel.org>
>
> Colin King also reported this, so please add him.

Right, i will add him.

>
>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> ---
>>    drivers/scsi/hisi_sas/hisi_sas_main.c | 5 -----
>>    1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> index f46f679fe825..a05ec7aece5a 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> @@ -1525,16 +1525,11 @@ static void
>> hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba,
>>           struct device *dev = hisi_hba->dev;
>>           int s = sizeof(struct host_to_dev_fis);
>>           int rc = TMF_RESP_FUNC_FAILED;
>> -       struct asd_sas_phy *sas_phy;
>>           struct ata_link *link;
>>           u8 fis[20] = {0};
>> -       u32 state;
>>           int i;
>>
>> -       state = hisi_hba->hw->get_phys_state(hisi_hba);
>>           for (i = 0; i < hisi_hba->n_phy; i++) {
>> -               if (!(state & BIT(sas_phy->id)))
>> -                       continue;
>>                   if (!(sas_port->phy_mask & BIT(i)))
>>                           continue;
>>
>> -- 
>> 2.33.0
>>
>>
>>
>
> .
>