[PATCH v3 0/2] Fix SAS wildcard scan on smartpqi and other controllers

[PATCH v3 0/2] Fix SAS wildcard scan on smartpqi and other controllers

[Martin Wilck]

commit 37c4e72b0651 ("scsi: Fix sas_user_scan() to handle wildcard and
multi-channel scans") modified the way SAS drivers handle the common way of
rescanning SCSI devices using "echo - - - >/sys/class/scsi_host/host$N/scan".
Before this patch, SAS drivers would only scan channel 0 for this "wildcard
scan" scenario; after this patch, it would scan all channels up to
shost->max_channel.

This can cause massive resource usage for some drivers, as the driver needs to
send an INQUIRY to LUN 0 to every supported ID and e.g. smartpqi sets
shost->max_id to 0xffffffff. These INQUIRYs mostly fail, but the kernel needs
to set up queues, tag sets, etc. before sending the INQUIRY. Also, some SAS
drivers assign special meaning to SCSI channels such as channel 0 for physical
and channel 1 for logical devices, and thus don't support "normal" SCSI
scanning of these channels anyway.

With smartpqi and hisi_sas, actual kernel crashes due to resource exhaustion
have been observed.

v1 and v2 of this patch set proposed leveraging the functions scan_start() and
scan_finished() provided by some SAS drivers for fixing this issue. But the
review of the v2 series by Sashiko showed that this is a complex and
error-prone endeavor. Therefore I asked for some guidance on the ML [1].
I am now proposing a different, simpler solution to the issues we observed,
which is simply reverting commit 37c4e72b0651 ("scsi: Fix sas_user_scan() to
handle wildcard and multi-channel scans").

This revert will require a different solution for the wildcard scan with mpi3mr
and mpt3sas with "Tri-mode" enabled. But it seems wrong in the first place
that adding the support for this special mode of operation for some drivers
broke wildcard scan for other drivers.

The first patch is a minor fix for smartpqi.

[1] https://lore.kernel.org/linux-scsi/61b4da9dcbee8fd71d1ecb2cfdca5c2408528bd7.camel@suse.com/

----
Changes v2 -> v3:

- Removed patch 2 and replaced it by a revert of 37c4e72b0651

Changes v1 -> v2 (obsolete)

- export and call do_scsi_scan_host() instead of duplicating its code
  (Hannes Reinecke)

Martin Wilck (2):
  scsi: smartpqi: use shost_to_hba() in pqi_scan_finished()
  Revert "scsi: Fix sas_user_scan() to handle wildcard and multi-channel
    scans"

 drivers/scsi/scsi_scan.c              |  2 +-
 drivers/scsi/scsi_transport_sas.c     | 60 ++++++---------------------
 drivers/scsi/smartpqi/smartpqi_init.c |  2 +-
 3 files changed, 14 insertions(+), 50 deletions(-)

-- 
2.54.0

[PATCH v3 1/2] scsi: smartpqi: use shost_to_hba() in pqi_scan_finished()

[Martin Wilck]

From: Martin Wilck <martin.wilck@suse.com>

shost_to_hba() is used everywhere except to obtain pqi_ctrl_info
from shosti, except in pqi_scan_finished(), where shost_priv() is used.
This causes one pointer dereference to be missed, as shost->hostdata
is a pointer in smartpqi. Fix it.

Fixes: 6c223761eb54 ("smartpqi: initial commit of Microsemi smartpqi driver")
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Don Brace <don.brace@microchip.com>
Cc: Don Brace <don.brace@microchip.com>
Cc: storagedev@microchip.com
Cc: stable@vger.kernel.org
---
 drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index 2026ac645d6a..5ec583dc2e7d 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -2642,7 +2642,7 @@ static int pqi_scan_finished(struct Scsi_Host *shost,
 {
 	struct pqi_ctrl_info *ctrl_info;
 
-	ctrl_info = shost_priv(shost);
+	ctrl_info = shost_to_hba(shost);
 
 	return !mutex_is_locked(&ctrl_info->scan_mutex);
 }
-- 
2.54.0

[PATCH v3 2/2] Revert "scsi: Fix sas_user_scan() to handle wildcard and multi-channel scans"

[Martin Wilck]

This reverts commit 37c4e72b0651e7697eb338cd1fb09feef472cc1a.

Said commit causes excessive resource usage and even system freeze with
some controllers, e.g. smartpqi and hisi_sas. The justification provided
by the patch authors [1] was supporting a special mode of the mpi3mr and
mpt3sas, so-called "Tri-mode", in which NVMe drives are exposed as SCSI
devices on a separate channel. While that's useful for these drivers, it
seems wrong to cause major breakage for other drivers for the sake of
this feature.

[1] https://lore.kernel.org/linux-scsi/CAFdVvOwjy+2ORJ6uJkspiLTPF05481U7gcS4QohFOFGPqAs8ig@mail.gmail.com/

Fixes: 37c4e72b0651 ("scsi: Fix sas_user_scan() to handle wildcard and multi-channel scans")
Signed-off-by: Martin Wilck <mwilck@suse.com>
Cc: Don Brace <don.brace@microchip.com>
Cc: storagedev@microchip.com
Cc: Ranjan Kumar <ranjan.kumar@broadcom.com>
Cc: Sathya Prakash Veerichetty <sathya.prakash@broadcom.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: mpi3mr-linuxdrv.pdl@broadcom.com
Cc: MPT-FusionLinux.pdl@broadcom.com
Cc: Yihang Li <liyihang9@h-partners.c
---
 drivers/scsi/scsi_scan.c          |  2 +-
 drivers/scsi/scsi_transport_sas.c | 60 +++++++------------------------
 2 files changed, 13 insertions(+), 49 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index ef22a4228b85..b118ed0bf53f 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1910,7 +1910,7 @@ int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
 
 	return 0;
 }
-EXPORT_SYMBOL(scsi_scan_host_selected);
+
 static void scsi_sysfs_add_devices(struct Scsi_Host *shost)
 {
 	struct scsi_device *sdev;
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 13412702188e..d8f2377b017f 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -40,8 +40,6 @@
 #include <scsi/scsi_transport_sas.h>
 
 #include "scsi_sas_internal.h"
-#include "scsi_priv.h"
-
 struct sas_host_attrs {
 	struct list_head rphy_list;
 	struct mutex lock;
@@ -1685,22 +1683,6 @@ int scsi_is_sas_rphy(const struct device *dev)
 }
 EXPORT_SYMBOL(scsi_is_sas_rphy);
 
-static void scan_channel_zero(struct Scsi_Host *shost, uint id, u64 lun)
-{
-	struct sas_host_attrs *sas_host = to_sas_host_attrs(shost);
-	struct sas_rphy *rphy;
-
-	list_for_each_entry(rphy, &sas_host->rphy_list, list) {
-		if (rphy->identify.device_type != SAS_END_DEVICE ||
-		    rphy->scsi_target_id == -1)
-			continue;
-
-		if (id == SCAN_WILD_CARD || id == rphy->scsi_target_id) {
-			scsi_scan_target(&rphy->dev, 0, rphy->scsi_target_id,
-					 lun, SCSI_SCAN_MANUAL);
-		}
-	}
-}
 
 /*
  * SCSI scan helper
@@ -1710,41 +1692,23 @@ static int sas_user_scan(struct Scsi_Host *shost, uint channel,
 		uint id, u64 lun)
 {
 	struct sas_host_attrs *sas_host = to_sas_host_attrs(shost);
-	int res = 0;
-	int i;
+	struct sas_rphy *rphy;
 
-	switch (channel) {
-	case 0:
-		mutex_lock(&sas_host->lock);
-		scan_channel_zero(shost, id, lun);
-		mutex_unlock(&sas_host->lock);
-		break;
+	mutex_lock(&sas_host->lock);
+	list_for_each_entry(rphy, &sas_host->rphy_list, list) {
+		if (rphy->identify.device_type != SAS_END_DEVICE ||
+		    rphy->scsi_target_id == -1)
+			continue;
 
-	case SCAN_WILD_CARD:
-		mutex_lock(&sas_host->lock);
-		scan_channel_zero(shost, id, lun);
-		mutex_unlock(&sas_host->lock);
-
-		for (i = 1; i <= shost->max_channel; i++) {
-			res = scsi_scan_host_selected(shost, i, id, lun,
-						      SCSI_SCAN_MANUAL);
-			if (res)
-				goto exit_scan;
+		if ((channel == SCAN_WILD_CARD || channel == 0) &&
+		    (id == SCAN_WILD_CARD || id == rphy->scsi_target_id)) {
+			scsi_scan_target(&rphy->dev, 0, rphy->scsi_target_id,
+					 lun, SCSI_SCAN_MANUAL);
 		}
-		break;
-
-	default:
-		if (channel <= shost->max_channel) {
-			res = scsi_scan_host_selected(shost, channel, id, lun,
-						      SCSI_SCAN_MANUAL);
-		} else {
-			res = -EINVAL;
-		}
-		break;
 	}
+	mutex_unlock(&sas_host->lock);
 
-exit_scan:
-	return res;
+	return 0;
 }
 
 
-- 
2.54.0