* [PATCH v8 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex
2026-10-19 22:13 [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
@ 2026-10-19 22:14 ` Brian Bunker
2026-10-19 22:14 ` [PATCH v8 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data Brian Bunker
` (4 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Brian Bunker @ 2026-10-19 22:14 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v8 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data
2026-10-19 22:13 [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
2026-10-19 22:14 ` [PATCH v8 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex Brian Bunker
@ 2026-10-19 22:14 ` Brian Bunker
2026-10-19 22:14 ` [PATCH v8 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data() Brian Bunker
` (3 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Brian Bunker @ 2026-10-19 22:14 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v8 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()
2026-10-19 22:13 [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
2026-10-19 22:14 ` [PATCH v8 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex Brian Bunker
2026-10-19 22:14 ` [PATCH v8 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data Brian Bunker
@ 2026-10-19 22:14 ` Brian Bunker
2026-10-19 22:14 ` [PATCH v8 4/5] scsi: core: Add device reprobe support to scsi_rescan_device() Brian Bunker
` (2 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Brian Bunker @ 2026-10-19 22:14 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, Brian Bunker,
Hannes Reinecke
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v8 4/5] scsi: core: Add device reprobe support to scsi_rescan_device()
2026-10-19 22:13 [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
` (2 preceding siblings ...)
2026-10-19 22:14 ` [PATCH v8 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data() Brian Bunker
@ 2026-10-19 22:14 ` Brian Bunker
2026-10-19 22:31 ` sashiko-bot
2026-10-19 22:14 ` [PATCH v8 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan Brian Bunker
2026-10-19 22:41 ` [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Bart Van Assche
5 siblings, 1 reply; 18+ messages in thread
From: Brian Bunker @ 2026-10-19 22:14 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v8 4/5] scsi: core: Add device reprobe support to scsi_rescan_device()
2026-10-19 22:14 ` [PATCH v8 4/5] scsi: core: Add device reprobe support to scsi_rescan_device() Brian Bunker
@ 2026-10-19 22:31 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-10-19 22:31 UTC (permalink / raw)
To: Brian Bunker, linux-scsi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v8 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan
2026-10-19 22:13 [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
` (3 preceding siblings ...)
2026-10-19 22:14 ` [PATCH v8 4/5] scsi: core: Add device reprobe support to scsi_rescan_device() Brian Bunker
@ 2026-10-19 22:14 ` Brian Bunker
2026-10-19 22:32 ` sashiko-bot
2026-10-19 22:41 ` [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Bart Van Assche
5 siblings, 1 reply; 18+ messages in thread
From: Brian Bunker @ 2026-10-19 22:14 UTC (permalink / raw)
To: linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, Bart Van Assche, krishna.kant, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v8 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan
2026-10-19 22:14 ` [PATCH v8 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan Brian Bunker
@ 2026-10-19 22:32 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-10-19 22:32 UTC (permalink / raw)
To: Brian Bunker, linux-scsi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-19 22:13 [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Brian Bunker
` (4 preceding siblings ...)
2026-10-19 22:14 ` [PATCH v8 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan Brian Bunker
@ 2026-10-19 22:41 ` Bart Van Assche
2026-10-20 17:17 ` Brian Bunker
5 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2026-10-19 22:41 UTC (permalink / raw)
To: Brian Bunker, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, krishna.kant
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.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-19 22:41 ` [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan Bart Van Assche
@ 2026-10-20 17:17 ` Brian Bunker
2026-10-20 22:09 ` Bart Van Assche
2026-10-21 10:45 ` Hannes Reinecke
0 siblings, 2 replies; 18+ messages in thread
From: Brian Bunker @ 2026-10-20 17:17 UTC (permalink / raw)
To: Bart Van Assche, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, krishna.kant
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-20 17:17 ` Brian Bunker
@ 2026-10-20 22:09 ` Bart Van Assche
2026-10-21 17:04 ` Brian Bunker
2026-10-21 10:45 ` Hannes Reinecke
1 sibling, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2026-10-20 22:09 UTC (permalink / raw)
To: Brian Bunker, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, krishna.kant
On 10/20/26 10:17 AM, Brian Bunker wrote:
> On 10/19/26 3:41 PM, Bart Van Assche wrote:
>> 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.
Having taken another look at the cover letter makes me wonder whether
it is really necessary to refetch standard INQUIRY data and reprobe the
device when the peripheral qualifier or device type has changed? This is
a huge behavior change with a complex implementation so it will be hard
to convince the SCSI maintainer that this change is both safe and
necessary. When I was working on active/active SCSI arrays myself we
didn't care about the INQUIRY data in the kernel and let multipathd
figure out which path(s) to use.
> 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.
This is a big behavior change. I prefer not to unbind disks if PQ
changes.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-20 22:09 ` Bart Van Assche
@ 2026-10-21 17:04 ` Brian Bunker
0 siblings, 0 replies; 18+ messages in thread
From: Brian Bunker @ 2026-10-21 17:04 UTC (permalink / raw)
To: Bart Van Assche, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
Hannes Reinecke, krishna.kant
On 10/20/26 3:09 PM, Bart Van Assche wrote:
> This is a big behavior change. I prefer not to unbind disks if PQ
> changes.
OK, no unbinding. v9 will never detach a driver from a bound disk.
> Having taken another look at the cover letter makes me wonder whether
> it is really necessary to refetch standard INQUIRY data and reprobe the
> device when the peripheral qualifier or device type has changed?
> [...] When I was working on active/active SCSI arrays myself we
> didn't care about the INQUIRY data in the kernel and let multipathd
> figure out which path(s) to use.
I'd like multipathd to make that decision too. The problem is a path it
never gets to see. If a host scans while the array reports PQ = 1 on a
path, scsi_bus_match() doesn't bind sd, so the path has no block device
and multipathd doesn't know it exists. When the array returns to
PQ = 0, nothing binds sd, and the path stays unusable until someone
deletes the device and rescans. User space can't fix that; only the
kernel can bind sd.
The array has no choice about PQ = 1 here. SPC-4 5.15.2.4.5 requires it
in the unavailable state:
a) INQUIRY (the peripheral qualifier (see 6.6.2) shall be set to
001b);
We discussed this in "The PQ=1 saga" thread Martin started in January
2023. The simplest fix, letting scsi_bus_match() bind at PQ = 1, came
up there, and Martin pointed out that some arrays rely on PQ = 1 to
keep a ULD from binding and that keying off TPGS is probably not
enough. The IBM 2145 case behind 948e922fc446 (since reverted) shows
why: it returns PQ = 1 with PDT 0 for LUNs that aren't mapped. So I'm
not proposing that.
What came out of that thread instead was to reprobe the device when it
leaves the unavailable state. Martin suggested triggering a re-probe on
the transition, and you suggested a rescan when the ALUA state goes
from unavailable or standby to an active state. With unbinding off the
table, that is all v9 needs to do:
- Only logical units with no driver bound are refreshed. A bound
disk is left exactly as it is today: no INQUIRY refetch, no
reprobe.
- If the refreshed INQUIRY data shows PQ = 0, the driver is bound.
Nothing is ever unbound.
The trigger has to come from the target, because a LU with no driver
bound gets no commands, so no UNIT ATTENTION or ALUA state change
reaches it. Our arrays make the host rescan when they leave
unavailable, the same way they do when the first volume is mapped to a
host. On iSCSI they send an asynchronous event with REPORTED LUNS DATA
HAS CHANGED, and iscsid writes to the sysfs rescan attribute of every
device on the session, which ends up in scsi_rescan_device(). On FC
they log out the host's initiators, and after the re-login the
transport scans the target. That scan finds the existing scsi_device
and today does nothing with it, so the scan path calls
scsi_rescan_device() for registered devices that have no driver bound.
Bound devices skip it, so the scan cost and the time scan_mutex is held
don't change for them.
That leaves one difference you should know about. While a path is
unavailable, a host that was running when it became unavailable keeps
a bound sd on that path, which ALUA and multipathd treat as a failed
path. A host that scanned while it was unavailable has no sd on it.
Both end up the same once the array returns to PQ = 0.
This makes v9 much smaller than v8: no unbind, no offline handling,
and no INQUIRY update on a device that has a driver bound. The fixes
you and Sashiko found still apply where the code remains
(device_is_registered() before binding, the inquiry_len handling, the
PQ 3 / PDT 1Fh filters, and the structural points).
Martin, does this direction work for you before I write it?
Thanks,
Brian
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-20 17:17 ` Brian Bunker
2026-10-20 22:09 ` Bart Van Assche
@ 2026-10-21 10:45 ` Hannes Reinecke
2026-10-21 13:21 ` Bart Van Assche
2026-10-21 18:50 ` Brian Bunker
1 sibling, 2 replies; 18+ messages in thread
From: Hannes Reinecke @ 2026-10-21 10:45 UTC (permalink / raw)
To: Brian Bunker, Bart Van Assche, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
krishna.kant
On 10/20/26 7:17 PM, Brian Bunker wrote:
> 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.
>
Aw. The mysteries of SPC. Can't you simply use 'Standby' state?
That would not require all this weird rescanning.
> 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.
>
Weelll ... an 'unavailable' device _never_ comes back.
The only chance it had to 'come back' would be to set a UNIT ATTENTION.
But that we will only ever see if we send commands to that device,
which we don't as it's unavailable ...
Unless the 'active' path is allowed to set a unit attention on behalf
of the unavailable paths, but I don't think that's possible.
Also we have to worry about the implications for multipathing:
If the device is set to 'unavailable' we'll be disconnecting it
from multipathing as the 'sd' driver will be detached.
So we'll need a full rescan (and a multipath rebuild) to get it
back.
If, say, the device would report 'standby' than it would appear
like 'just' another active-passive array, multipath maps would
be left intact, multipath would do the polling, and the device
will be reinstated once the internal link is back up.
We'll be missing all of these things when you set the device
to 'unavailable'.
What it the proposed strategy to get it back?
Note: this is completely orthogonal to this patchset.
It's just me wondering how this setup is supposed to work.
> - 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.
>
Yes, but that was the point.
If reprobe fails, what are you going to do?
You cannot 'magically' get it back, as _normally_ you would do a reprobe
to get it back. But that failed. So you _really_ need to delete that
device and restart from scratch.
>> 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.
>
When do you do the LOGO? When entering the unavaiblable state?
Or when resetting it back to normal?
> 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.
>
See above.
The whole thing would be _so_ much easier if your array could return
'standby' instead of 'unavailable', as then most of the issues above
wouldn't apply.
But that's not my decision to take...
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-21 10:45 ` Hannes Reinecke
@ 2026-10-21 13:21 ` Bart Van Assche
2026-10-21 18:50 ` Brian Bunker
1 sibling, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2026-10-21 13:21 UTC (permalink / raw)
To: Hannes Reinecke, Brian Bunker, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
krishna.kant
On 10/21/26 3:45 AM, Hannes Reinecke wrote:
> Aw. The mysteries of SPC. Can't you simply use 'Standby' state?
> That would not require all this weird rescanning.
+1 for switching to active/standby. Ten years ago I learned that
Windows SCSI ALUA initiator support for the "unavailable" mode was not
working reliably. This forced us to change the storage array
implementation from active/unavailable to active/standby. I haven't
checked since then whether the Windows SCSI ALUA implementation has
improved.
Bart.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-21 10:45 ` Hannes Reinecke
2026-10-21 13:21 ` Bart Van Assche
@ 2026-10-21 18:50 ` Brian Bunker
2026-10-22 11:56 ` Hannes Reinecke
1 sibling, 1 reply; 18+ messages in thread
From: Brian Bunker @ 2026-10-21 18:50 UTC (permalink / raw)
To: Hannes Reinecke, Bart Van Assche, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
krishna.kant
On 10/21/26 12:45 PM, Hannes Reinecke wrote:
> Aw. The mysteries of SPC. Can't you simply use 'Standby' state?
> That would not require all this weird rescanning.
We do use standby today, and have since 2017. That is where the
trouble is. SPC-4 5.15.2.4.4 says that in standby the device server
"shall support those of the following commands that it supports while
in the active/optimized primary target port asymmetric access state",
and the list includes:
l) PERSISTENT RESERVE IN;
m) PERSISTENT RESERVE OUT;
We support persistent reservations in active/optimized, so standby has
to accept them too. When the link between the two arrays is down, the
array that has stopped serving the volume can't mirror a reservation
change to its peer, so it can't accept one without the two arrays
ending up with different reservation state. Making standby work in
that situation takes a lot of extra handling on the array. The
unavailable list in 5.15.2.4.5 has no PR commands, which is exactly
what this case needs.
Standby also isn't handled the same way by every host. ESX and AIX,
for example, don't treat a path as down when RTPG reports it as
standby; their path checkers keep trying to use it. So we carry
OS-specific exceptions for how standby is presented to each host type.
ESX and AIX both handle unavailable. Linux is the one we can't use it
with today, because of the problem this series addresses. If Linux
handled it too, unavailable could be the rule for us, and most of the
OS-specific code and the extra PR handling could go away. That is the
motivation for doing this in Linux.
Bart, on Windows: I don't know how current Windows handles
unavailable. If it still doesn't handle it well, we can keep using
standby for Windows hosts, but that would be the one exception rather
than the rule.
> Weelll ... an 'unavailable' device _never_ comes back.
> The only chance it had to 'come back' would be to set a UNIT ATTENTION.
> But that we will only ever see if we send commands to that device,
> which we don't as it's unavailable ...
[...]
> What it the proposed strategy to get it back?
It depends on whether sd is bound, and after Bart's reply sd will stay
bound on a running host (I've agreed to drop unbinding; see my reply to
Bart from earlier today).
- A host that was running when the path became unavailable keeps sd
bound. This is the case you describe for standby: the multipath
map stays intact, multipathd's path checker keeps polling the
path, and when the array leaves unavailable the UNIT ATTENTIONs it
posts (ALUA state changed, INQUIRY DATA HAS CHANGED) are seen on
those polls and the path is reinstated.
- A host that scanned while the path was unavailable has no sd on
it, so nothing polls it, and you're right that a UNIT ATTENTION
alone would never be seen. For that case the array makes the host
look again when it leaves unavailable: on iSCSI it sends an
asynchronous event with REPORTED LUNS DATA HAS CHANGED, and iscsid
rescans each device on the session; on FC it logs out the
initiators, and the transport scans the target after the re-login.
That rescan is where v9 binds sd.
> Also we have to worry about the implications for multipathing:
> If the device is set to 'unavailable' we'll be disconnecting it
> from multipathing as the 'sd' driver will be detached.
With unbinding dropped, that no longer happens. A bound path stays in
the map as a failed path, as it would in standby.
> When do you do the LOGO? When entering the unavaiblable state?
> Or when resetting it back to normal?
When leaving unavailable. Entering it, the array only posts INQUIRY
DATA HAS CHANGED and the ALUA state change; sd is bound and sees them
on its next command. Leaving it, the array posts both UNIT ATTENTIONs
and then, on FC, logs out the initiators so that a host with no sd on
the path rescans.
> Yes, but that was the point.
> If reprobe fails, what are you going to do?
> You cannot 'magically' get it back, as _normally_ you would do a reprobe
> to get it back. But that failed. So you _really_ need to delete that
> device and restart from scratch.
In v9 the reprobe only ever binds a driver to a LU that has none. If
the bind fails, the device is left in the state it was already in: a
scsi_device with no ULD, exactly what the scan creates for a PQ = 1 LU.
Nothing that worked before is lost, and the next rescan the array
triggers tries again. Deleting the device and scanning from scratch
would end at the same sd probe, so I'd rather log the failure and leave
the device for the next rescan than add a removal path.
Thanks,
Brian
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-21 18:50 ` Brian Bunker
@ 2026-10-22 11:56 ` Hannes Reinecke
2026-10-22 18:04 ` Brian Bunker
0 siblings, 1 reply; 18+ messages in thread
From: Hannes Reinecke @ 2026-10-22 11:56 UTC (permalink / raw)
To: Brian Bunker, Bart Van Assche, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
krishna.kant
On 10/21/26 8:50 PM, Brian Bunker wrote:
> On 10/21/26 12:45 PM, Hannes Reinecke wrote:
>> Aw. The mysteries of SPC. Can't you simply use 'Standby' state?
>> That would not require all this weird rescanning.
>
> We do use standby today, and have since 2017. That is where the
> trouble is. SPC-4 5.15.2.4.4 says that in standby the device server
> "shall support those of the following commands that it supports while
> in the active/optimized primary target port asymmetric access state",
> and the list includes:
>
> l) PERSISTENT RESERVE IN;
> m) PERSISTENT RESERVE OUT;
>
> We support persistent reservations in active/optimized, so standby has
> to accept them too. When the link between the two arrays is down, the
> array that has stopped serving the volume can't mirror a reservation
> change to its peer, so it can't accept one without the two arrays
> ending up with different reservation state. Making standby work in
> that situation takes a lot of extra handling on the array. The
> unavailable list in 5.15.2.4.5 has no PR commands, which is exactly
> what this case needs.
>
> Standby also isn't handled the same way by every host. ESX and AIX,
> for example, don't treat a path as down when RTPG reports it as
> standby; their path checkers keep trying to use it. So we carry
> OS-specific exceptions for how standby is presented to each host type.
>
> ESX and AIX both handle unavailable. Linux is the one we can't use it
> with today, because of the problem this series addresses. If Linux
> handled it too, unavailable could be the rule for us, and most of the
> OS-specific code and the extra PR handling could go away. That is the
> motivation for doing this in Linux.
>
> Bart, on Windows: I don't know how current Windows handles
> unavailable. If it still doesn't handle it well, we can keep using
> standby for Windows hosts, but that would be the one exception rather
> than the rule.
>
>> Weelll ... an 'unavailable' device _never_ comes back.
>> The only chance it had to 'come back' would be to set a UNIT ATTENTION.
>> But that we will only ever see if we send commands to that device,
>> which we don't as it's unavailable ...
> [...]
>> What it the proposed strategy to get it back?
>
> It depends on whether sd is bound, and after Bart's reply sd will stay
> bound on a running host (I've agreed to drop unbinding; see my reply to
> Bart from earlier today).
>
> - A host that was running when the path became unavailable keeps sd
> bound. This is the case you describe for standby: the multipath
> map stays intact, multipathd's path checker keeps polling the
> path, and when the array leaves unavailable the UNIT ATTENTIONs it
> posts (ALUA state changed, INQUIRY DATA HAS CHANGED) are seen on
> those polls and the path is reinstated.
>
> - A host that scanned while the path was unavailable has no sd on
> it, so nothing polls it, and you're right that a UNIT ATTENTION
> alone would never be seen. For that case the array makes the host
> look again when it leaves unavailable: on iSCSI it sends an
> asynchronous event with REPORTED LUNS DATA HAS CHANGED, and iscsid
> rescans each device on the session; on FC it logs out the
> initiators, and the transport scans the target after the re-login.
> That rescan is where v9 binds sd.
>
>> Also we have to worry about the implications for multipathing:
>> If the device is set to 'unavailable' we'll be disconnecting it
>> from multipathing as the 'sd' driver will be detached.
>
> With unbinding dropped, that no longer happens. A bound path stays in
> the map as a failed path, as it would in standby.
>
Hmm.
But if you want to have this behaviour it would make sense to modify
scsi_bus_match() to extend to pqual 01b, too; then 'sd' would stay
bound to the scsi device even after rescan, and we wouldn't need
to worry about driver detaching and multipathd becoming inoperational.
>> When do you do the LOGO? When entering the unavaiblable state?
>> Or when resetting it back to normal?
>
> When leaving unavailable. Entering it, the array only posts INQUIRY
> DATA HAS CHANGED and the ALUA state change; sd is bound and sees them
> on its next command. Leaving it, the array posts both UNIT ATTENTIONs
> and then, on FC, logs out the initiators so that a host with no sd on
> the path rescans.
>
Ah. That'll explain it.
>> Yes, but that was the point.
>> If reprobe fails, what are you going to do?
>> You cannot 'magically' get it back, as _normally_ you would do a reprobe
>> to get it back. But that failed. So you _really_ need to delete that
>> device and restart from scratch.
>
> In v9 the reprobe only ever binds a driver to a LU that has none. If
> the bind fails, the device is left in the state it was already in: a
> scsi_device with no ULD, exactly what the scan creates for a PQ = 1 LU.
> Nothing that worked before is lost, and the next rescan the array
> triggers tries again. Deleting the device and scanning from scratch
> would end at the same sd probe, so I'd rather log the failure and leave
> the device for the next rescan than add a removal path.
>
Which if fine (I think).
But still it might be worthwhile exploring the idea of extending
scsi_bus_match(), as that will solve any ambiguity with 'sd' attaching
to the device.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-22 11:56 ` Hannes Reinecke
@ 2026-10-22 18:04 ` Brian Bunker
2026-10-23 6:59 ` Hannes Reinecke
0 siblings, 1 reply; 18+ messages in thread
From: Brian Bunker @ 2026-10-22 18:04 UTC (permalink / raw)
To: Hannes Reinecke, Bart Van Assche, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
krishna.kant
On 10/22/26 1:56 PM, Hannes Reinecke wrote:
> But if you want to have this behaviour it would make sense to modify
> scsi_bus_match() to extend to pqual 01b, too; then 'sd' would stay
> bound to the scsi device even after rescan, and we wouldn't need
> to worry about driver detaching and multipathd becoming inoperational.
[...]
> But still it might be worthwhile exploring the idea of extending
> scsi_bus_match(), as that will solve any ambiguity with 'sd' attaching
> to the device.
I agree, and I'd much rather do that than the reprobe. It was my first
idea in "The PQ=1 saga" thread in 2023, but matching PQ = 1 on its own
isn't safe, and I think I now understand why Martin pushed back then.
PQ = 001b has two meanings. One is the ALUA unavailable state, which
SPC-4 5.15.2.4.5 a) requires. The other is a LUN that isn't mapped to
the initiator at all. SPC-4 6.6.1 says:
In response to an INQUIRY command received by an incorrect logical
unit, the SCSI target device shall return the INQUIRY data with the
peripheral qualifier set to the value defined in 6.6.2. The device
server shall terminate an INQUIRY command with CHECK CONDITION
status only if the device server is unable to return the requested
INQUIRY data.
So the INQUIRY succeeds, and 6.6.2 decides the qualifier. An array
can map a volume at any of its LUNs, so 011b ("The device server is
not capable of supporting a peripheral device on this logical unit")
would be wrong. 001b is the correct answer for a LUN that could hold a
volume but currently doesn't:
001b A peripheral device having the indicated peripheral device type
is not connected to this logical unit. However, the device
server is capable of supporting the indicated peripheral device
type on this logical unit.
An array that returns PQ = 1 for an unmapped LUN is therefore doing
exactly what the standard says. The trap is set elsewhere, by REPORT
LUNS. Some arrays, at least in the past, returned a full LUN list
(0 to 255) in REPORT LUNS and left it to the initiator to work out
which LUNs were really there. Linux then sends INQUIRY to every one of
them and, correctly, gets PQ = 1 back from all the unmapped ones. Binding sd at
PQ = 1 would turn each of those into a disk with nothing behind it: no
READ CAPACITY, no logical block size. I suspect that is the experience
behind Martin's "there are also storage arrays out there that rely on
PQ=1 to inhibit devices being claimed", and the IBM 2145 case in
948e922fc446 (PQ = 1, PDT 0 for a LUN that isn't mapped) looks like
the same thing.
Checking TPGS doesn't help, because the TPGS bits come from the same
INQUIRY response. The array fills in the standard INQUIRY data for an
unmapped LUN the same way it does for a mapped one.
A command other than INQUIRY does tell the two apart. A LUN that isn't
mapped rejects REPORT TARGET PORT GROUPS with LOGICAL UNIT NOT
SUPPORTED, while a LU in the unavailable state must accept it
(5.15.2.4.5 c)) and reports its target port group as unavailable.
The ALUA handler is already in the right place to send it.
scsi_sysfs_add_sdev() calls scsi_dh_add_device() before device_add(),
so scsi_dh_alua has attached before scsi_bus_match() runs. The only
thing missing is the RTPG result: alua_check_vpd() queues the first
RTPG with alua_rtpg_queue(), so it normally completes after the device
has been matched.
So my proposal is:
- In scsi_dh_alua, for a device that reported PQ = 1, send the first
RTPG synchronously at attach time and set sdev->access_state from
that device's own response. An unmapped LUN fails the RTPG and
stays as it is today. The queued RTPG and the shared port group
handling stay as they are, so an unmapped LUN can't pick up
another LU's state from the port group.
- In scsi_bus_match(), match PQ = 0 as today, and also PQ = 1 when
access_state says unavailable.
sd then binds to an unavailable path whether the host was running when
the path became unavailable or scanned while it was. The path stays in
the multipath map as a failed path, multipathd keeps polling it, and
the existing UNIT ATTENTION handling brings it back, as you described.
This relies on scsi_dh_alua being attached at scan time (built in, or
loaded before the scan), which I think is how it should work anyway.
It would replace the v8 reprobe completely: no INQUIRY refresh, no
reprobe, and no change to the scan path.
Would you take a synchronous first RTPG in alua_check_vpd() for PQ = 1
devices? If you'd rather keep the handler as it is, the alternative is
for the SCSI core to send a TEST UNIT READY to PQ = 1 LUNs during the
scan and look for LOGICAL UNIT NOT ACCESSIBLE, TARGET PORT IN
UNAVAILABLE STATE, but I think the handler is the better place for it.
Thanks,
Brian
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan
2026-10-22 18:04 ` Brian Bunker
@ 2026-10-23 6:59 ` Hannes Reinecke
0 siblings, 0 replies; 18+ messages in thread
From: Hannes Reinecke @ 2026-10-23 6:59 UTC (permalink / raw)
To: Brian Bunker, Bart Van Assche, linux-scsi
Cc: Martin K . Petersen, Martin K . Petersen, James.Bottomley,
krishna.kant
On 10/22/26 8:04 PM, Brian Bunker wrote:
> On 10/22/26 1:56 PM, Hannes Reinecke wrote:
>> But if you want to have this behaviour it would make sense to modify
>> scsi_bus_match() to extend to pqual 01b, too; then 'sd' would stay
>> bound to the scsi device even after rescan, and we wouldn't need
>> to worry about driver detaching and multipathd becoming inoperational.
> [...]
>> But still it might be worthwhile exploring the idea of extending
>> scsi_bus_match(), as that will solve any ambiguity with 'sd' attaching
>> to the device.
>
> I agree, and I'd much rather do that than the reprobe. It was my first
> idea in "The PQ=1 saga" thread in 2023, but matching PQ = 1 on its own
> isn't safe, and I think I now understand why Martin pushed back then.
>
> PQ = 001b has two meanings. One is the ALUA unavailable state, which
> SPC-4 5.15.2.4.5 a) requires. The other is a LUN that isn't mapped to
> the initiator at all. SPC-4 6.6.1 says:
>
> In response to an INQUIRY command received by an incorrect logical
> unit, the SCSI target device shall return the INQUIRY data with the
> peripheral qualifier set to the value defined in 6.6.2. The device
> server shall terminate an INQUIRY command with CHECK CONDITION
> status only if the device server is unable to return the requested
> INQUIRY data.
>
> So the INQUIRY succeeds, and 6.6.2 decides the qualifier. An array
> can map a volume at any of its LUNs, so 011b ("The device server is
> not capable of supporting a peripheral device on this logical unit")
> would be wrong. 001b is the correct answer for a LUN that could hold a
> volume but currently doesn't:
>
> 001b A peripheral device having the indicated peripheral device type
> is not connected to this logical unit. However, the device
> server is capable of supporting the indicated peripheral device
> type on this logical unit.
>
> An array that returns PQ = 1 for an unmapped LUN is therefore doing
> exactly what the standard says. The trap is set elsewhere, by REPORT
> LUNS. Some arrays, at least in the past, returned a full LUN list
> (0 to 255) in REPORT LUNS and left it to the initiator to work out
> which LUNs were really there. Linux then sends INQUIRY to every one of
> them and, correctly, gets PQ = 1 back from all the unmapped ones. Binding sd at
> PQ = 1 would turn each of those into a disk with nothing behind it: no
> READ CAPACITY, no logical block size. I suspect that is the experience
> behind Martin's "there are also storage arrays out there that rely on
> PQ=1 to inhibit devices being claimed", and the IBM 2145 case in
> 948e922fc446 (PQ = 1, PDT 0 for a LUN that isn't mapped) looks like
> the same thing.
>
I actually had similar experiences but dared not to raise them to
not derail the discussion :-)
> Checking TPGS doesn't help, because the TPGS bits come from the same
> INQUIRY response. The array fills in the standard INQUIRY data for an
> unmapped LUN the same way it does for a mapped one.
>
> A command other than INQUIRY does tell the two apart. A LUN that isn't
> mapped rejects REPORT TARGET PORT GROUPS with LOGICAL UNIT NOT
> SUPPORTED, while a LU in the unavailable state must accept it
> (5.15.2.4.5 c)) and reports its target port group as unavailable.
>
> The ALUA handler is already in the right place to send it.
> scsi_sysfs_add_sdev() calls scsi_dh_add_device() before device_add(),
> so scsi_dh_alua has attached before scsi_bus_match() runs. The only
> thing missing is the RTPG result: alua_check_vpd() queues the first
> RTPG with alua_rtpg_queue(), so it normally completes after the device
> has been matched.
>
> So my proposal is:
>
> - In scsi_dh_alua, for a device that reported PQ = 1, send the first
> RTPG synchronously at attach time and set sdev->access_state from
> that device's own response. An unmapped LUN fails the RTPG and
> stays as it is today. The queued RTPG and the shared port group
> handling stay as they are, so an unmapped LUN can't pick up
> another LU's state from the port group.
>
> - In scsi_bus_match(), match PQ = 0 as today, and also PQ = 1 when
> access_state says unavailable.
>
> sd then binds to an unavailable path whether the host was running when
> the path became unavailable or scanned while it was. The path stays in
> the multipath map as a failed path, multipathd keeps polling it, and
> the existing UNIT ATTENTION handling brings it back, as you described.
> This relies on scsi_dh_alua being attached at scan time (built in, or
> loaded before the scan), which I think is how it should work anyway.
>
> It would replace the v8 reprobe completely: no INQUIRY refresh, no
> reprobe, and no change to the scan path.
>
> Would you take a synchronous first RTPG in alua_check_vpd() for PQ = 1
> devices? If you'd rather keep the handler as it is, the alternative is
> for the SCSI core to send a TEST UNIT READY to PQ = 1 LUNs during the
> scan and look for LOGICAL UNIT NOT ACCESSIBLE, TARGET PORT IN
> UNAVAILABLE STATE, but I think the handler is the better place for it.
>
Actually my proposal would even go a bit further: move the ALUA probing
out of the alua handler and into the SCSI core. ALUA is a standard SCSI
feature, and really is unrelated to multipathing. An admin might decide
to not use multipathing, but that doesn't change the fact that the
device supports ALUA.
So yes, that is a direction which I wanted to go anyway.
We could move 'submit_rtpg' into the SCSI core, and execute it
directly during scsi_add_lun(). Calling into the device handler
will be too much overhead as we don't need all this elaborate
'pg' parsing; all we're interested in here is to get the ALUA
state. That should be relatively straightforward.
And then we can evaluate the ALUA state during bus_probe and
we should be able to achieve what we wanted.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 18+ messages in thread