[PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan

[PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan

[Brian Bunker]

This series teaches the SCSI rescan path to refetch standard INQUIRY data
and reprobe the device when the peripheral qualifier or device type has
changed.  The motivating case is an ALUA target that transitions through
the "unavailable" state and afterwards reports a different peripheral
qualifier; today the kernel keeps the stale INQUIRY data and the device's
sysfs attributes diverge from what the target reports.

  1/5 serializes the INQUIRY-derived sysfs attributes under
      sdev->inquiry_mutex so a concurrent reprobe cannot tear the read.
  2/5 adds scsi_update_inquiry_data() as the single point that stores
      INQUIRY data in sdev and updates the fields derived from it.
      Returns SCSI_INQ_REPROBE_NEEDED only when peripheral qualifier or
      device type changes, as these are the only fields that determine
      driver binding.
  3/5 refactors scsi_add_lun() to use scsi_update_inquiry_data(), sharing
      one implementation between initial-probe and reprobe paths.
  4/5 teaches scsi_rescan_device() to refetch INQUIRY and reprobe the
      device when scsi_update_inquiry_data() reports a change.
  5/5 calls scsi_rescan_device() from the scan path for existing devices
      so that a SCAN_LUNS pass refreshes them as well.

Changes since v7 (prompted by the Sashiko review of v7 unless noted):
  - 1/5: Also take inquiry_mutex in the modalias attribute, and read
    sdev->type with READ_ONCE() in scsi_bus_uevent(), which gets a const
    device. Dropped Hannes' Reviewed-by.
  - 2/5: Compute scsi_level and type in local variables and store each
    once with WRITE_ONCE(). Both are read without inquiry_mutex (the I/O
    path, upper-level drivers, scsi_bus_uevent()), and v7 stored an
    intermediate value first: the unincremented scsi_level, and the
    uncorrected type of a well-known LUN. Found while addressing the
    1/5 comment, not by Sashiko.
  - 4/5: Set the device offline after a failed device_reprobe() under
    state_mutex, so a concurrent __scsi_remove_device() cannot have
    SDEV_CANCEL or SDEV_DEL overwritten with SDEV_OFFLINE.
  - 4/5: scsi_probe_lun() takes a rescan argument and, on a rescan,
    no longer writes the host's short_inquiry flag, the target's
    scsi_level or lun_in_cdb. These share words with bitfields that are
    written at runtime without a common lock (tmf_in_progress,
    cdl_enable); they are only set when a device is first probed.
  - Rebased onto the current 7.4/scsi-staging; no conflicts.

Changes since v6 (all prompted by the Sashiko review of v6):
  - 2/5: sdev->inquiry is now allocated once at its maximum size
    (256 bytes) and later updates are copied into it in place. v6 freed
    and replaced the buffer, which is a use-after-free for the helpers
    in scsi_device.h that read sdev->inquiry without a lock
    (scsi_device_tpgs(), scsi_device_protection(), ...). The fixed size
    also means a growing inquiry_len can never run past the buffer, and
    inq_len is clamped to 255 to fit the unsigned char inquiry_len.
  - 2/5: Capability flags (removable, lockable, ppr, wdtr, sdtr,
    tagged_supported, simple_tags, soft_reset, is_ata, allow_restart)
    are now set only on the first call. Rewriting them on a live device
    undid adjustments made after probe (BLIST_NOT_LOCKABLE, usb-storage
    and scsi_ioctl clearing lockable, aacraid forcing tagged_supported,
    ...), and raced with runtime state bits in the same word. A rescan
    now updates the INQUIRY buffer, vendor/model/rev, PQ, type and
    scsi_level only. Dropped Hannes' Reviewed-by.
  - 3/5: No code changes.
  - 4/5: Rewritten. If the INQUIRY refresh fails, the rescan carries on
    with the existing data instead of skipping the VPD update and the
    driver rescan and returning success. The device handler rescan
    method is called in all cases again, as before this series. The
    get_device()/put_device() pair around device_reprobe() is removed:
    callers hold a reference, and dropping the last one from
    scsi_evt_thread() would have run the release function from the
    work it cancels. The reprobe failure path now checks whether the
    device could actually be set offline before saying so.
    scsi_probe_lun() is now called for live devices, so it returns the
    INQUIRY length instead of storing it in sdev (the length is updated
    with the data under inquiry_mutex), and only writes lun_in_cdb and
    the target's scsi_level when they change. Dropped Hannes'
    Reviewed-by.
  - 5/5: Rewritten. v6 allocated a second scsi_device when an initial
    scan found a device still in SDEV_CREATED, passed a length of 256
    that truncated inquiry_len to 0, leaked a reference for pseudo
    devices, and called the driver rescan method without the device
    lock. The scan path is now unchanged apart from calling
    scsi_rescan_device() for an existing, fully set up device. Dropped
    Hannes' Reviewed-by.
  - Build-tested every patch with gcc W=1, and the series with clang 23
    and CONFIG_WARN_CONTEXT_ANALYSIS=y; no new warnings.

v7 cover-letter Message-ID: <20261014220014.94330-1-brian@purestorage.com>
v6 cover-letter Message-ID: <20261012210511.16061-1-brian@purestorage.com>
v5 cover-letter Message-ID: <20260618233508.97960-1-brian@purestorage.com>
v4 cover-letter Message-ID: <20260530002019.47109-1-brian@purestorage.com>
v3 cover-letter Message-ID: <20260429224939.77082-1-brian@purestorage.com>
v2 cover-letter Message-ID: <20260429012733.40855-1-brian@purestorage.com>
v1 cover-letter Message-ID: <20260424215324.99045-1-brian@purestorage.com>

Brian Bunker (5):
  scsi: core: Protect INQUIRY sysfs attributes with mutex
  scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data
  scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()
  scsi: core: Add device reprobe support to scsi_rescan_device()
  scsi: core: Handle reprobe for existing devices during SCSI scan

 drivers/scsi/scsi.c        | 215 +++++++++++++++++++++++++++++
 drivers/scsi/scsi_scan.c   | 271 +++++++++++++++++++++----------------
 drivers/scsi/scsi_sysfs.c  |  37 +++--
 include/scsi/scsi_device.h |  13 ++
 4 files changed, 410 insertions(+), 126 deletions(-)


base-commit: db8b4d2bf2d3e02b4ff78bbf69d00e11795840de
-- 
2.56.0

[PATCH v8 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex

[Brian Bunker]

All INQUIRY-derived sysfs attributes (type, scsi_level, vendor, model,
rev, cdl_supported, modalias, and the binary inquiry attribute) read
data that can be updated during device rescan. These reads must be
protected against concurrent updates.

Use the existing inquiry_mutex to protect access to these sysfs
attributes. This ensures that userspace always sees consistent INQUIRY
data, even if a rescan is updating the buffer concurrently.

Update the sdev_rd_attr macro to take inquiry_mutex around the field
access and switch to sysfs_emit. Since vendor, model, and rev are
NUL-terminated fixed-size arrays, %s format handles all field types
correctly.

scsi_bus_uevent() gets a const device and only needs the single-byte
type for MODALIAS, so read it with READ_ONCE() instead of taking the
mutex.

This is preparatory work for adding INQUIRY data update support during
device rescan operations.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_sysfs.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index fe54792ed69e..5f3e3fcdbad3 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -551,7 +551,13 @@ static int scsi_bus_uevent(const struct device *dev, struct kobj_uevent_env *env
 
 	sdev = to_scsi_device(dev);
 
-	add_uevent_var(env, "MODALIAS=" SCSI_DEVICE_MODALIAS_FMT, sdev->type);
+	/*
+	 * sdev->type is a single byte that a rescan may update under
+	 * inquiry_mutex, so a lockless read sees either the old or the new
+	 * type.
+	 */
+	add_uevent_var(env, "MODALIAS=" SCSI_DEVICE_MODALIAS_FMT,
+		       READ_ONCE(sdev->type));
 	return 0;
 }
 
@@ -638,21 +644,30 @@ sdev_show_##field (struct device *dev, struct device_attribute *attr,	\
 
 /*
  * sdev_rd_attr: macro to create a function and attribute variable for a
- * read only field.
+ * read-only field. inquiry_mutex protects INQUIRY-derived fields against
+ * concurrent updates during device rescan.
  */
 #define sdev_rd_attr(field, format_string)				\
-	sdev_show_function(field, format_string)			\
-static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL);
+static ssize_t								\
+sdev_show_##field(struct device *dev, struct device_attribute *attr,	\
+		  char *buf)						\
+{									\
+	struct scsi_device *sdev = to_scsi_device(dev);			\
+									\
+	guard(mutex)(&sdev->inquiry_mutex);				\
+	return sysfs_emit(buf, format_string, sdev->field);		\
+}									\
+static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL)
 
 /*
  * Create the actual show/store functions and data structures.
  */
-sdev_rd_attr (type, "%d\n");
-sdev_rd_attr (scsi_level, "%d\n");
-sdev_rd_attr (vendor, "%.8s\n");
-sdev_rd_attr (model, "%.16s\n");
-sdev_rd_attr (rev, "%.4s\n");
-sdev_rd_attr (cdl_supported, "%d\n");
+sdev_rd_attr(type, "%d\n");
+sdev_rd_attr(scsi_level, "%d\n");
+sdev_rd_attr(cdl_supported, "%d\n");
+sdev_rd_attr(vendor, "%s\n");
+sdev_rd_attr(model, "%s\n");
+sdev_rd_attr(rev, "%s\n");
 
 static ssize_t
 sdev_show_device_busy(struct device *dev, struct device_attribute *attr,
@@ -917,6 +932,7 @@ static ssize_t show_inquiry(struct file *filep, struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct scsi_device *sdev = to_scsi_device(dev);
 
+	guard(mutex)(&sdev->inquiry_mutex);
 	if (!sdev->inquiry)
 		return -EINVAL;
 
@@ -963,6 +979,7 @@ sdev_show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct scsi_device *sdev;
 	sdev = to_scsi_device(dev);
+	guard(mutex)(&sdev->inquiry_mutex);
 	return snprintf (buf, 20, SCSI_DEVICE_MODALIAS_FMT "\n", sdev->type);
 }
 static DEVICE_ATTR(modalias, S_IRUGO, sdev_show_modalias, NULL);
-- 
2.56.0

[PATCH v8 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data

[Brian Bunker]

Add a new function scsi_update_inquiry_data() that stores standard
INQUIRY data for a SCSI device and updates the fields derived from it:

- Vendor, model, revision strings
- Peripheral qualifier, device type and SCSI level
- On the first call only: capability flags (removable, lockable,
  tagged queuing support, transfer modes, ATA detection, ...)

The function:
- Takes the inquiry_mutex to protect against concurrent sysfs reads
- Allocates sdev->inquiry once at its maximum size and copies later
  updates into it in place. Several helpers in <scsi/scsi_device.h>
  (scsi_device_tpgs(), scsi_device_protection(), ...) read
  sdev->inquiry without any lock, so the buffer must never be freed
  while the device is live.
- Computes scsi_level and type in local variables and stores each one
  once with WRITE_ONCE(). Both are also read without inquiry_mutex (by
  the I/O path, upper-level drivers and the uevent code), so a lockless
  reader must never see an intermediate value, such as the unincremented
  scsi_level or the uncorrected type of a well-known LUN.
- Leaves the capability flags alone when refreshing an existing device.
  LLDs and upper-level drivers adjust several of them after the initial
  probe (usb-storage and scsi_ioctl clear lockable, aacraid forces
  tagged_supported, ...), and they share a word with runtime state bits
  that are written without inquiry_mutex.
- Applies the BLIST_ISROM type override on every call. The BLIST_ISROM
  removable override and BLIST_NOTQ only affect the capability flags,
  so they are applied on the first call only
- Returns 1 if device type or peripheral qualifier changed, indicating
  the caller should call device_reprobe() to re-match drivers
- Returns 0 on success with no changes requiring reprobe
- Returns negative errno on failure

This is the core infrastructure needed for updating INQUIRY data during
device rescan operations, which is required for proper ALUA unavailable
state handling.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi.c        | 215 +++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  13 +++
 2 files changed, 228 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3af9e77847f7..6a421961dbd0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -61,6 +61,7 @@
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_device.h>
+#include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_host.h>
@@ -549,6 +550,220 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	kfree(vpd_buf);
 }
 
+/*
+ * Size of the buffer that holds sdev->inquiry. sdev->inquiry_len is an
+ * unsigned char, so at most 255 bytes of INQUIRY data are ever stored.
+ */
+#define SCSI_INQUIRY_BUF_LEN	256
+
+/*
+ * Set the capability flags derived from INQUIRY data. These are only set
+ * when the device is first probed: LLDs and upper-level drivers adjust
+ * several of them afterwards (lockable, tagged_supported, ppr, ...), and
+ * they share a word with runtime state bits that are updated without
+ * inquiry_mutex, so they must not be rewritten on a live device.
+ */
+static void scsi_set_inquiry_flags(struct scsi_device *sdev,
+				   unsigned char *inq_result, size_t inq_len)
+	__must_hold(&sdev->inquiry_mutex)
+{
+	/*
+	 * Check if this is an ATA device (SATA emulation layer).
+	 * ATA devices need allow_restart set to work around SATL power
+	 * management specifications.
+	 */
+	if (strncmp(sdev->vendor, "ATA     ", 8) == 0) {
+		sdev->is_ata = 1;
+		sdev->allow_restart = 1;
+	}
+
+	/*
+	 * BLIST_ISROM is a quirk for devices that report the wrong type but
+	 * should be treated as (removable) CD-ROM.
+	 */
+	if (sdev->sdev_bflags & BLIST_ISROM)
+		sdev->removable = 1;
+	else
+		sdev->removable = (inq_result[1] & 0x80) >> 7;
+
+	/*
+	 * Set lockable to match removable. Devices with removable media
+	 * can typically have their media locked/unlocked via the
+	 * ALLOW_MEDIUM_REMOVAL command.
+	 */
+	sdev->lockable = sdev->removable;
+
+	/* Capability flags from INQUIRY bytes 3, 7 and 56 */
+	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
+	if (sdev->scsi_level >= SCSI_3 ||
+	    (inq_len > 56 && inq_result[56] & 0x04))
+		sdev->ppr = 1;
+	if (inq_result[7] & 0x60)
+		sdev->wdtr = 1;
+	if (inq_result[7] & 0x10)
+		sdev->sdtr = 1;
+
+	/* BLIST_NOTQ forces tagged queuing off */
+	if (sdev->scsi_level >= SCSI_2 && (inq_result[7] & 2) &&
+	    !(sdev->sdev_bflags & BLIST_NOTQ)) {
+		sdev->tagged_supported = 1;
+		sdev->simple_tags = 1;
+	}
+}
+
+/**
+ * scsi_update_inquiry_data - Update standard INQUIRY data for a SCSI device
+ * @sdev: The device to update
+ * @inq_result: Buffer containing new INQUIRY data
+ * @inq_len: Length of inquiry data
+ *
+ * Stores the standard INQUIRY data for @sdev and updates the fields derived
+ * from it: vendor, model, revision, peripheral qualifier, device type and
+ * SCSI level. This is used both during initial device setup and when an
+ * existing device is rescanned. On the first call the capability flags
+ * derived from INQUIRY data are set as well; see scsi_set_inquiry_flags().
+ *
+ * The INQUIRY buffer is allocated once, at its maximum size, and later
+ * updates copy the new data into it in place. Several helpers in
+ * <scsi/scsi_device.h> read sdev->inquiry without holding inquiry_mutex,
+ * so the buffer must not be freed or reallocated while the device is live.
+ *
+ * The BLIST_ISROM type override is applied on every call. Its removable
+ * override, like BLIST_NOTQ, only affects the capability flags and is
+ * therefore only applied on the first call.
+ *
+ * Returns:
+ *   SCSI_INQ_UNCHANGED on success
+ *   SCSI_INQ_REPROBE_NEEDED if peripheral qualifier or device type changed
+ *  -ENOMEM on allocation failure
+ *  -EINVAL if inquiry data is too short
+ */
+int scsi_update_inquiry_data(struct scsi_device *sdev,
+			     unsigned char *inq_result, size_t inq_len)
+{
+	unsigned char *new_inquiry = NULL;
+	unsigned char old_type;
+	unsigned char old_periph_qual;
+	unsigned char type;
+	char scsi_level;
+	bool refresh;
+
+	/*
+	 * Ensure we have at least the minimum standard INQUIRY data (36 bytes)
+	 * to safely access device type, vendor, model, rev, and capability flags.
+	 */
+	if (inq_len < 36) {
+		sdev_printk(KERN_WARNING, sdev,
+			    "INQUIRY data too short (%zu bytes), need at least 36\n",
+			    inq_len);
+		return -EINVAL;
+	}
+	inq_len = min_t(size_t, inq_len, SCSI_INQUIRY_BUF_LEN - 1);
+
+	/*
+	 * The buffer is only allocated on the first call, from scsi_add_lun(),
+	 * before the device is visible to anyone else.
+	 */
+	if (!sdev->inquiry) {
+		new_inquiry = kzalloc(SCSI_INQUIRY_BUF_LEN, GFP_KERNEL);
+		if (!new_inquiry)
+			return -ENOMEM;
+	}
+
+	mutex_lock(&sdev->inquiry_mutex);
+
+	/*
+	 * Save peripheral qualifier and device type before updating. These
+	 * are the only two values that determine whether device_reprobe() is
+	 * needed: type controls which upper-layer driver (sd, st, sr, ...) is
+	 * bound, and PQ controls whether the LUN is accessible at all
+	 * (scsi_bus_match() only matches PQ == 0).
+	 */
+	refresh = !new_inquiry;
+	old_type = sdev->type;
+	old_periph_qual = sdev->inq_periph_qual;
+
+	if (new_inquiry)
+		sdev->inquiry = new_inquiry;
+	memcpy(sdev->inquiry, inq_result, inq_len);
+	memset(sdev->inquiry + inq_len, 0, SCSI_INQUIRY_BUF_LEN - inq_len);
+	sdev->inquiry_len = inq_len;
+
+	strscpy(sdev->vendor, sdev->inquiry + INQUIRY_VENDOR_OFFSET);
+	strscpy(sdev->model, sdev->inquiry + INQUIRY_MODEL_OFFSET);
+	/*
+	 * memcpy() instead of strscpy() because strscpy() would read past
+	 * the end of the INQUIRY data if its length is exactly 36 bytes.
+	 */
+	memcpy(sdev->rev, sdev->inquiry + INQUIRY_REVISION_OFFSET,
+	       INQUIRY_REVISION_LEN);
+	sdev->rev[INQUIRY_REVISION_LEN] = '\0';
+	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
+
+	/*
+	 * Compute scsi_level from INQUIRY bytes 2 and 3. This must be
+	 * updated under inquiry_mutex alongside the other INQUIRY-derived
+	 * fields so sysfs readers always see a consistent snapshot.
+	 *
+	 * scsi_level and type are also read without inquiry_mutex, by the
+	 * I/O path, upper-level drivers and scsi_bus_uevent(). Compute them
+	 * in local variables and store each one once, so that a lockless
+	 * reader sees either the old or the new value, never an
+	 * intermediate one.
+	 */
+	scsi_level = inq_result[2] & 0x0f;
+	if (scsi_level >= 2 ||
+	    (scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
+		scsi_level++;
+	WRITE_ONCE(sdev->scsi_level, scsi_level);
+
+	/*
+	 * Update device type from INQUIRY byte 0. BLIST_ISROM overrides the
+	 * reported type with TYPE_ROM.
+	 */
+	if (sdev->sdev_bflags & BLIST_ISROM) {
+		type = TYPE_ROM;
+	} else {
+		type = inq_result[0] & 0x1f;
+
+		/*
+		 * Some devices may respond with wrong type for well-known
+		 * logical units. Force well-known type to enumerate them
+		 * correctly.
+		 */
+		if (scsi_is_wlun(sdev->lun) && type != TYPE_WLUN) {
+			sdev_printk(KERN_WARNING, sdev,
+				"%s: correcting incorrect peripheral device type 0x%x for W-LUN 0x%16xhN\n",
+				__func__, type,
+				(unsigned int)sdev->lun);
+			type = TYPE_WLUN;
+		}
+	}
+	WRITE_ONCE(sdev->type, type);
+
+	if (!refresh)
+		scsi_set_inquiry_flags(sdev, inq_result, inq_len);
+
+	mutex_unlock(&sdev->inquiry_mutex);
+
+	if (refresh) {
+		if (old_type != sdev->type) {
+			sdev_printk(KERN_NOTICE, sdev,
+				    "device type changed from %d to %d\n",
+				    old_type, sdev->type);
+			return SCSI_INQ_REPROBE_NEEDED;
+		}
+		if (old_periph_qual != sdev->inq_periph_qual) {
+			sdev_printk(KERN_NOTICE, sdev,
+				    "peripheral qualifier changed from %d to %d\n",
+				    old_periph_qual, sdev->inq_periph_qual);
+			return SCSI_INQ_REPROBE_NEEDED;
+		}
+	}
+
+	return SCSI_INQ_UNCHANGED;
+}
+
 /**
  * scsi_report_opcode - Find out if a given command is supported
  * @sdev:	scsi device to query
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index e6fd6bc30644..743babf1602c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -408,6 +408,19 @@ void scsi_attach_vpd(struct scsi_device *sdev);
 void scsi_cdl_check(struct scsi_device *sdev);
 int scsi_cdl_enable(struct scsi_device *sdev, bool enable);
 
+/**
+ * enum scsi_inq_update_result - Return values for scsi_update_inquiry_data()
+ * @SCSI_INQ_UNCHANGED: INQUIRY data updated, no reprobe needed
+ * @SCSI_INQ_REPROBE_NEEDED: INQUIRY data updated, standard INQUIRY data changed
+ */
+enum scsi_inq_update_result {
+	SCSI_INQ_UNCHANGED = 0,
+	SCSI_INQ_REPROBE_NEEDED = 1,
+};
+
+int scsi_update_inquiry_data(struct scsi_device *sdev,
+			     unsigned char *inq_result, size_t inq_len);
+
 extern int __must_check scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
-- 
2.56.0

[PATCH v8 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()

[Brian Bunker]

Refactor scsi_add_lun() to use the new scsi_update_inquiry_data()
function instead of inline INQUIRY parsing code. This consolidates
INQUIRY data handling in one place and ensures consistent behavior
between initial device setup and device rescan operations.

The following fields are now set by scsi_update_inquiry_data():
- inquiry buffer, vendor, model, rev pointers
- type, removable, lockable
- inq_periph_qual
- soft_reset, ppr, wdtr, sdtr
- tagged_supported, simple_tags
- is_ata, allow_restart

Also update scsi_probe_lun() to compute scsi_level into a local
variable rather than writing directly to sdev->scsi_level.
scsi_update_inquiry_data() is now the authoritative setter of
sdev->scsi_level under inquiry_mutex; scsi_probe_lun() needs the
level early for lun_in_cdb and sdev_target->scsi_level before
scsi_update_inquiry_data() is called.

scsi_add_lun() is only ever called for freshly allocated sdev instances
where sdev->inquiry is NULL, so the redundant !sdev->inquiry guard is
dropped along with the now-unreachable sanity check that followed it.

This patch maintains identical behavior to the previous code.
scsi_add_lun() continues to handle the remaining BLIST flags and
device-specific setup that doesn't come directly from INQUIRY data.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_scan.c | 127 +++++++++------------------------------
 1 file changed, 29 insertions(+), 98 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 8aae12fd38f3..93a8719fcf26 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -650,6 +650,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	int first_inquiry_len, try_inquiry_len, next_inquiry_len;
 	int response_len = 0;
 	int pass, count, result, resid;
+	char scsi_level;
 	struct scsi_failure failure_defs[] = {
 		/*
 		 * not-ready to ready transition or power-on, reset, continue.
@@ -838,23 +839,26 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	 */
 
 	/*
-	 * The scanning code needs to know the scsi_level, even if no
-	 * device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so
-	 * non-zero LUNs can be scanned.
+	 * The scanning code needs to know the scsi_level before
+	 * scsi_update_inquiry_data() is called, both to set the target
+	 * scsi_level and to determine lun_in_cdb. Use a local variable
+	 * here; sdev->scsi_level is set later under inquiry_mutex in
+	 * scsi_update_inquiry_data() to avoid races with concurrent sysfs
+	 * readers.
 	 */
-	sdev->scsi_level = inq_result[2] & 0x0f;
-	if (sdev->scsi_level >= 2 ||
-	    (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
-		sdev->scsi_level++;
-	sdev->sdev_target->scsi_level = sdev->scsi_level;
+	scsi_level = inq_result[2] & 0x0f;
+	if (scsi_level >= 2 ||
+	    (scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
+		scsi_level++;
+	sdev->sdev_target->scsi_level = scsi_level;
 
 	/*
 	 * If SCSI-2 or lower, and if the transport requires it,
 	 * store the LUN value in CDB[1].
 	 */
 	sdev->lun_in_cdb = 0;
-	if (sdev->scsi_level <= SCSI_2 &&
-	    sdev->scsi_level != SCSI_UNKNOWN &&
+	if (scsi_level <= SCSI_2 &&
+	    scsi_level != SCSI_UNKNOWN &&
 	    !sdev->host->no_scsi2_lun_in_cdb)
 		sdev->lun_in_cdb = 1;
 
@@ -883,17 +887,6 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	struct queue_limits lim;
 	int ret;
 
-	/*
-	 * XXX do not save the inquiry, since it can change underneath us,
-	 * save just vendor/model/rev.
-	 *
-	 * Rather than save it and have an ioctl that retrieves the saved
-	 * value, have an ioctl that executes the same INQUIRY code used
-	 * in scsi_probe_lun, let user level programs doing INQUIRY
-	 * scanning run at their own risk, or supply a user level program
-	 * that can correctly scan.
-	 */
-
 	/*
 	 * Copy at least 36 bytes of INQUIRY data, so that we don't
 	 * dereference unallocated memory when accessing the Vendor,
@@ -902,54 +895,26 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	 * these strings are invalid, but often they contain plausible data
 	 * nonetheless.  It doesn't matter if the device sent < 36 bytes
 	 * total, since scsi_probe_lun() initializes inq_result with 0s.
+	 *
+	 * Set sdev_bflags before calling scsi_update_inquiry_data() so it
+	 * can use the correct blacklist flags (especially BLIST_ISROM).
 	 */
-	sdev->inquiry = kmemdup(inq_result,
-				max_t(size_t, sdev->inquiry_len, 36),
-				GFP_KERNEL);
-	if (sdev->inquiry == NULL)
-		return SCSI_SCAN_NO_RESPONSE;
+	sdev->sdev_bflags = *bflags;
 
-	strscpy(sdev->vendor, sdev->inquiry + INQUIRY_VENDOR_OFFSET);
-	strscpy(sdev->model, sdev->inquiry + INQUIRY_MODEL_OFFSET);
 	/*
-	 * memcpy() instead of strscpy() because strscpy() would read past
-	 * the end of sdev->inquiry if its length is exactly 36 bytes.
+	 * scsi_probe_lun() already enforces a minimum of 36 bytes, so
+	 * sdev->inquiry_len is guaranteed >= 36 here.
 	 */
-	memcpy(sdev->rev, sdev->inquiry + INQUIRY_REVISION_OFFSET,
-	       INQUIRY_REVISION_LEN);
-	sdev->rev[INQUIRY_REVISION_LEN] = '\0';
-
-	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
-	if (sdev->is_ata) {
-		/*
-		 * sata emulation layer device.  This is a hack to work around
-		 * the SATL power management specifications which state that
-		 * when the SATL detects the device has gone into standby
-		 * mode, it shall respond with NOT READY.
-		 */
-		sdev->allow_restart = 1;
-	}
-
-	if (*bflags & BLIST_ISROM) {
-		sdev->type = TYPE_ROM;
-		sdev->removable = 1;
-	} else {
-		sdev->type = (inq_result[0] & 0x1f);
-		sdev->removable = (inq_result[1] & 0x80) >> 7;
-
-		/*
-		 * some devices may respond with wrong type for
-		 * well-known logical units. Force well-known type
-		 * to enumerate them correctly.
-		 */
-		if (scsi_is_wlun(sdev->lun) && sdev->type != TYPE_WLUN) {
-			sdev_printk(KERN_WARNING, sdev,
-				"%s: correcting incorrect peripheral device type 0x%x for W-LUN 0x%16xhN\n",
-				__func__, sdev->type, (unsigned int)sdev->lun);
-			sdev->type = TYPE_WLUN;
-		}
+	if (scsi_update_inquiry_data(sdev, inq_result, sdev->inquiry_len) < 0)
+		return SCSI_SCAN_NO_RESPONSE;
 
-	}
+	/*
+	 * scsi_update_inquiry_data() has already set type, removable, lockable,
+	 * inq_periph_qual, scsi_level, inquiry_len, soft_reset, ppr, wdtr, sdtr,
+	 * tagged_supported, simple_tags, is_ata, and allow_restart from INQUIRY
+	 * data. Handle special cases that need the raw inq_result or additional
+	 * logic.
+	 */
 
 	if (sdev->type == TYPE_RBC || sdev->type == TYPE_ROM) {
 		/* RBC and MMC devices can return SCSI-3 compliance and yet
@@ -960,46 +925,12 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 			*bflags |= BLIST_NOREPORTLUN;
 	}
 
-	/*
-	 * For a peripheral qualifier (PQ) value of 1 (001b), the SCSI
-	 * spec says: The device server is capable of supporting the
-	 * specified peripheral device type on this logical unit. However,
-	 * the physical device is not currently connected to this logical
-	 * unit.
-	 *
-	 * The above is vague, as it implies that we could treat 001 and
-	 * 011 the same. Stay compatible with previous code, and create a
-	 * scsi_device for a PQ of 1
-	 *
-	 * Don't set the device offline here; rather let the upper
-	 * level drivers eval the PQ to decide whether they should
-	 * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check.
-	 */ 
-
-	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
-	sdev->lockable = sdev->removable;
-	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
-
-	if (sdev->scsi_level >= SCSI_3 ||
-			(sdev->inquiry_len > 56 && inq_result[56] & 0x04))
-		sdev->ppr = 1;
-	if (inq_result[7] & 0x60)
-		sdev->wdtr = 1;
-	if (inq_result[7] & 0x10)
-		sdev->sdtr = 1;
-
 	sdev_printk(KERN_NOTICE, sdev, "%s %.8s %.16s %.4s PQ: %d "
 			"ANSI: %d%s\n", scsi_device_type(sdev->type),
 			sdev->vendor, sdev->model, sdev->rev,
 			sdev->inq_periph_qual, inq_result[2] & 0x07,
 			(inq_result[3] & 0x0f) == 1 ? " CCS" : "");
 
-	if ((sdev->scsi_level >= SCSI_2) && (inq_result[7] & 2) &&
-	    !(*bflags & BLIST_NOTQ)) {
-		sdev->tagged_supported = 1;
-		sdev->simple_tags = 1;
-	}
-
 	/*
 	 * Some devices (Texel CD ROM drives) have handshaking problems
 	 * when used with the Seagate controllers. borken is initialized
-- 
2.56.0

[PATCH v8 4/5] scsi: core: Add device reprobe support to scsi_rescan_device()

[Brian Bunker]

Update INQUIRY data on rescan and call device_reprobe() if PQ or type
changed. Critical for ALUA unavailable state handling (SPC-4 5.15.2.4.4).

If the INQUIRY data cannot be refreshed (allocation failure, or the
device does not answer INQUIRY), the failure is logged and the rescan
continues as before with the existing data, so the VPD pages and the
device handler and upper-level driver rescan methods are still updated.

The device lock is dropped around device_reprobe(), which takes it
itself. sdev remains valid in that window because callers hold a
reference; scsi_evt_thread() is covered by the cancel_work_sync() in
scsi_device_dev_release(). Taking an extra reference here instead would
be wrong: dropping the last reference from the event work would run the
release function, and its cancel_work_sync(), from that same work.

If device_reprobe() fails, the device is set offline under state_mutex,
like every other SCSI core state change, so a concurrent
__scsi_remove_device() cannot have its SDEV_CANCEL or SDEV_DEL state
overwritten with SDEV_OFFLINE.

scsi_probe_lun() is now called for live devices, so:
- Return the INQUIRY length instead of storing it in sdev->inquiry_len.
  scsi_update_inquiry_data() stores it under inquiry_mutex together
  with the data, so sysfs never pairs a new length with old data.
- Add a rescan argument. On a rescan, leave the host's short_inquiry
  flag, the target's scsi_level and lun_in_cdb alone. short_inquiry
  shares a word with runtime host state such as tmf_in_progress, and
  lun_in_cdb shares one with cdl_enable, which sysfs writes without
  the device lock, so these are only written when a device is first
  probed.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_scan.c | 140 +++++++++++++++++++++++++++++++++------
 1 file changed, 119 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 93a8719fcf26..87fd3ba5c3cc 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -635,21 +635,31 @@ EXPORT_SYMBOL(scsi_sanitize_inquiry_string);
  * @inq_result:	area to store the INQUIRY result
  * @result_len: len of inq_result
  * @bflags:	store any bflags found here
+ * @rescan:	@sdev is a live device whose INQUIRY data is being refreshed
  *
  * Description:
  *     Probe the lun associated with @req using a standard SCSI INQUIRY;
  *
- *     If the INQUIRY is successful, zero is returned and the
- *     INQUIRY data is in @inq_result; the scsi_level and INQUIRY length
- *     are copied to the scsi_device any flags value is stored in *@bflags.
+ *     If the INQUIRY is successful, the number of valid bytes of INQUIRY
+ *     data (at least 36) is returned, the INQUIRY data is in @inq_result
+ *     and any flags value is stored in *@bflags. The INQUIRY length is not
+ *     stored in @sdev: this function is also called for live devices, and
+ *     scsi_update_inquiry_data() updates it together with the data.
+ *     A negative errno is returned on failure.
+ *
+ *     When @rescan is true, the host's short_inquiry flag, the target's
+ *     scsi_level and lun_in_cdb are left alone. They are bitfields or
+ *     share a word with bitfields that are written at runtime without a
+ *     common lock, so they are only set when a device is first probed.
  **/
 static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
-			  int result_len, blist_flags_t *bflags)
+			  int result_len, blist_flags_t *bflags, bool rescan)
 {
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
 	int first_inquiry_len, try_inquiry_len, next_inquiry_len;
 	int response_len = 0;
 	int pass, count, result, resid;
+	int inquiry_len;
 	char scsi_level;
 	struct scsi_failure failure_defs[] = {
 		/*
@@ -798,7 +808,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		return -EIO;
 
 	/* Don't report any more data than the device says is valid */
-	sdev->inquiry_len = min(try_inquiry_len, response_len);
+	inquiry_len = min(try_inquiry_len, response_len);
 
 	/*
 	 * XXX Abort if the response length is less than 36? If less than
@@ -815,14 +825,14 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	 * and displaying garbage for the Vendor, Product, or Revision
 	 * strings.
 	 */
-	if (sdev->inquiry_len < 36) {
-		if (!sdev->host->short_inquiry) {
+	if (inquiry_len < 36) {
+		if (!rescan && !sdev->host->short_inquiry) {
 			shost_printk(KERN_INFO, sdev->host,
 				    "scsi scan: INQUIRY result too short (%d),"
-				    " using 36\n", sdev->inquiry_len);
+				    " using 36\n", inquiry_len);
 			sdev->host->short_inquiry = 1;
 		}
-		sdev->inquiry_len = 36;
+		inquiry_len = 36;
 	}
 
 	/*
@@ -850,19 +860,22 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (scsi_level >= 2 ||
 	    (scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
 		scsi_level++;
-	sdev->sdev_target->scsi_level = scsi_level;
 
-	/*
-	 * If SCSI-2 or lower, and if the transport requires it,
-	 * store the LUN value in CDB[1].
-	 */
-	sdev->lun_in_cdb = 0;
-	if (scsi_level <= SCSI_2 &&
-	    scsi_level != SCSI_UNKNOWN &&
-	    !sdev->host->no_scsi2_lun_in_cdb)
-		sdev->lun_in_cdb = 1;
+	if (!rescan) {
+		sdev->sdev_target->scsi_level = scsi_level;
 
-	return 0;
+		/*
+		 * If SCSI-2 or lower, and if the transport requires it,
+		 * store the LUN value in CDB[1].
+		 */
+		sdev->lun_in_cdb = 0;
+		if (scsi_level <= SCSI_2 &&
+		    scsi_level != SCSI_UNKNOWN &&
+		    !sdev->host->no_scsi2_lun_in_cdb)
+			sdev->lun_in_cdb = 1;
+	}
+
+	return inquiry_len;
 }
 
 /**
@@ -1131,6 +1144,7 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
 	unsigned char *result;
 	blist_flags_t bflags;
 	int res = SCSI_SCAN_NO_RESPONSE, result_len = 256;
+	int ret;
 
 	/*
 	 * The rescan flag is used as an optimization, the first scan of a
@@ -1169,8 +1183,10 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
 	if (!result)
 		goto out_free_sdev;
 
-	if (scsi_probe_lun(sdev, result, result_len, &bflags))
+	ret = scsi_probe_lun(sdev, result, result_len, &bflags, false);
+	if (ret < 0)
 		goto out_free_result;
+	sdev->inquiry_len = ret;
 
 	if (bflagsp)
 		*bflagsp = bflags;
@@ -1658,9 +1674,54 @@ int scsi_resume_device(struct scsi_device *sdev)
 }
 EXPORT_SYMBOL(scsi_resume_device);
 
+/**
+ * scsi_refresh_inquiry - Fetch fresh standard INQUIRY data for a device
+ * @sdev: The SCSI device to refresh
+ *
+ * Failures are logged and otherwise ignored, so that the caller carries on
+ * with the rest of the rescan using the INQUIRY data it already has.
+ *
+ * Returns true if the peripheral qualifier or device type changed and the
+ * device needs to be reprobed.
+ */
+static bool scsi_refresh_inquiry(struct scsi_device *sdev)
+{
+	unsigned char *inq_result;
+	blist_flags_t bflags;
+	int ret = -ENOMEM;
+
+	inq_result = kmalloc(256, GFP_KERNEL);
+	if (inq_result) {
+		ret = scsi_probe_lun(sdev, inq_result, 256, &bflags, true);
+		if (ret >= 0)
+			ret = scsi_update_inquiry_data(sdev, inq_result, ret);
+		kfree(inq_result);
+	}
+
+	if (ret < 0) {
+		sdev_printk(KERN_WARNING, sdev,
+			    "failed to refresh INQUIRY data: %d\n", ret);
+		return false;
+	}
+
+	return ret == SCSI_INQ_REPROBE_NEEDED;
+}
+
+/**
+ * scsi_rescan_device - Rescan a SCSI device
+ * @sdev: The SCSI device to rescan
+ *
+ * Refreshes the INQUIRY data and VPD pages of @sdev and lets the device
+ * handler and the upper-level driver update their view of the device. If
+ * the peripheral qualifier or device type changed, the device is reprobed
+ * so that the right upper-level driver, if any, is bound to it.
+ *
+ * The caller must hold a reference to @sdev.
+ */
 int scsi_rescan_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	bool need_reprobe;
 	int ret = 0;
 
 	device_lock(dev);
@@ -1677,12 +1738,49 @@ int scsi_rescan_device(struct scsi_device *sdev)
 		goto unlock;
 	}
 
+	need_reprobe = scsi_refresh_inquiry(sdev);
+
 	scsi_attach_vpd(sdev);
 	scsi_cdl_check(sdev);
 
 	if (sdev->handler && sdev->handler->rescan)
 		sdev->handler->rescan(sdev);
 
+	if (need_reprobe) {
+		/*
+		 * The peripheral qualifier or device type changed, so the
+		 * driver binding has to be re-evaluated. device_reprobe()
+		 * takes the device lock itself. sdev stays valid while the
+		 * lock is dropped because the caller holds a reference;
+		 * scsi_evt_thread() is covered by the cancel_work_sync() in
+		 * scsi_device_dev_release().
+		 */
+		device_unlock(dev);
+		ret = device_reprobe(dev);
+		device_lock(dev);
+
+		/*
+		 * state_mutex serializes against __scsi_remove_device(), so
+		 * a device that is being removed cannot be moved from
+		 * SDEV_CANCEL or SDEV_DEL to SDEV_OFFLINE.
+		 */
+		mutex_lock(&sdev->state_mutex);
+		if (sdev->sdev_state == SDEV_CANCEL ||
+		    sdev->sdev_state == SDEV_DEL) {
+			ret = -ENODEV;
+		} else if (ret < 0) {
+			if (scsi_device_set_state(sdev, SDEV_OFFLINE) == 0)
+				sdev_printk(KERN_WARNING, sdev,
+					    "device reprobe failed (%d), marked offline\n",
+					    ret);
+			else
+				sdev_printk(KERN_WARNING, sdev,
+					    "device reprobe failed (%d)\n", ret);
+		}
+		mutex_unlock(&sdev->state_mutex);
+		goto unlock;
+	}
+
 	if (dev->driver && try_module_get(dev->driver->owner)) {
 		struct scsi_driver *drv = to_scsi_driver(dev->driver);
 
-- 
2.56.0

[PATCH v8 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan

[Brian Bunker]

Complement scsi_rescan_device() reprobe by handling the scan path.
When a manual or transport-initiated scan finds a LUN that already has
a fully set up scsi_device, call scsi_rescan_device() for it so that
changes to the standard INQUIRY data are picked up and the device is
reprobed if its peripheral qualifier or device type changed.

The device lock is taken inside scsi_rescan_device() while the host's
scan_mutex is held; that lock order is already used by
scsi_remove_device(). Pseudo devices and devices that are still being
set up by another scan are left alone.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_scan.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 87fd3ba5c3cc..c6424773c3f1 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1156,6 +1156,16 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
 			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
 				"scsi scan: device exists on %s\n",
 				dev_name(&sdev->sdev_gendev)));
+			/*
+			 * Rescanning a device that has been fully set up:
+			 * refresh its INQUIRY data so that peripheral
+			 * qualifier or device type changes (e.g. ALUA state
+			 * transitions) are picked up.
+			 */
+			if (rescan != SCSI_SCAN_INITIAL &&
+			    !scsi_device_created(sdev) &&
+			    !scsi_device_is_pseudo_dev(sdev))
+				scsi_rescan_device(sdev);
 			if (sdevp)
 				*sdevp = sdev;
 			else
-- 
2.56.0

Re: [PATCH v8 4/5] scsi: core: Add device reprobe support to scsi_rescan_device()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: core: broken device_reprobe() error check in scsi_rescan_device()
- [High] scsi: core: NULL deref in scsi_rescan_device() before device_add()
- [Medium] scsi: core: scsi_probe_lun() misuses sdev->inquiry_len on rescan
--

commit 09090dd4acc8a64408156c003c7bd95cb8781601
Author: Brian Bunker <brian@purestorage.com>

scsi: core: Add device reprobe support to scsi_rescan_device()

This commit adds INQUIRY data refresh and device reprobe support to
scsi_rescan_device() when the peripheral qualifier or device type changes.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 93a8719fcf265..87fd3ba5c3cc0 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c

[ ... ]

> @@ -1658,9 +1674,54 @@ int scsi_resume_device(struct scsi_device *sdev)
>  }
>  EXPORT_SYMBOL(scsi_resume_device);
>  
> +/**
> + * scsi_refresh_inquiry - Fetch fresh standard INQUIRY data for a device
> + * @sdev: The SCSI device to refresh
> + *
> + * Failures are logged and otherwise ignored, so that the caller carries on
> + * with the rest of the rescan using the INQUIRY data it already has.
> + *
> + * Returns true if the peripheral qualifier or device type changed and the
> + * device needs to be reprobed.
> + */
> +static bool scsi_refresh_inquiry(struct scsi_device *sdev)
> +{
> +	unsigned char *inq_result;
> +	blist_flags_t bflags;
> +	int ret = -ENOMEM;
> +
> +	inq_result = kmalloc(256, GFP_KERNEL);
> +	if (inq_result) {
> +		ret = scsi_probe_lun(sdev, inq_result, 256, &bflags, true);

[Severity: Medium]
Does calling scsi_probe_lun() on a live sdev cause it to misinterpret
sdev->inquiry_len?

During initial probing in scsi_probe_and_add_lun(), sdev->inquiry_len is 0
unless shost->hostt->sdev_init() configured a maximum INQUIRY transfer
length. Once probed, scsi_probe_and_add_lun() and scsi_update_inquiry_data()
overwrite sdev->inquiry_len with the valid INQUIRY response length (at least
36 bytes).

When scsi_refresh_inquiry() calls scsi_probe_lun() on a live device,
sdev->inquiry_len holds the previous response length rather than 0 or a
low-level driver limit. In scsi_probe_lun():

	first_inquiry_len = sdev->inquiry_len ? sdev->inquiry_len : 36;
	try_inquiry_len = first_inquiry_len;
	pass = 1;
	...
		if (pass == 1) {
			if (BLIST_INQUIRY_36 & *bflags)
				next_inquiry_len = 36;
			else if (sdev->inquiry_len &&
				 response_len > sdev->inquiry_len &&
				 (inq_result[2] & 0x7) < 6) /* SPC-4 */
				next_inquiry_len = sdev->inquiry_len;

If a pre-SPC-4 device initially returned a short INQUIRY (for example, 36
bytes while in PQ=1 / ALUA unavailable state) and later reports a larger
response_len on rescan, sdev->inquiry_len is non-zero, so next_inquiry_len is
capped at the old sdev->inquiry_len and pass 2 is skipped.

Conversely, if the device previously returned more than 36 bytes, pass 1
starts at that larger sdev->inquiry_len instead of 36 bytes, and if that
transfer fails after a state change, scsi_probe_lun() returns -EIO without
falling back to a 36-byte inquiry.

> +		if (ret >= 0)
> +			ret = scsi_update_inquiry_data(sdev, inq_result, ret);
> +		kfree(inq_result);
> +	}
> +
> +	if (ret < 0) {
> +		sdev_printk(KERN_WARNING, sdev,
> +			    "failed to refresh INQUIRY data: %d\n", ret);
> +		return false;
> +	}
> +
> +	return ret == SCSI_INQ_REPROBE_NEEDED;
> +}
> +
> +/**
> + * scsi_rescan_device - Rescan a SCSI device
> + * @sdev: The SCSI device to rescan
> + *
> + * Refreshes the INQUIRY data and VPD pages of @sdev and lets the device
> + * handler and the upper-level driver update their view of the device. If
> + * the peripheral qualifier or device type changed, the device is reprobed
> + * so that the right upper-level driver, if any, is bound to it.
> + *
> + * The caller must hold a reference to @sdev.
> + */
>  int scsi_rescan_device(struct scsi_device *sdev)
>  {
>  	struct device *dev = &sdev->sdev_gendev;
> +	bool need_reprobe;
>  	int ret = 0;
>  
>  	device_lock(dev);
> @@ -1677,12 +1738,49 @@ int scsi_rescan_device(struct scsi_device *sdev)
>  		goto unlock;
>  	}
>  
> +	need_reprobe = scsi_refresh_inquiry(sdev);
> +
>  	scsi_attach_vpd(sdev);
>  	scsi_cdl_check(sdev);
>  
>  	if (sdev->handler && sdev->handler->rescan)
>  		sdev->handler->rescan(sdev);
>  
> +	if (need_reprobe) {
> +		/*
> +		 * The peripheral qualifier or device type changed, so the
> +		 * driver binding has to be re-evaluated. device_reprobe()
> +		 * takes the device lock itself. sdev stays valid while the
> +		 * lock is dropped because the caller holds a reference;
> +		 * scsi_evt_thread() is covered by the cancel_work_sync() in
> +		 * scsi_device_dev_release().
> +		 */
> +		device_unlock(dev);
> +		ret = device_reprobe(dev);
> +		device_lock(dev);

[Severity: High]
Can device_reprobe(dev) be called here before device_add(dev) has initialized
dev->p, leading to a NULL pointer dereference in __device_attach()?

In scsi_add_lun(), sdev->sdev_state is transitioned to SDEV_RUNNING before
sdev_configure(), scsi_attach_vpd(), scsi_cdl_check(), and
scsi_sysfs_add_sdev() run (and during async scans, scsi_sysfs_add_sdev() is
deferred until scsi_finish_async_scan()):

drivers/scsi/scsi_scan.c:scsi_add_lun() {
	...
	mutex_lock(&sdev->state_mutex);
	ret = scsi_device_set_state(sdev, SDEV_RUNNING);
	if (ret)
		ret = scsi_device_set_state(sdev, SDEV_BLOCK);
	mutex_unlock(&sdev->state_mutex);
	...
	if (!async && scsi_sysfs_add_sdev(sdev) != 0)
		return SCSI_SCAN_NO_RESPONSE;
}

If a command issued during this window receives a UNIT_ATTENTION with
INQUIRY_DATA_HAS_CHANGED (scheduling sdev->event_work -> scsi_evt_thread() ->
scsi_rescan_device()), or a caller invokes scsi_rescan_device() while sdev is
in SDEV_RUNNING before device_add() has been called, scsi_rescan_device()
only checks sdev->sdev_state == SDEV_RUNNING without checking
device_is_registered(dev) or sdev->is_visible.

When need_reprobe is true, device_reprobe() -> bus_rescan_devices_helper() ->
device_attach() -> __device_attach() dereferences dev->p->dead while dev->p
is still NULL.

> +
> +		/*
> +		 * state_mutex serializes against __scsi_remove_device(), so
> +		 * a device that is being removed cannot be moved from
> +		 * SDEV_CANCEL or SDEV_DEL to SDEV_OFFLINE.
> +		 */
> +		mutex_lock(&sdev->state_mutex);
> +		if (sdev->sdev_state == SDEV_CANCEL ||
> +		    sdev->sdev_state == SDEV_DEL) {
> +			ret = -ENODEV;
> +		} else if (ret < 0) {
> +			if (scsi_device_set_state(sdev, SDEV_OFFLINE) == 0)

[Severity: High]
Will checking ret < 0 here ever catch upper-level driver probe failures from
device_reprobe()?

Following the call chain device_reprobe() -> bus_rescan_devices_helper() ->
device_attach() -> __device_attach() -> __device_attach_driver() ->
driver_probe_device() -> __driver_probe_device() -> really_probe():

drivers/base/dd.c:really_probe() {
	...
	ret = call_driver_probe(dev, drv);
	if (ret) {
		if (link_ret == -EAGAIN)
			ret = -EPROBE_DEFER;

		/*
		 * Return probe errors as positive values so that the callers
		 * can distinguish them from other errors.
		 */
		ret = -ret;
		goto probe_failed;
	}
}

And in __device_attach_driver():

drivers/base/dd.c:__device_attach_driver() {
	...
	ret = driver_probe_device(drv, dev);
	if (ret < 0)
		return ret;
	return ret == 0;
}

Because really_probe() negates driver probe errors into positive values and
__device_attach_driver() converts positive returns into 0, device_reprobe()
returns 0 when a SCSI driver's probe callback fails, leaving the device in
SDEV_RUNNING without a driver instead of marking it SDEV_OFFLINE.

Conversely, when probing is deferred (such as when defer_all_probes is set
during system suspend/hibernation), really_probe() returns -EPROBE_DEFER
(negative) and queues dev on deferred_probe_pending_list, which causes the
ret < 0 check to mark the device SDEV_OFFLINE while its probe is deferred.

> +				sdev_printk(KERN_WARNING, sdev,
> +					    "device reprobe failed (%d), marked offline\n",
> +					    ret);
> +			else
> +				sdev_printk(KERN_WARNING, sdev,
> +					    "device reprobe failed (%d)\n", ret);
> +		}
> +		mutex_unlock(&sdev->state_mutex);
> +		goto unlock;
> +	}

[ ... ]

>
> 

完美结束。

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019221405.40614-1-brian@purestorage.com?part=4

Re: [PATCH v8 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: core: premature rescan of unregistered sdev in scsi_probe_and_add_lun()
- [Medium] scsi: core: redundant LUN 0 rescan in scsi_probe_and_add_lun()
--

commit 4955c24621465fbad93e330007f0c5daeb098941
Author: Brian Bunker <brian@purestorage.com>

scsi: core: Handle reprobe for existing devices during SCSI scan

This commit updates scsi_probe_and_add_lun() to call scsi_rescan_device()
when a non-initial scan encounters an existing scsi_device that has
finished creation, refreshing its INQUIRY data and reprobing if its
peripheral qualifier or device type changed.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 87fd3ba5c3cc0..c6424773c3f1c 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1156,6 +1156,16 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
>  			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
>  				"scsi scan: device exists on %s\n",
>  				dev_name(&sdev->sdev_gendev)));
> +			/*
> +			 * Rescanning a device that has been fully set up:
> +			 * refresh its INQUIRY data so that peripheral
> +			 * qualifier or device type changes (e.g. ALUA state
> +			 * transitions) are picked up.
> +			 */
> +			if (rescan != SCSI_SCAN_INITIAL &&
> +			    !scsi_device_created(sdev) &&
> +			    !scsi_device_is_pseudo_dev(sdev))
> +				scsi_rescan_device(sdev);

[Severity: High]
Can this call scsi_rescan_device() on an sdev before scsi_sysfs_add_sdev()
has registered sdev->sdev_gendev with the driver core?

During an asynchronous host scan (shost->async_scan == 1, set by
scsi_prep_async_scan()), scsi_add_lun() transitions sdev->sdev_state out of
SDEV_CREATED/SDEV_CREATED_BLOCK before scsi_sysfs_add_sdev() is called, and
defers scsi_sysfs_add_sdev() until scsi_finish_async_scan() ->
scsi_sysfs_add_devices():

drivers/scsi/scsi_scan.c:scsi_add_lun() {
	...
	mutex_lock(&sdev->state_mutex);
	ret = scsi_device_set_state(sdev, SDEV_RUNNING);
	if (ret)
		ret = scsi_device_set_state(sdev, SDEV_BLOCK);
	mutex_unlock(&sdev->state_mutex);
	...
	if (!async && scsi_sysfs_add_sdev(sdev) != 0)
		return SCSI_SCAN_NO_RESPONSE;
}

In that window, !scsi_device_created(sdev) is already true, while
sdev->is_visible is still 0 and sdev->sdev_gendev.p is NULL (since
device_private_init() only runs inside device_add()).

For example, when fc_scsi_scan_rport() runs while shost->async_scan is set,
it calls scsi_scan_target(..., SCAN_WILD_CARD, SCSI_SCAN_RESCAN).
__scsi_scan_target() first probes and adds LUN 0 via
scsi_probe_and_add_lun() (setting sdev_state to SDEV_RUNNING with
is_visible == 0), and then immediately calls scsi_report_lun_scan(...,
SCSI_SCAN_RESCAN), which calls scsi_probe_and_add_lun() on LUN 0 a second
time. A concurrent scsi_scan_target(), scsi_scan_host_selected(), or
__scsi_add_device() call with rescan != SCSI_SCAN_INITIAL can also acquire
shost->scan_mutex while shost->async_scan is set without waiting in
scsi_complete_async_scans().

If scsi_rescan_device() is called on this unregistered sdev and
scsi_refresh_inquiry() returns true (need_reprobe), the call chain:

  scsi_rescan_device()
    -> device_reprobe()
      -> bus_rescan_devices_helper()
        -> device_attach()
          -> __device_attach()

dereferences dev->p->dead in __device_attach() while dev->p is still NULL.
Should this check sdev->is_visible as well?

[Severity: Medium]
Also, during a wildcard target scan in __scsi_scan_target() with
rescan != SCSI_SCAN_INITIAL, won't LUN 0 be rescanned twice (or rescanned
immediately after initial creation)?

In __scsi_scan_target(), LUN 0 is probed first and then
scsi_report_lun_scan() is called right after with the same rescan value:

drivers/scsi/scsi_scan.c:__scsi_scan_target() {
	...
	res = scsi_probe_and_add_lun(shost, starget, 0, &bflags, NULL, rescan,
				     NULL);
	if (res == SCSI_SCAN_LUN_PRESENT || res == SCSI_SCAN_TARGET_PRESENT) {
		if (scsi_report_lun_scan(shost, starget, bflags, rescan) != 0)
			scsi_sequential_lun_scan(shost, starget, bflags,
						 starget->scsi_level, rescan);
	}
}

Unlike scsi_sequential_lun_scan(), which starts at lun = 1,
scsi_report_lun_scan() iterates over all LUNs returned by REPORT LUNS
including LUN 0 and calls scsi_probe_and_add_lun(shost, starget, lun, NULL,
NULL, rescan, NULL) again:

drivers/scsi/scsi_scan.c:scsi_report_lun_scan() {
	...
	for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
		lun = scsilun_to_int(lunp);
		...
			res = scsi_probe_and_add_lun(shost, starget,
				lun, NULL, NULL, rescan, NULL);
	}
}

When LUN 0 already exists, __scsi_scan_target() rescans LUN 0 once and
scsi_report_lun_scan() immediately rescans LUN 0 a second time.

When LUN 0 is newly discovered during a scan where rescan !=
SCSI_SCAN_INITIAL (such as manual sysfs scans or transport scans like
fc_scsi_scan_rport(), iscsi_scan_session(), and snic_scsi_scan_tgt(), which
always pass SCSI_SCAN_RESCAN), the first scsi_probe_and_add_lun() call in
__scsi_scan_target() adds LUN 0 and sets sdev->sdev_state to SDEV_RUNNING.
Moments later, scsi_report_lun_scan() visits LUN 0 again, sees
!scsi_device_created(sdev), and calls scsi_rescan_device() on the newly
added LUN 0 (which also blocks on device_lock(&sdev->sdev_gendev) waiting
for asynchronous sd_probe() to finish while holding shost->scan_mutex).

>  			if (sdevp)
>  				*sdevp = sdev;
>  			else

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019221405.40614-1-brian@purestorage.com?part=5

Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan

[Bart Van Assche]

On 10/19/26 3:13 PM, Brian Bunker wrote:
> This series teaches the SCSI rescan path to refetch standard INQUIRY data
> and reprobe the device when the peripheral qualifier or device type has
> changed.  The motivating case is an ALUA target that transitions through
> the "unavailable" state and afterwards reports a different peripheral
> qualifier; today the kernel keeps the stale INQUIRY data and the device's
> sysfs attributes diverge from what the target reports.

Some feedback from an experimental LLM can be found below. Please take a
look.

Thanks,

Bart.
------------------------------------------------------------------------
I reviewed the v8 series and wrote it up in 
review-v8-inquiry-reprobe.md. 4/5 and 5/5 have real bugs, and the design 
has one open question for you. The series applies cleanly to 
db8b4d2bf2d3, and each patch built with no W=1 warnings for scsi.o, 
scsi_scan.o and scsi_sysfs.o. checkpatch --strict only reported nits.

The open question: a target can now remove a disk that is bound and in 
use. scsi_bus_match() only matches devices whose peripheral qualifier 
(PQ) is 0. So if a LUN changes PQ from 0 to 1, device_reprobe() detaches 
sd and the disk disappears. That can be triggered by a unit attention, a 
sysfs rescan, or (after 5/5) any scan. My suggestion is to only reprobe 
when no driver is bound (PQ 1 → 0), but the author should say which 
direction the ALUA case actually needs.

Bugs in 4/5:

device_reprobe() error check: it returns 0 when sd_probe() fails, and a 
negative value for -EPROBE_DEFER. So the check misses real failures and 
marks the device offline when probing is merely deferred.
Offlining can't be undone: scsi_rescan_device() gives up unless the 
device is SDEV_RUNNING, so an offlined device never comes back through a 
later rescan. I'd drop the offline step.
sdev->inquiry_len changes meaning: on a live device it holds the 
previous response length, not the LLD limit, which breaks 
scsi_probe_lun()'s retry passes.
NULL dev->p dereference: possible when a reprobe happens before 
scsi_sysfs_add_sdev() has run, e.g. during async scans. 4/5 and 5/5 
should check is_visible.

Problems in 5/5:

Every scan now does a full rescan of every existing LUN while holding 
scan_mutex. One hung LUN stalls the whole scan.
LUN 0 gets rescanned twice per scan, or right after it is created.

Other points:

In 1/5, cdl_supported doesn't come from INQUIRY, so the mutex doesn't 
protect it.
2/5 and 3/5 should be merged into one patch, and 
scsi_update_inquiry_data() should stay internal to the SCSI core rather 
than go in the public scsi_device.h.
3/5 deletes a still-useful comment explaining why PQ=1 devices are created.
The rescan path skips the scan path's checks for PQ=3 and PQ=1/PDT=0x1f 
("no LUN" responses).
A change of vendor or model (a different LU behind the same LUN) is 
accepted silently, leaving the device flags and handler stale.

Sashiko had already flagged the device_reprobe() error check, 
inquiry_len, the NULL dereference and the double LUN 0 rescan. I checked 
each against the code and they hold up. The review file says which 
findings came from it.
------------------------------------------------------------------------
# Review: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan

Series: 
<https://lore.kernel.org/linux-scsi/20261019221405.40614-1-brian@purestorage.com/>
Base: `db8b4d2bf2d3` (7.4/scsi-staging). Applies cleanly.

**Build/checkpatch:** each patch builds without warnings with gcc W=1 
(`scsi.o`, `scsi_scan.o`, `scsi_sysfs.o`, x86_64 defconfig). `checkpatch 
--strict` reports only nits (1/5: `S_IRUGO`; 2/5: parenthesis alignment 
at `scsi.c:736`; 4/5: quoted string split across lines, which was 
already there).

Sashiko has already replied to 4/5 and 5/5. I checked its findings 
against the code and note below which ones hold up.

---

## Design-level concerns (cover letter / 4/5 / 5/5)

1. **A target can now unbind sd from a live disk.** `scsi_bus_match()` 
only matches `inq_periph_qual == 0`. If a bound LUN changes PQ from 0 to 
1, `device_reprobe()` detaches sd and the gendisk goes away. The trigger 
can be a UNIT ATTENTION "INQUIRY DATA HAS CHANGED" 
(`scsi_evt_thread()`), a sysfs `rescan`, or, after 5/5, any host or 
transport scan. With dm-multipath that means a path is torn down for 
something that may be a short ALUA transition. Without multipath, a 
mounted filesystem loses its block device. When PQ returns to 0, the 
disk comes back under a new `sdX` name, and only if someone rescans again.
    - Please describe the exact direction the motivating case needs. My 
guess is a LUN found while PQ=1 that later reports PQ=0, so no ULD is 
bound yet. If so, only reprobe when `!dev->driver` (or only for PQ 1 → 
0) and leave bound devices alone.
    - Cite the SPC text that requires a PQ change across the ALUA 
unavailable state. SPC-4 5.15.2.4.4 (cited in 4/5) describes which 
commands an unavailable LU must support. It isn't obvious that it 
requires the PQ to change.

2. **5/5 widens rescan behaviour a lot.** Today a scan (`echo - - - > 
scan`, FC RSCN → `fc_scsi_scan_rport()`, iSCSI session rescan, ...) does 
nothing for LUNs that already exist. After 5/5, every existing LUN gets 
a full `scsi_rescan_device()`: an INQUIRY of up to 3 passes × 3 retries, 
VPD re-read, CDL check, `handler->rescan` and `sd_rescan()` → 
revalidate. All of this runs with `shost->scan_mutex` held. On hosts 
with many LUNs this costs a lot. One unresponsive LUN now stalls the 
whole scan, and every other scanner waits on `scan_mutex`. The commit 
message should say this explicitly. Better still, limit the scan path to 
the INQUIRY refresh / PQ check.

3. **The rescan path doesn't apply the scan path's "no LUN" filters.** 
`scsi_probe_and_add_lun()` drops PQ=3 and PQ=1/PDT=0x1f responses. The 
refresh path accepts them as-is: `sdev->type = 0x1f`, `inq_periph_qual = 
3`, the ULD is unbound, and the `scsi_device` lives on in a state the 
scan path would never create. Decide how these responses should be 
handled, for example leave them to the REPORTED LUNS DATA HAS CHANGED / 
removal logic.

4. **Identity changes are silently accepted.** If vendor/model change, a 
different LU now sits behind this LUN. `sdev_bflags`, the device handler 
(attached once in `scsi_sysfs_add_sdev()`), TPGS-based decisions, etc. 
are all stale. `scsi_probe_lun()` computes fresh `bflags` on a rescan 
and then discards them. Either reject or warn on a vendor/model change, 
or don't refresh these strings at all.

---

## 1/5 scsi: core: Protect INQUIRY sysfs attributes with mutex

- `cdl_supported` is not derived from INQUIRY. `scsi_cdl_check()` sets 
it from RSOC data without `inquiry_mutex`, so the lock protects nothing 
for it. The commit message also lists it as INQUIRY-derived.
- The `vendor`/`model`/`rev` strings and `sdev->inquiry` are also read 
without a lock by in-kernel users (`scsi_transport_spi.c:217`, 
`scsi_dh_emc.c:355`, `mpt3sas`, `sdev_printk` callers, the 
`scsi_device_tpgs()`-style helpers). The mutex only makes sysfs 
consistent. Please don't claim more than that, and consider 
`data_race()` annotations or a note about KCSAN.
- `sdev->type` is written with `WRITE_ONCE()` (2/5) but read with 
`READ_ONCE()` only in `scsi_bus_uevent()`. The many plain readers (sd, 
st, sr, `scsi_lib`) are still unannotated.
- Moving `cdl_supported` to a different position in the list is 
unrelated churn. Nit: use `0444` instead of `S_IRUGO` in the new macro body.

## 2/5 scsi: core: Add scsi_update_inquiry_data()

- **Placement:** this is SCSI-core-internal and only `scsi_scan.c` calls 
it, yet it lives in `scsi.c` and is declared, together with `enum 
scsi_inq_update_result`, in the public `include/scsi/scsi_device.h` 
without an export. Move it to `scsi_scan.c` (static) or at least declare 
it in `scsi_priv.h`.
- **Patch split:** 2/5 adds a copy of the `scsi_add_lun()` logic with no 
caller, and 3/5 deletes the original. Merging 2/5 and 3/5 into one patch 
would make the move reviewable as a move.
- "First call" is inferred from `refresh = !new_inquiry`, i.e. from 
whether a buffer was allocated. An explicit `bool initial` argument 
would be clearer and less fragile.
- `int` return mixes negative errnos with enum values. That works, but 
returning the enum and passing errors back separately, or using `bool 
*reprobe`, would be cleaner.
- `memcpy()` into the live `sdev->inquiry` while other code reads it 
without a lock (see 1/5) is a data race. It's byte-granular, so probably 
harmless, but it should be documented or annotated.
- On a refresh, `sdev->scsi_level` is updated but `starget->scsi_level` 
and `lun_in_cdb` deliberately aren't (4/5). After a level change they 
disagree. Consider not refreshing `scsi_level` either.
- Nit: with a zeroed 256-byte buffer, the 
`memcpy()`-instead-of-`strscpy()` comment for `rev` is no longer 
accurate. The `__func__` in the W-LUN warning now prints 
`scsi_update_inquiry_data` instead of `scsi_add_lun`.

## 3/5 scsi: core: Refactor scsi_add_lun()

- This deletes the comment explaining why a `scsi_device` is still 
created for PQ=1 and why the ULD decides whether to attach. That 
rationale still applies and becomes more relevant with this series, so 
please keep it next to the PQ handling.
- "This patch maintains identical behavior": not quite. `sdev->inquiry` 
is now 256 bytes instead of `max(inquiry_len, 36)`, and 
`sdev->scsi_level` stays 0 between `scsi_probe_lun()` and 
`scsi_add_lun()`. Both are harmless, but the claim should be softened.

## 4/5 scsi: core: Add device reprobe support to scsi_rescan_device()

- **The `device_reprobe()` error check is broken (Sashiko is right).** 
`really_probe()` turns probe errors into positive values, and 
`bus_rescan_devices_helper()` returns `ret < 0 ? ret : 0`. So a failing 
`sd_probe()` gives 0 and the device stays RUNNING without a driver. 
Meanwhile `-EPROBE_DEFER` (e.g. `defer_all_probes` during suspend) gives 
a negative value and marks the device offline.
- **Offlining on reprobe failure can't be undone.** 
`scsi_rescan_device()` bails out with `-EWOULDBLOCK` unless the device 
is `SDEV_RUNNING`, so an offlined device can never recover through a 
later rescan. Not finding or binding a ULD is not an I/O error. I'd drop 
the `SDEV_OFFLINE` transition completely and just log.
- **`sdev->inquiry_len` changes meaning (Sashiko is right).** In 
`scsi_probe_lun()`, `sdev->inquiry_len` is the LLD-provided maximum / 
first-pass length. On a live device it now holds the previous response 
length. Pass 1 then starts at that length, with no fallback to 36 if it 
fails, and pass 2 is capped at the old length. Keep the LLD limit in a 
separate field, or pass the initial length in explicitly.
- **NULL `dev->p` dereference before `device_add()`.** `scsi_add_lun()` 
sets `SDEV_RUNNING` before `scsi_sysfs_add_sdev()`, and async scans 
defer that call. A rescan in that window, from a UA event or from 5/5, 
that decides to reprobe calls `__device_attach()` with `dev->p == NULL`. 
Check `sdev->is_visible` / `device_is_registered()` before reprobing.
- The device lock is dropped around `device_reprobe()`, so the state may 
change in between. Anything assumed before the unlock should be 
rechecked afterwards (the `CANCEL`/`DEL` check partly does this).
- Nit: `kmalloc(256, ...)` / `256` are magic numbers. Share a constant 
with 2/5.
- Every existing `scsi_rescan_device()` caller (libata resume, 
virtio_scsi, storvsc, smartpqi, aacraid, mvumi, the UA event) now sends 
an extra INQUIRY. Fragile USB/ATA bridges have a history of disliking 
extra INQUIRYs, so call this out in the commit message.

## 5/5 scsi: core: Handle reprobe for existing devices during SCSI scan

- `!scsi_device_created(sdev)` doesn't imply the device is registered. 
During async scans, `scsi_sysfs_add_sdev()` is deferred (same issue as 
above). Add an `sdev->is_visible` check.
- `__scsi_scan_target()` probes LUN 0 and then `scsi_report_lun_scan()` 
visits LUN 0 again with the same `rescan` value. A newly added LUN 0 is 
therefore "rescanned" right after creation, which blocks on 
`device_lock()` while async `sd_probe()` runs with `scan_mutex` held. An 
existing LUN 0 is rescanned twice per scan.
- See design concern 2 on the cost and `scan_mutex` hold time.

---

## Summary

1/5 is close, apart from the `cdl_supported` and scope points. 2/5 and 
3/5 should be merged and kept inside the SCSI core. 4/5 and 5/5 have 
real correctness bugs: the reprobe error handling, the irreversible 
offlining, the changed meaning of `inquiry_len`, and a NULL `dev->p` 
dereference during async scans. The bigger open question is whether 
unbinding sd from a bound, in-use disk because the target says so is 
acceptable at all. I'd ask for it to be limited to binding a ULD when 
none is bound.

Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan

[Brian Bunker]

On 10/19/26 3:41 PM, Bart Van Assche wrote:
> Some feedback from an experimental LLM can be found below. Please take a
> look.

Hi Bart,

Thanks for the detailed review. Before I post v9, I want to answer the
design question so that v9 isn't heading in a direction you and Martin
don't want.

> The open question: a target can now remove a disk that is bound and in
> use. [...] My suggestion is to only reprobe when no driver is bound
> (PQ 1 -> 0), but the author should say which direction the ALUA case
> actually needs.

Both directions. Let me describe the setup that motivates this, because
it isn't a short interruption.

Pure ActiveCluster is two separate arrays that present the same volume.
The volume is synchronously replicated between them, and each array
exposes it through its own front-end ports, which are in their own
target port groups. A host sees paths to both arrays as one multipath
device.

When the replication link between the arrays breaks, one array keeps
serving the volume and the other must stop, because it can no longer
mirror writes or persistent reservation changes to its peer. Its
front-end ports stay up, but its paths must not serve I/O. The array
reports those target port groups as unavailable. Unavailable is the
state we can use here, because its required command set is one we can
fully support without the link: we don't have to accept reservations
we cannot mirror. This lasts as long as the link is down, which can be
much longer than a microcode download.

> - Cite the SPC text that requires a PQ change across the ALUA
> unavailable state.

You're right that the citation in 4/5 is wrong; it should be SPC-4
5.15.2.4.5, not 5.15.2.4.4. That section says that while in the
unavailable state the device server shall support:

  a) INQUIRY (the peripheral qualifier (see 6.6.2) shall be set to
     001b);

So the PQ change isn't something we chose: once the array uses
unavailable, the standard requires PQ = 001b. We set it to 1 on
entering unavailable and back to 0 when we leave it.

Why I think Linux should honor it in both directions: today the
outcome depends on when the host looked, not on what the target
reports.

  - A host that scans while the link is down sees PQ = 1 on those
    paths. scsi_bus_match() doesn't bind sd, so they are not disks.
    When the link comes back and PQ returns to 0, nothing binds sd, and
    those paths stay unusable until someone deletes and rescans the
    device. That is the bug this series started from.

  - A host that was already running keeps sd bound on the same paths
    for as long as the link is down.

Reprobing on a PQ change in either direction makes the binding follow
the PQ the target reports, which is the rule the scan path already
applies. With dm-multipath, the unavailable paths leave the map while
the link is down and come back as new paths afterwards, while the
other array's paths keep serving I/O. Without multipath, a disk whose
only path is unavailable cannot do I/O anyway, and it reappears when
PQ returns to 0.

If you or Martin would still rather not unbind a bound disk by
default, please say so now and I'll rework v9 around that.

> Offlining can't be undone: [...] I'd drop the offline step.

Agreed. Hannes, you asked in the v4 review for more than a warning when
device_reprobe() fails, which is where the SDEV_OFFLINE step came from.
But Bart is right: scsi_rescan_device() only acts on SDEV_RUNNING
devices, so an offlined device never comes back through a later
rescan, and failing to bind a ULD is not an I/O error. For v9 I plan to
just log a real reprobe failure and not treat -EPROBE_DEFER as one.
Please shout if you see that differently.

> 5/5 widens rescan behaviour a lot.

5/5 is needed for FC, but you're right that it does far too much. The
two directions reach the host differently:

  - Entering unavailable, the array reports INQUIRY DATA HAS CHANGED
    (along with the ALUA state change). sd is bound and I/O is flowing,
    so the host sees the UNIT ATTENTION on its next command, and
    scsi_evt_thread() -> scsi_rescan_device() handles it. That is 4/5.

  - Leaving unavailable, a UNIT ATTENTION alone doesn't work. With
    PQ = 1 nothing is bound to the LUN and no commands reach it, so
    nothing ever sees the UNIT ATTENTION. So the array also makes the
    host rescan, the same way it does when the first volume is mapped
    to a host.

    On iSCSI it sends an asynchronous SCSI event with REPORTED LUNS
    DATA HAS CHANGED. iscsid writes to the sysfs rescan attribute of
    every existing device on the session, which is scsi_rescan_device()
    and therefore 4/5, and then scans the host for new LUNs.

    On FC there is no equivalent, so it logs out the host's initiators.
    After the re-login, fc_scsi_scan_rport() calls
    scsi_scan_target(..., SCSI_SCAN_RESCAN), and that scan finds the
    existing scsi_device. Without 5/5 it does nothing with it, PQ = 0
    is never seen, and sd never binds.

So the scan path only has to handle LUNs that can't receive a UNIT
ATTENTION, which are the ones with no driver bound. For v9 I plan to
limit 5/5 to existing devices that are registered and have no driver
bound. A bound LUN learns about INQUIRY changes through UNIT ATTENTIONs
on its own I/O, so the scan path leaves it alone. On a normal host that
skips nearly every LUN, which removes the per-scan cost and the
scan_mutex hold time you described. It also avoids the double LUN 0
rescan, except in the rare case where LUN 0 itself has no driver bound.

For the rest, v9 will:

  - check device_is_registered() before reprobing, so a rescan before
    scsi_sysfs_add_sdev() can't reach __device_attach() with dev->p
    still NULL;
  - keep the LLD's INQUIRY length limit separate from the last response
    length, so scsi_probe_lun() behaves the same on a refresh as on the
    first probe;
  - when a refresh returns PQ 3, PQ 1 with PDT 0x1f, or a different
    vendor/model, warn and keep the existing data rather than accept
    a response that the scan path would treat differently;
  - merge 2/5 and 3/5, declare scsi_update_inquiry_data() in
    scsi_priv.h, and keep the comment explaining why a PQ = 1 device is
    still created;
  - drop cdl_supported from the INQUIRY-derived list in 1/5 and limit
    its claims to what the mutex actually protects;
  - say in the 4/5 commit message that every scsi_rescan_device()
    caller now sends an extra INQUIRY;
  - address the smaller points (0444, a shared constant instead of 256,
    the explicit first-call argument, the stale comments).

Thanks,

Brian