[PATCH for-next 0/5] scsi: hisi_sas/libsas: Fix SATA devices missing issue during hisi_sas_debug_I_T_nexus_reset()

[PATCH for-next 0/5] scsi: hisi_sas/libsas: Fix SATA devices missing issue during hisi_sas_debug_I_T_nexus_reset()

[Jie Zhan]

SATA devices on an expander may be removed and not be found again
when I_T nexus reset and revalidation are processed simultaneously,
which is probable to happen in hisi_sas_clear_nexus_ha().

The issue came up since commit 71453bd9d1bf ("scsi: hisi_sas: Use
sas_ata_wait_after_reset() in IT nexus reset"), where the ex_phy's
change count is updated in hisi_sas_debug_I_T_nexus_reset(), stopping
future revalidation from discovering the SATA devices after link
reset.

This patchset fixes the issue.

Jie Zhan (5):
  Revert "scsi: hisi_sas: Drain bcast events in
    hisi_sas_rescan_topology()"
  Revert "scsi: hisi_sas: Don't send bcast events from HW during nexus
    HA reset"
  scsi: libsas: Add smp_ata_check_ready_type()
  scsi: hisi_sas: Fix SATA devices missing issue during I_T nexus reset
  scsi: libsas: Do not export sas_ata_wait_after_reset()

 drivers/scsi/hisi_sas/hisi_sas_main.c | 31 ++++++++-------------------
 drivers/scsi/libsas/sas_ata.c         | 28 ++++++++++++++++++++++--
 drivers/scsi/libsas/sas_expander.c    |  4 ++--
 drivers/scsi/libsas/sas_internal.h    |  2 ++
 include/scsi/sas_ata.h                |  7 +++---
 5 files changed, 42 insertions(+), 30 deletions(-)

-- 
2.30.0

[PATCH for-next 1/5] Revert "scsi: hisi_sas: Drain bcast events in hisi_sas_rescan_topology()"

[Jie Zhan]

This reverts commit 11ff0c98fca35df16c84d4eee52008faecaf10a6.

Draining or flushing events in hisi_sas_rescan_topology() can hang
the driver, typically with phy up or phy down events being processed,
i.e. sas_porte_bytes_dmaed() or sas_phye_loss_of_signal().

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 54860d252466..4527ac266bb6 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1323,7 +1323,6 @@ static void hisi_sas_refresh_port_id(struct hisi_hba *hisi_hba)
 
 static void hisi_sas_rescan_topology(struct hisi_hba *hisi_hba, u32 state)
 {
-	struct sas_ha_struct *sas_ha = &hisi_hba->sha;
 	struct asd_sas_port *_sas_port = NULL;
 	int phy_no;
 
@@ -1352,12 +1351,6 @@ static void hisi_sas_rescan_topology(struct hisi_hba *hisi_hba, u32 state)
 			hisi_sas_phy_down(hisi_hba, phy_no, 0, GFP_KERNEL);
 		}
 	}
-	/*
-	 * Ensure any bcast events are processed prior to calling async nexus
-	 * reset calls from hisi_sas_clear_nexus_ha() ->
-	 * hisi_sas_async_I_T_nexus_reset()
-	 */
-	sas_drain_work(sas_ha);
 }
 
 static void hisi_sas_reset_init_all_devices(struct hisi_hba *hisi_hba)
-- 
2.30.0

[PATCH for-next 2/5] Revert "scsi: hisi_sas: Don't send bcast events from HW during nexus HA reset"

[Jie Zhan]

This reverts commit f5f2a2716055ad8c0c4ff83e51d667646c6c5d8a.

This is now unnecessary to solve the SATA devices missing issue in
hisi_sas_clear_nexus_ha(). Hence, we should not ignore bcast events during
sas_eh_handle_sas_errors() in case of missing bcast events, unless a
justified need is found and a mechanism to defer (but not ignore) bcast
events in sas_eh_handle_sas_errors() is provided.

Also, in hisi_sas_clear_nexus_ha(), there is nothing further to handle in
"out: " other than return, so that part can be reverted.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 4527ac266bb6..62080d0fad6f 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1812,14 +1812,12 @@ static int hisi_sas_clear_nexus_ha(struct sas_ha_struct *sas_ha)
 	struct hisi_hba *hisi_hba = sas_ha->lldd_ha;
 	HISI_SAS_DECLARE_RST_WORK_ON_STACK(r);
 	ASYNC_DOMAIN_EXCLUSIVE(async);
-	int i, ret;
+	int i;
 
 	queue_work(hisi_hba->wq, &r.work);
 	wait_for_completion(r.completion);
-	if (!r.done) {
-		ret = TMF_RESP_FUNC_FAILED;
-		goto out;
-	}
+	if (!r.done)
+		return TMF_RESP_FUNC_FAILED;
 
 	for (i = 0; i < HISI_SAS_MAX_DEVICES; i++) {
 		struct hisi_sas_device *sas_dev = &hisi_hba->devices[i];
@@ -1836,9 +1834,7 @@ static int hisi_sas_clear_nexus_ha(struct sas_ha_struct *sas_ha)
 	async_synchronize_full_domain(&async);
 	hisi_sas_release_tasks(hisi_hba);
 
-	ret = TMF_RESP_FUNC_COMPLETE;
-out:
-	return ret;
+	return TMF_RESP_FUNC_COMPLETE;
 }
 
 static int hisi_sas_query_task(struct sas_task *task)
@@ -1986,14 +1982,10 @@ void hisi_sas_phy_bcast(struct hisi_sas_phy *phy)
 {
 	struct asd_sas_phy *sas_phy = &phy->sas_phy;
 	struct hisi_hba	*hisi_hba = phy->hisi_hba;
-	struct sas_ha_struct *sha = &hisi_hba->sha;
 
 	if (test_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags))
 		return;
 
-	if (test_bit(SAS_HA_FROZEN, &sha->state))
-		return;
-
 	sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
 }
 EXPORT_SYMBOL_GPL(hisi_sas_phy_bcast);
-- 
2.30.0

[PATCH for-next 3/5] scsi: libsas: Add smp_ata_check_ready_type()

[Jie Zhan]

Create function smp_ata_check_ready_type() for lldds to wait for
SATA devices to come up after a link reset.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/scsi/libsas/sas_ata.c      | 25 +++++++++++++++++++++++++
 drivers/scsi/libsas/sas_expander.c |  4 ++--
 drivers/scsi/libsas/sas_internal.h |  2 ++
 include/scsi/sas_ata.h             |  6 ++++++
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 78e6046fb55a..4b65cd79150f 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -287,6 +287,31 @@ static int sas_ata_clear_pending(struct domain_device *dev, struct ex_phy *phy)
 		return 1;
 }
 
+int smp_ata_check_ready_type(struct ata_link *link)
+{
+	struct domain_device *dev = link->ap->private_data;
+	struct sas_phy *phy = sas_get_local_phy(dev);
+	struct domain_device *ex_dev = dev->parent;
+	enum sas_device_type type = SAS_PHY_UNUSED;
+	u8 sas_addr[SAS_ADDR_SIZE];
+	int res;
+
+	res = sas_get_phy_attached_dev(ex_dev, phy->number, sas_addr, &type);
+	sas_put_local_phy(phy);
+	if (res)
+		return res;
+
+	switch (type) {
+	case SAS_SATA_PENDING:
+		return 0;
+	case SAS_END_DEVICE:
+		return 1;
+	default:
+		return -ENODEV;
+	}
+}
+EXPORT_SYMBOL_GPL(smp_ata_check_ready_type);
+
 static int smp_ata_check_ready(struct ata_link *link)
 {
 	int res;
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 2907ca5d0ed4..a04cad620e93 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1688,8 +1688,8 @@ static int sas_get_phy_change_count(struct domain_device *dev,
 	return res;
 }
 
-static int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
-				    u8 *sas_addr, enum sas_device_type *type)
+int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
+			     u8 *sas_addr, enum sas_device_type *type)
 {
 	int res;
 	struct smp_disc_resp *disc_resp;
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 6cf190ade35e..6f593fa69b58 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -88,6 +88,8 @@ struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id);
 int sas_ex_phy_discover(struct domain_device *dev, int single);
 int sas_get_report_phy_sata(struct domain_device *dev, int phy_id,
 			    struct smp_rps_resp *rps_resp);
+int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
+			     u8 *sas_addr, enum sas_device_type *type);
 int sas_try_ata_reset(struct asd_sas_phy *phy);
 void sas_hae_reset(struct work_struct *work);
 
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index e47f0aec0722..e7d466df8157 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -36,6 +36,7 @@ void sas_ata_device_link_abort(struct domain_device *dev, bool force_reset);
 int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
 			int force_phy_id);
 int sas_ata_wait_after_reset(struct domain_device *dev, unsigned long deadline);
+int smp_ata_check_ready_type(struct ata_link *link);
 #else
 
 
@@ -104,6 +105,11 @@ static inline int sas_ata_wait_after_reset(struct domain_device *dev,
 {
 	return -ETIMEDOUT;
 }
+
+static inline int smp_ata_check_ready_type(struct ata_link *link)
+{
+	return 0;
+}
 #endif
 
 #endif /* _SAS_ATA_H_ */
-- 
2.30.0

[PATCH for-next 4/5] scsi: hisi_sas: Fix SATA devices missing issue during I_T nexus reset

[Jie Zhan]

SATA devices on an expander may be removed and not be found again
when I_T nexus reset and revalidation are processed simultaneously.

The issue comes from:

- Revalidation can remove SATA devices in link reset, e.g. in
  hisi_sas_clear_nexus_ha().

- However, hisi_sas_debug_I_T_nexus_reset() polls the state of a SATA
  device on an expander after sending link_reset, where it calls:
   hisi_sas_debug_I_T_nexus_reset
    sas_ata_wait_after_reset
     ata_wait_after_reset
      ata_wait_ready
       smp_ata_check_ready
        sas_ex_phy_discover
         sas_ex_phy_discover_helper
          sas_set_ex_phy
  The ex_phy's change count is updated in sas_set_ex_phy(), so SATA devices
  after a link reset may not be found later through revalidation.

A similar issue was reported in:
commit 0f3fce5cc77e ("[SCSI] libsas: fix ata_eh clobbering ex_phys via
smp_ata_check_ready")
commit 87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing
with ata error handling").

To address this issue, in hisi_sas_debug_I_T_nexus_reset(), we now call
smp_ata_check_ready_type() that only polls the device type while not
updating the ex_phy's data of libsas.

Fixes: 71453bd9d1bf ("scsi: hisi_sas: Use sas_ata_wait_after_reset() in IT nexus reset")
Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 62080d0fad6f..41ba22f6c7f0 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1694,13 +1694,15 @@ static int hisi_sas_debug_I_T_nexus_reset(struct domain_device *device)
 		return rc;
 	}
 
+	/* Remote phy */
 	if (rc)
 		return rc;
 
-	/* Remote phy */
 	if (dev_is_sata(device)) {
-		rc = sas_ata_wait_after_reset(device,
-					HISI_SAS_WAIT_PHYUP_TIMEOUT);
+		struct ata_link *link = &device->sata_dev.ap->link;
+
+		rc = ata_wait_after_reset(link, HISI_SAS_WAIT_PHYUP_TIMEOUT,
+					  smp_ata_check_ready_type);
 	} else {
 		msleep(2000);
 	}
-- 
2.30.0

[PATCH for-next 5/5] scsi: libsas: Do not export sas_ata_wait_after_reset()

[Jie Zhan]

sas_ata_wait_after_reset() does not need to be exported since it is
no longer referenced outside libsas.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c | 3 +--
 include/scsi/sas_ata.h        | 7 -------
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 4b65cd79150f..f7439bf9cdc6 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -383,7 +383,7 @@ static int sas_ata_printk(const char *level, const struct domain_device *ddev,
 	return r;
 }
 
-int sas_ata_wait_after_reset(struct domain_device *dev, unsigned long deadline)
+static int sas_ata_wait_after_reset(struct domain_device *dev, unsigned long deadline)
 {
 	struct sata_device *sata_dev = &dev->sata_dev;
 	int (*check_ready)(struct ata_link *link);
@@ -405,7 +405,6 @@ int sas_ata_wait_after_reset(struct domain_device *dev, unsigned long deadline)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(sas_ata_wait_after_reset);
 
 static int sas_ata_hard_reset(struct ata_link *link, unsigned int *class,
 			      unsigned long deadline)
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index e7d466df8157..9c927d46f136 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -35,7 +35,6 @@ void sas_ata_end_eh(struct ata_port *ap);
 void sas_ata_device_link_abort(struct domain_device *dev, bool force_reset);
 int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
 			int force_phy_id);
-int sas_ata_wait_after_reset(struct domain_device *dev, unsigned long deadline);
 int smp_ata_check_ready_type(struct ata_link *link);
 #else
 
@@ -100,12 +99,6 @@ static inline int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
 	return 0;
 }
 
-static inline int sas_ata_wait_after_reset(struct domain_device *dev,
-					   unsigned long deadline)
-{
-	return -ETIMEDOUT;
-}
-
 static inline int smp_ata_check_ready_type(struct ata_link *link)
 {
 	return 0;
-- 
2.30.0

Re: [PATCH for-next 0/5] scsi: hisi_sas/libsas: Fix SATA devices missing issue during hisi_sas_debug_I_T_nexus_reset()

[Martin K. Petersen]

Jie,

> SATA devices on an expander may be removed and not be found again
> when I_T nexus reset and revalidation are processed simultaneously,
> which is probable to happen in hisi_sas_clear_nexus_ha().

Applied to 6.2/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH for-next 0/5] scsi: hisi_sas/libsas: Fix SATA devices missing issue during hisi_sas_debug_I_T_nexus_reset()

[Martin K. Petersen]

On Fri, 18 Nov 2022 16:37:09 +0800, Jie Zhan wrote:

> SATA devices on an expander may be removed and not be found again
> when I_T nexus reset and revalidation are processed simultaneously,
> which is probable to happen in hisi_sas_clear_nexus_ha().
> 
> The issue came up since commit 71453bd9d1bf ("scsi: hisi_sas: Use
> sas_ata_wait_after_reset() in IT nexus reset"), where the ex_phy's
> change count is updated in hisi_sas_debug_I_T_nexus_reset(), stopping
> future revalidation from discovering the SATA devices after link
> reset.
> 
> [...]

Applied to 6.2/scsi-queue, thanks!

[1/5] Revert "scsi: hisi_sas: Drain bcast events in hisi_sas_rescan_topology()"
      https://git.kernel.org/mkp/scsi/c/7e613be7c63d
[2/5] Revert "scsi: hisi_sas: Don't send bcast events from HW during nexus HA reset"
      https://git.kernel.org/mkp/scsi/c/94a3555d1f0f
[3/5] scsi: libsas: Add smp_ata_check_ready_type()
      https://git.kernel.org/mkp/scsi/c/9181ce3cb5d9
[4/5] scsi: hisi_sas: Fix SATA devices missing issue during I_T nexus reset
      https://git.kernel.org/mkp/scsi/c/3c2673a09cf1
[5/5] scsi: libsas: Do not export sas_ata_wait_after_reset()
      https://git.kernel.org/mkp/scsi/c/4d450cf2b00d

-- 
Martin K. Petersen	Oracle Linux Engineering