[PATCH v2 0/7] scsi_debug: Enable lock context analysis

[PATCH v2 0/7] scsi_debug: Enable lock context analysis

[Bart Van Assche]

Hi Martin,

This patch series fixes a locking bug in the scsi_debug driver and enables
lock context analysis.

Please consider this patch series for the next merge window.

Changes compared to v1:
 - Fixed an issue where the error code was clobbered in resp_write_same() and
   __resp_write_same() when fetch_to_dev_buffer() fails (John Garry and
   sashiko-bot).
 - Fixed return value handling in resp_read_dt0() and resp_write_dt0():
   previously a positive byte count returned by do_device_access() was
   incorrectly treated as a SCSI status code and returned to the caller. Now,
   the SCSI status is passed via an output pointer and only negative return
   values trigger an error return (sashiko-bot).
 - Improved lock context annotations:
   * Annotated existing rwlock_t members (&sip->macc_data_lck,
     &sip->macc_sector_lck, &sip->macc_meta_lck) directly with __acquires /
     __releases instead of defining and embedding dummy context lock structs
     in struct sdeb_store_info.
   * Dropped the unwarranted __context_unsafe() annotation from
     sdeb_data_read_lock().
   * Updated the commit descriptions to match the code changes.

Thanks,

Bart.

Bart Van Assche (7):
  scsi: scsi_debug: Fix a locking bug in resp_write_same()
  scsi: scsi_debug: Split resp_write_same()
  scsi: scsi_debug: Split corrupt_lbas()
  scsi: scsi_debug: Split resp_read_dt0()
  scsi: scsi_debug: Split resp_write_dt0()
  scsi: scsi_debug: Improve lock context annotations
  scsi: core: Enable lock context analysis for the scsi_debug driver

 drivers/scsi/Makefile     |   1 +
 drivers/scsi/scsi_debug.c | 372 +++++++++++++++++++++-----------------
 2 files changed, 210 insertions(+), 163 deletions(-)

[PATCH v2 1/7] scsi: scsi_debug: Fix a locking bug in resp_write_same()

[Bart Van Assche]

If fetch_to_dev_buffer() fails in resp_write_same(), the function jumps
to 'out' without releasing the data write lock acquired earlier via
sdeb_data_write_lock(). Jump to 'unlock' instead so that
sdeb_data_write_unlock() is called on the error path. This bug has been
discovered by building the scsi_debug driver with Clang and modified
lock context annotations. The modified lock context annotations are
available in patch "scsi: scsi_debug: Improve lock context annotations".

Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6941809dfdb7..ed8d69f305f8 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5402,7 +5402,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 
 	if (-1 == ret) {
 		ret = DID_ERROR << 16;
-		goto out;
+		goto unlock;
 	} else if (sdebug_verbose && !ndob && (ret < lb_size))
 		sdev_printk(KERN_INFO, scp->device,
 			    "%s: %s: lb size=%u, IO sent=%d bytes\n",
@@ -5419,8 +5419,9 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 	/* If ZBC zone then bump its write pointer */
 	if (sdebug_dev_is_zoned(devip))
 		zbc_inc_wp(devip, lba, num);
-	sdeb_data_write_unlock(sip);
 	ret = 0;
+unlock:
+	sdeb_data_write_unlock(sip);
 out:
 	if (meta_data_locked)
 		sdeb_meta_write_unlock(sip);

[PATCH v2 2/7] scsi: scsi_debug: Split resp_write_same()

[Bart Van Assche]

Since the Clang thread-safety analyzer does not support conditional
locking, split the function resp_write_same() such that all locking
statements end up in the same branch of an if-statement.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index ed8d69f305f8..d0d2cbac487b 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5360,8 +5360,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
 	return ret;
 }
 
-static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
-			   u32 ei_lba, bool unmap, bool ndob)
+static int __resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
+			     u32 ei_lba, bool unmap, bool ndob)
 {
 	struct scsi_device *sdp = scp->device;
 	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
@@ -5373,20 +5373,14 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 						scp->device->hostdata, true);
 	u8 *fs1p;
 	u8 *fsp;
-	bool meta_data_locked = false;
-
-	if (sdebug_dev_is_zoned(devip) || scsi_debug_lbp()) {
-		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
 
 	ret = check_device_access_params(scp, lba, num, true);
 	if (ret)
-		goto out;
+		return ret;
 
 	if (unmap && scsi_debug_lbp()) {
 		unmap_region(sip, lba, num);
-		goto out;
+		return ret;
 	}
 	lbaa = lba;
 	block = do_div(lbaa, sdebug_store_sectors);
@@ -5422,9 +5416,25 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 	ret = 0;
 unlock:
 	sdeb_data_write_unlock(sip);
-out:
-	if (meta_data_locked)
+	return ret;
+}
+
+static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num, u32 ei_lba,
+			   bool unmap, bool ndob)
+{
+	struct scsi_device *sdp = scp->device;
+	struct sdebug_dev_info *devip = sdp->hostdata;
+	struct sdeb_store_info *sip = devip2sip(devip, true);
+	int ret;
+
+	if (sdebug_dev_is_zoned(devip) || scsi_debug_lbp()) {
+		sdeb_meta_write_lock(sip);
+		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob);
+	}
+
 	return ret;
 }
 

[PATCH v2 3/7] scsi: scsi_debug: Split corrupt_lbas()

[Bart Van Assche]

Move the inner logic of corrupt_lbas() into a new helper function
__corrupt_lbas() and call it from separate locked and unlocked branches
in corrupt_lbas(). This patch prepares for enabling context analysis.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 42 ++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index d0d2cbac487b..0f4637d1265a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4948,39 +4948,26 @@ static int resp_write_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
-static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
-		u32 nr_bit_errors, s32 reftag_adjust)
+static int __corrupt_lbas(struct sdeb_store_info *sip, u64 lba, u32 num,
+			  u32 nr_bit_errors, s32 reftag_adjust)
 {
-	struct sdeb_store_info *sip = devip2sip(devip, false);
-	bool meta_data_locked = false;
 	u32 block, num_mapped, b, i;
-	int error = 0;
-
-	if (sdebug_dev_is_zoned(devip) ||
-	    sdebug_dix ||
-	    scsi_debug_lbp())  {
-		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
 
 	if (!sip) {
 		pr_err("can't corrupt with fake_rw\n");
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	if (num > sdebug_capacity || lba > sdebug_capacity - num) {
 		pr_err("logical blocks out of bounds: %llu:%u", lba, num);
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	if (scsi_debug_lbp() &&
 	    (!map_state(sip, lba, &num_mapped) || num > num_mapped)) {
 		pr_err("can't modify unmapped logical blocks: %llu:%u",
 			lba, num);
-		error = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	/*
@@ -5018,9 +5005,24 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
 	}
 	sdeb_data_unlock(sip, false);
 
-out_unlock:
-	if (meta_data_locked)
+	return 0;
+}
+
+static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
+			u32 nr_bit_errors, s32 reftag_adjust)
+{
+	struct sdeb_store_info *sip = devip2sip(devip, false);
+	int error;
+
+	if (sdebug_dev_is_zoned(devip) || sdebug_dix || scsi_debug_lbp()) {
+		sdeb_meta_write_lock(sip);
+		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
+				       reftag_adjust);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
+				       reftag_adjust);
+	}
 	return error;
 }
 

[PATCH v2 4/7] scsi: scsi_debug: Split resp_read_dt0()

[Bart Van Assche]

Split this function to remove conditional locking. This patch prepares
for enabling context analysis.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 93 +++++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 39 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 0f4637d1265a..b0785237bfea 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4577,6 +4577,51 @@ static int resp_read_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
+/*
+ * Returns the number of bytes transferred, or -1 with *scsi_status set to a
+ * SCSI status code on failure.
+ */
+static int __resp_read_dt0(struct scsi_cmnd *scp, struct sdeb_store_info *sip,
+			   u64 lba, u32 num, u32 ei_lba, int *scsi_status)
+{
+	u8 *cmd = scp->cmnd;
+
+	/* DIX + T10 DIF */
+	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
+		switch (prot_verify_read(scp, lba, num, ei_lba)) {
+		case 1: /* Guard tag error */
+			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+				mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+				*scsi_status = check_condition_result;
+				return -1;
+			} else if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+				mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+				*scsi_status = illegal_condition_result;
+				return -1;
+			}
+			break;
+		case 3: /* Reference tag error */
+			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+				mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+				*scsi_status = check_condition_result;
+				return -1;
+			} else if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
+				mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+				*scsi_status = illegal_condition_result;
+				return -1;
+			}
+			break;
+		}
+	}
+
+	*scsi_status = DID_ERROR << 16;
+	return do_device_access(sip, scp, 0, lba, num, 0, false, false);
+}
+
 static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 {
 	bool check_prot;
@@ -4586,7 +4631,7 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	u64 lba;
 	struct sdeb_store_info *sip = devip2sip(devip, true);
 	u8 *cmd = scp->cmnd;
-	bool meta_data_locked = false;
+	int scsi_status = DID_ERROR << 16;
 
 	switch (cmd[0]) {
 	case READ_16:
@@ -4668,48 +4713,18 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	}
 
 	if (sdebug_dev_is_zoned(devip) ||
-	    (sdebug_dix && scsi_prot_sg_count(scp)))  {
+	    (sdebug_dix && scsi_prot_sg_count(scp))) {
 		sdeb_meta_read_lock(sip);
-		meta_data_locked = true;
-	}
-
-	/* DIX + T10 DIF */
-	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
-		switch (prot_verify_read(scp, lba, num, ei_lba)) {
-		case 1: /* Guard tag error */
-			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return check_condition_result;
-			} else if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return illegal_condition_result;
-			}
-			break;
-		case 3: /* Reference tag error */
-			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-				      LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return check_condition_result;
-			} else if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
-				sdeb_meta_read_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-				      LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return illegal_condition_result;
-			}
-			break;
-		}
+		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba,
+				      &scsi_status);
+		sdeb_meta_read_unlock(sip);
+	} else {
+		ret = __resp_read_dt0(scp, sip, lba, num, ei_lba,
+				      &scsi_status);
 	}
 
-	ret = do_device_access(sip, scp, 0, lba, num, 0, false, false);
-	if (meta_data_locked)
-		sdeb_meta_read_unlock(sip);
 	if (unlikely(ret == -1))
-		return DID_ERROR << 16;
+		return scsi_status;
 
 	scsi_set_resid(scp, scsi_bufflen(scp) - ret);
 

[PATCH v2 5/7] scsi: scsi_debug: Split resp_write_dt0()

[Bart Van Assche]

Prepare for enabling lock context analysis by eliminating conditional
locking.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 122 +++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 54 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index b0785237bfea..229f35262382 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5041,6 +5041,65 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
 	return error;
 }
 
+/*
+ * Returns the number of bytes transferred, or -1 with *scsi_status set to a
+ * SCSI status code on failure.
+ */
+static int __resp_write_dt0(struct scsi_cmnd *scp,
+			    struct sdebug_dev_info *devip,
+			    struct sdeb_store_info *sip, u64 lba, u32 num,
+			    u32 ei_lba, u8 group, int *scsi_status)
+{
+	int ret;
+
+	*scsi_status = check_device_access_params(scp, lba, num, true);
+	if (*scsi_status)
+		return -1;
+
+	/* DIX + T10 DIF */
+	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
+		switch (prot_verify_write(scp, lba, num, ei_lba)) {
+		case 1: /* Guard tag error */
+			if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+				mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+				*scsi_status = illegal_condition_result;
+				return -1;
+			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+				mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
+				*scsi_status = check_condition_result;
+				return -1;
+			}
+			break;
+		case 3: /* Reference tag error */
+			if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
+				mk_sense_buffer(scp, ILLEGAL_REQUEST,
+					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+				*scsi_status = illegal_condition_result;
+				return -1;
+			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+				mk_sense_buffer(scp, ABORTED_COMMAND,
+					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
+				*scsi_status = check_condition_result;
+				return -1;
+			}
+			break;
+		}
+	}
+
+	*scsi_status = DID_ERROR << 16;
+	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
+	if (unlikely(scsi_debug_lbp()))
+		map_region(sip, lba, num);
+
+	/* If ZBC zone then bump its write pointer */
+	if (sdebug_dev_is_zoned(devip))
+		zbc_inc_wp(devip, lba, num);
+
+	return ret;
+}
+
 static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 {
 	bool check_prot;
@@ -5051,7 +5110,7 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	u64 lba;
 	struct sdeb_store_info *sip = devip2sip(devip, true);
 	u8 *cmd = scp->cmnd;
-	bool meta_data_locked = false;
+	int scsi_status = DID_ERROR << 16;
 
 	if (unlikely(sdebug_opts & SDEBUG_OPT_UNALIGNED_WRITE &&
 		     atomic_read(&sdeb_inject_pending))) {
@@ -5118,63 +5177,18 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	}
 
 	if (sdebug_dev_is_zoned(devip) ||
-	    (sdebug_dix && scsi_prot_sg_count(scp)) ||
-	    scsi_debug_lbp())  {
+	    (sdebug_dix && scsi_prot_sg_count(scp)) || scsi_debug_lbp()) {
 		sdeb_meta_write_lock(sip);
-		meta_data_locked = true;
-	}
-
-	ret = check_device_access_params(scp, lba, num, true);
-	if (ret) {
-		if (meta_data_locked)
-			sdeb_meta_write_unlock(sip);
-		return ret;
-	}
-
-	/* DIX + T10 DIF */
-	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
-		switch (prot_verify_write(scp, lba, num, ei_lba)) {
-		case 1: /* Guard tag error */
-			if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return illegal_condition_result;
-			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
-				return check_condition_result;
-			}
-			break;
-		case 3: /* Reference tag error */
-			if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ILLEGAL_REQUEST,
-					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return illegal_condition_result;
-			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
-				sdeb_meta_write_unlock(sip);
-				mk_sense_buffer(scp, ABORTED_COMMAND,
-					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
-				return check_condition_result;
-			}
-			break;
-		}
-	}
-
-	ret = do_device_access(sip, scp, 0, lba, num, group, true, false);
-	if (unlikely(scsi_debug_lbp()))
-		map_region(sip, lba, num);
-
-	/* If ZBC zone then bump its write pointer */
-	if (sdebug_dev_is_zoned(devip))
-		zbc_inc_wp(devip, lba, num);
-	if (meta_data_locked)
+		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
+				       &scsi_status);
 		sdeb_meta_write_unlock(sip);
+	} else {
+		ret = __resp_write_dt0(scp, devip, sip, lba, num, ei_lba, group,
+				       &scsi_status);
+	}
 
 	if (unlikely(-1 == ret))
-		return DID_ERROR << 16;
+		return scsi_status;
 	else if (unlikely(sdebug_verbose &&
 			  (ret < (num * sdebug_sector_size))))
 		sdev_printk(KERN_INFO, scp->device,

[PATCH v2 6/7] scsi: scsi_debug: Improve lock context annotations

[Bart Van Assche]

Prepare the scsi_debug driver for enabling compiler-based lock context
analysis:

- Annotate the lock and unlock helper functions with __acquires,
  __releases, __acquires_shared, and __releases_shared.

- Remove sparse __acquire() and __release() calls from the helper
  functions since these mislead the lock context analyzer.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_debug.c | 76 ++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 36 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 229f35262382..4329a0c4e137 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4042,42 +4042,43 @@ static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
 
 static inline void
 sdeb_read_lock(rwlock_t *lock)
+	__acquires_shared(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
 		read_lock(lock);
 }
 
 static inline void
 sdeb_read_unlock(rwlock_t *lock)
+	__releases_shared(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
 		read_unlock(lock);
 }
 
 static inline void
 sdeb_write_lock(rwlock_t *lock)
+	__acquires(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__acquire(lock);
-	else
+	if (!sdebug_no_rwlock)
 		write_lock(lock);
 }
 
 static inline void
 sdeb_write_unlock(rwlock_t *lock)
+	__releases(lock)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock)
-		__release(lock);
-	else
+	if (!sdebug_no_rwlock)
 		write_unlock(lock);
 }
 
 static inline void
 sdeb_data_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4086,6 +4087,7 @@ sdeb_data_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4094,6 +4096,7 @@ sdeb_data_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4102,6 +4105,7 @@ sdeb_data_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_data_lck)
 {
 	BUG_ON(!sip);
 
@@ -4110,6 +4114,7 @@ sdeb_data_write_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4118,6 +4123,7 @@ sdeb_data_sector_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4126,6 +4132,7 @@ sdeb_data_sector_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4134,6 +4141,7 @@ sdeb_data_sector_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_sector_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_sector_lck)
 {
 	BUG_ON(!sip);
 
@@ -4155,6 +4163,8 @@ sdeb_data_sector_write_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
+	__acquires(&sip->macc_data_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (atomic)
 		sdeb_data_write_lock(sip);
@@ -4164,6 +4174,8 @@ sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
 
 static inline void
 sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
+	__releases(&sip->macc_data_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (atomic)
 		sdeb_data_write_unlock(sip);
@@ -4174,6 +4186,8 @@ sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
 /* Allow many reads but only 1x write per sector */
 static inline void
 sdeb_data_sector_lock(struct sdeb_store_info *sip, bool do_write)
+	__acquires(&sip->macc_sector_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (do_write)
 		sdeb_data_sector_write_lock(sip);
@@ -4183,6 +4197,8 @@ sdeb_data_sector_lock(struct sdeb_store_info *sip, bool do_write)
 
 static inline void
 sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
+	__releases(&sip->macc_sector_lck)
+	__context_unsafe(/*conditional locking*/)
 {
 	if (do_write)
 		sdeb_data_sector_write_unlock(sip);
@@ -4192,13 +4208,10 @@ sdeb_data_sector_unlock(struct sdeb_store_info *sip, bool do_write)
 
 static inline void
 sdeb_meta_read_lock(struct sdeb_store_info *sip)
+	__acquires_shared(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			read_lock(&sip->macc_meta_lck);
 		else
@@ -4208,13 +4221,10 @@ sdeb_meta_read_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_read_unlock(struct sdeb_store_info *sip)
+	__releases_shared(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			read_unlock(&sip->macc_meta_lck);
 		else
@@ -4224,13 +4234,10 @@ sdeb_meta_read_unlock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_write_lock(struct sdeb_store_info *sip)
+	__acquires(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__acquire(&sip->macc_meta_lck);
-		else
-			__acquire(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			write_lock(&sip->macc_meta_lck);
 		else
@@ -4240,13 +4247,10 @@ sdeb_meta_write_lock(struct sdeb_store_info *sip)
 
 static inline void
 sdeb_meta_write_unlock(struct sdeb_store_info *sip)
+	__releases(&sip->macc_meta_lck)
+	__context_unsafe(/*conditional locking*/)
 {
-	if (sdebug_no_rwlock) {
-		if (sip)
-			__release(&sip->macc_meta_lck);
-		else
-			__release(&sdeb_fake_rw_lck);
-	} else {
+	if (!sdebug_no_rwlock) {
 		if (sip)
 			write_unlock(&sip->macc_meta_lck);
 		else

[PATCH v2 7/7] scsi: core: Enable lock context analysis for the scsi_debug driver

[Bart Van Assche]

Enable lock context analysis now that all lock context analysis warnings
have been fixed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 533623382eca..32d816e942d6 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -18,6 +18,7 @@ CONTEXT_ANALYSIS_constants.o := y
 CONTEXT_ANALYSIS_hosts.o := y
 CONTEXT_ANALYSIS_scsi.o := y
 CONTEXT_ANALYSIS_scsi_common.o := y
+CONTEXT_ANALYSIS_scsi_debug.o := y
 CONTEXT_ANALYSIS_scsi_devinfo.o := y
 CONTEXT_ANALYSIS_scsi_dh.o := y
 CONTEXT_ANALYSIS_scsi_error.o := y

Re: [PATCH v2 1/7] scsi: scsi_debug: Fix a locking bug in resp_write_same()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/7] scsi: scsi_debug: Split resp_write_same()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/7] scsi: scsi_debug: Split corrupt_lbas()

[Christoph Hellwig]

On Thu, Sep 24, 2026 at 03:54:39PM -0700, Bart Van Assche wrote:
> Move the inner logic of corrupt_lbas() into a new helper function
> __corrupt_lbas() and call it from separate locked and unlocked branches
> in corrupt_lbas(). This patch prepares for enabling context analysis.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 4/7] scsi: scsi_debug: Split resp_read_dt0()

[Christoph Hellwig]

On Thu, Sep 24, 2026 at 03:54:40PM -0700, Bart Van Assche wrote:
>  }
>  
> +/*
> + * Returns the number of bytes transferred, or -1 with *scsi_status set to a
> + * SCSI status code on failure.
> + */
> +static int __resp_read_dt0(struct scsi_cmnd *scp, struct sdeb_store_info *sip,
> +			   u64 lba, u32 num, u32 ei_lba, int *scsi_status)
> +{
> +	u8 *cmd = scp->cmnd;
> +
> +	/* DIX + T10 DIF */
> +	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
> +		switch (prot_verify_read(scp, lba, num, ei_lba)) {

Pleas split the enrire DIX case out into a separate helper to reduce
the indentation when you touch this anyway.

> +		case 1: /* Guard tag error */
> +			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
> +				mk_sense_buffer(scp, ABORTED_COMMAND,
> +					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
> +				*scsi_status = check_condition_result;
> +				return -1;
> +			} else if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {

No need for an else after a return. 

> +				*scsi_status = check_condition_result;
> +				return -1;
> +			} else if (scp->prot_flags & SCSI_PROT_REF_CHECK) {

Same here.

Re: [PATCH v2 5/7] scsi: scsi_debug: Split resp_write_dt0()

[Christoph Hellwig]

On Thu, Sep 24, 2026 at 03:54:41PM -0700, Bart Van Assche wrote:
> Prepare for enabling lock context analysis by eliminating conditional
> locking.

Same comments as for the last patch here.

Re: [PATCH v2 7/7] scsi: core: Enable lock context analysis for the scsi_debug driver

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 1/7] scsi: scsi_debug: Fix a locking bug in resp_write_same()

[John Garry]

On 9/24/26 23:54, Bart Van Assche wrote:
> If fetch_to_dev_buffer() fails in resp_write_same(), the function jumps
> to 'out' without releasing the data write lock acquired earlier via
> sdeb_data_write_lock(). Jump to 'unlock' instead so that
> sdeb_data_write_unlock() is called on the error path. This bug has been
> discovered by building the scsi_debug driver with Clang and modified
> lock context annotations. The modified lock context annotations are
> available in patch "scsi: scsi_debug: Improve lock context annotations".
> 
> Fixes: 84f3a3c01d70 ("scsi: scsi_debug: Atomic write support")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: John Garry <john.garry@linux.dev>

> ---
>   drivers/scsi/scsi_debug.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 6941809dfdb7..ed8d69f305f8 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5402,7 +5402,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>   
>   	if (-1 == ret) {
>   		ret = DID_ERROR << 16;
> -		goto out;
> +		goto unlock;
>   	} else if (sdebug_verbose && !ndob && (ret < lb_size))
>   		sdev_printk(KERN_INFO, scp->device,
>   			    "%s: %s: lb size=%u, IO sent=%d bytes\n",
> @@ -5419,8 +5419,9 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>   	/* If ZBC zone then bump its write pointer */
>   	if (sdebug_dev_is_zoned(devip))
>   		zbc_inc_wp(devip, lba, num);
> -	sdeb_data_write_unlock(sip);
>   	ret = 0;
> +unlock:
> +	sdeb_data_write_unlock(sip);
>   out:
>   	if (meta_data_locked)
>   		sdeb_meta_write_unlock(sip);

Re: [PATCH v2 2/7] scsi: scsi_debug: Split resp_write_same()

[John Garry]

On 9/24/26 23:54, Bart Van Assche wrote:
> Since the Clang thread-safety analyzer does not support conditional
> locking, split the function resp_write_same() such that all locking
> statements end up in the same branch of an if-statement.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: John Garry <john.garry@linux.dev>

> ---
>   drivers/scsi/scsi_debug.c | 34 ++++++++++++++++++++++------------
>   1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index ed8d69f305f8..d0d2cbac487b 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5360,8 +5360,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
>   	return ret;
>   }
>   
> -static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
> -			   u32 ei_lba, bool unmap, bool ndob)
> +static int __resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
> +			     u32 ei_lba, bool unmap, bool ndob)
>   {
>   	struct scsi_device *sdp = scp->device;
>   	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
> @@ -5373,20 +5373,14 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>   						scp->device->hostdata, true);
>   	u8 *fs1p;
>   	u8 *fsp;
> -	bool meta_data_locked = false;
> -
> -	if (sdebug_dev_is_zoned(devip) || scsi_debug_lbp()) {
> -		sdeb_meta_write_lock(sip);
> -		meta_data_locked = true;
> -	}
>   
>   	ret = check_device_access_params(scp, lba, num, true);
>   	if (ret)
> -		goto out;
> +		return ret;
>   
>   	if (unmap && scsi_debug_lbp()) {
>   		unmap_region(sip, lba, num);
> -		goto out;
> +		return ret;
>   	}
>   	lbaa = lba;
>   	block = do_div(lbaa, sdebug_store_sectors);
> @@ -5422,9 +5416,25 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>   	ret = 0;
>   unlock:
>   	sdeb_data_write_unlock(sip);
> -out:
> -	if (meta_data_locked)
> +	return ret;
> +}
> +
> +static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num, u32 ei_lba,
> +			   bool unmap, bool ndob)
> +{
> +	struct scsi_device *sdp = scp->device;
> +	struct sdebug_dev_info *devip = sdp->hostdata;
> +	struct sdeb_store_info *sip = devip2sip(devip, true);
> +	int ret;
> +
> +	if (sdebug_dev_is_zoned(devip) || scsi_debug_lbp()) {
> +		sdeb_meta_write_lock(sip);
> +		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob);
>   		sdeb_meta_write_unlock(sip);
> +	} else {
> +		ret = __resp_write_same(scp, lba, num, ei_lba, unmap, ndob);
> +	}
> +
>   	return ret;
>   }
>   

Re: [PATCH v2 3/7] scsi: scsi_debug: Split corrupt_lbas()

[John Garry]

On 9/24/26 23:54, Bart Van Assche wrote:
> Move the inner logic of corrupt_lbas() into a new helper function
> __corrupt_lbas() and call it from separate locked and unlocked branches
> in corrupt_lbas(). This patch prepares for enabling context analysis.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: John Garry <john.garry@linux.dev>

> ---
>   drivers/scsi/scsi_debug.c | 42 ++++++++++++++++++++-------------------
>   1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index d0d2cbac487b..0f4637d1265a 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -4948,39 +4948,26 @@ static int resp_write_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>   	return 0;
>   }
>   
> -static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
> -		u32 nr_bit_errors, s32 reftag_adjust)
> +static int __corrupt_lbas(struct sdeb_store_info *sip, u64 lba, u32 num,
> +			  u32 nr_bit_errors, s32 reftag_adjust)
>   {
> -	struct sdeb_store_info *sip = devip2sip(devip, false);
> -	bool meta_data_locked = false;
>   	u32 block, num_mapped, b, i;
> -	int error = 0;
> -
> -	if (sdebug_dev_is_zoned(devip) ||
> -	    sdebug_dix ||
> -	    scsi_debug_lbp())  {
> -		sdeb_meta_write_lock(sip);
> -		meta_data_locked = true;
> -	}
>   
>   	if (!sip) {
>   		pr_err("can't corrupt with fake_rw\n");
> -		error = -EINVAL;
> -		goto out_unlock;
> +		return -EINVAL;

These checks don't need locking, so could be in corrupt_lbas(). It does 
not make much difference though.

>   	}
>   
>   	if (num > sdebug_capacity || lba > sdebug_capacity - num) {
>   		pr_err("logical blocks out of bounds: %llu:%u", lba, num);
> -		error = -EINVAL;
> -		goto out_unlock;
> +		return -EINVAL;
>   	}
>   
>   	if (scsi_debug_lbp() &&
>   	    (!map_state(sip, lba, &num_mapped) || num > num_mapped)) {
>   		pr_err("can't modify unmapped logical blocks: %llu:%u",
>   			lba, num);
> -		error = -EINVAL;
> -		goto out_unlock;
> +		return -EINVAL;
>   	}
>   
>   	/*
> @@ -5018,9 +5005,24 @@ static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
>   	}
>   	sdeb_data_unlock(sip, false);
>   
> -out_unlock:
> -	if (meta_data_locked)
> +	return 0;
> +}
> +
> +static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
> +			u32 nr_bit_errors, s32 reftag_adjust)
> +{
> +	struct sdeb_store_info *sip = devip2sip(devip, false);
> +	int error;
> +
> +	if (sdebug_dev_is_zoned(devip) || sdebug_dix || scsi_debug_lbp()) {
> +		sdeb_meta_write_lock(sip);
> +		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
> +				       reftag_adjust);
>   		sdeb_meta_write_unlock(sip);
> +	} else {
> +		error = __corrupt_lbas(sip, lba, num, nr_bit_errors,
> +				       reftag_adjust);
> +	}
>   	return error;
>   }
>   

Re: [PATCH v2 4/7] scsi: scsi_debug: Split resp_read_dt0()

[John Garry]

On 9/24/26 23:54, Bart Van Assche wrote:
> +/*
> + * Returns the number of bytes transferred, or -1 with *scsi_status set to a
> + * SCSI status code on failure.
> + */
> +static int __resp_read_dt0(struct scsi_cmnd *scp, struct sdeb_store_info *sip,
> +			   u64 lba, u32 num, u32 ei_lba, int *scsi_status)
> +{
> +	u8 *cmd = scp->cmnd;
> +
> +	/* DIX + T10 DIF */
> +	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
> +		switch (prot_verify_read(scp, lba, num, ei_lba)) {
> +		case 1: /* Guard tag error */
> +			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
> +				mk_sense_buffer(scp, ABORTED_COMMAND,
> +					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
> +				*scsi_status = check_condition_result;
> +				return -1;
> +			} else if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
> +				mk_sense_buffer(scp, ILLEGAL_REQUEST,
> +					LOGICAL_BLOCK_GUARD_CHECK_FAILED);
> +				*scsi_status = illegal_condition_result;
> +				return -1;
> +			}
> +			break;
> +		case 3: /* Reference tag error */
> +			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
> +				mk_sense_buffer(scp, ABORTED_COMMAND,
> +					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
> +				*scsi_status = check_condition_result;
> +				return -1;
> +			} else if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
> +				mk_sense_buffer(scp, ILLEGAL_REQUEST,
> +					LOGICAL_BLOCK_REFERENCE_TAG_CHECK_FAILED);
> +				*scsi_status = illegal_condition_result;
> +				return -1;
> +			}
> +			break;
> +		}
> +	}
> +
> +	*scsi_status = DID_ERROR << 16;

This just seems odd. We are still setting scsi_status = DID_ERROR << 16 
even if no error. I know that it is not checked (for no error), but it 
is a good practice to make it hold a proper value.

Similar could be said how it is initialized in resp_read_dt0().
> +	return do_device_access(sip, scp, 0, lba, num, 0, false, false);
> +}

Re: [PATCH v2 6/7] scsi: scsi_debug: Improve lock context annotations

[John Garry]

On 9/24/26 23:54, Bart Van Assche wrote:
>   static inline void
>   sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
> +	__acquires(&sip->macc_data_lck)

Is this proper?

sdeb_data_lock() can call sdeb_data_read_lock(), and 
sdeb_data_read_lock() uses __acquires_shared().

Indeed, but functions which sdeb_data_lock() call already have lock 
context annotations, so I wonder if this annotation is even needed.

> +	__context_unsafe(/*conditional locking*/)
>   {
>   	if (atomic)
>   		sdeb_data_write_lock(sip);
> @@ -4164,6 +4174,8 @@ sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
>   
>   static inline void
>   sdeb_data_unlock(struct sdeb_store_info *sip, bool atomic)
> +	__releases(&sip->macc_data_lck)

Re: [PATCH v2 7/7] scsi: core: Enable lock context analysis for the scsi_debug driver

[John Garry]

On 9/24/26 23:54, Bart Van Assche wrote:
> Enable lock context analysis now that all lock context analysis warnings
> have been fixed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: John Garry <john.garry@linux.dev>

> ---
>   drivers/scsi/Makefile | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index 533623382eca..32d816e942d6 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -18,6 +18,7 @@ CONTEXT_ANALYSIS_constants.o := y
>   CONTEXT_ANALYSIS_hosts.o := y
>   CONTEXT_ANALYSIS_scsi.o := y
>   CONTEXT_ANALYSIS_scsi_common.o := y
> +CONTEXT_ANALYSIS_scsi_debug.o := y
>   CONTEXT_ANALYSIS_scsi_devinfo.o := y
>   CONTEXT_ANALYSIS_scsi_dh.o := y
>   CONTEXT_ANALYSIS_scsi_error.o := y

Re: [PATCH v2 6/7] scsi: scsi_debug: Improve lock context annotations

[Bart Van Assche]

On 9/25/26 2:15 AM, John Garry wrote:
> On 9/24/26 23:54, Bart Van Assche wrote:
>>   static inline void
>>   sdeb_data_lock(struct sdeb_store_info *sip, bool atomic)
>> +    __acquires(&sip->macc_data_lck)
> 
> Is this proper?
> 
> sdeb_data_lock() can call sdeb_data_read_lock(), and 
> sdeb_data_read_lock() uses __acquires_shared().
> 
> Indeed, but functions which sdeb_data_lock() call already have lock 
> context annotations, so I wonder if this annotation is even needed.
> 
>> +    __context_unsafe(/*conditional locking*/)

Hi John,

Some annotations are needed. If I remove both annotations Clang reports
the following warnings:

drivers/scsi/scsi_debug.c:4168:3: error: rwlock 'sip->macc_data_lck' is 
acquired exclusively and shared in the same scope 
[-Werror,-Wthread-safety-analysis]
  4168 |                 sdeb_data_write_lock(sip);
       |                 ^
drivers/scsi/scsi_debug.c:4170:3: note: the other acquisition of rwlock 
'sip->macc_data_lck' is here
  4170 |                 sdeb_data_read_lock(sip);
       |                 ^
drivers/scsi/scsi_debug.c:4171:1: error: rwlock 'sip->macc_data_lck' is 
still held at the end of function [-Werror,-Wthread-safety-analysis]
  4171 | }
       | ^
drivers/scsi/scsi_debug.c:4168:3: note: rwlock acquired here
  4168 |                 sdeb_data_write_lock(sip);
       |                 ^
drivers/scsi/scsi_debug.c:4177:3: error: releasing rwlock 
'sip->macc_data_lck' that was not held [-Werror,-Wthread-safety-analysis]
  4177 |                 sdeb_data_write_unlock(sip);
       |                 ^
drivers/scsi/scsi_debug.c:4179:3: error: releasing rwlock 
'sip->macc_data_lck' that was not held [-Werror,-Wthread-safety-analysis]
  4179 |                 sdeb_data_read_unlock(sip);
       |                 ^

Thanks,

Bart.