[PATCH 0/5] scsi: remove dead code and redundant checks

[PATCH 0/5] scsi: remove dead code and redundant checks

[shao.mingyin]

This series removes checks that the surrounding code already guarantees and
one store that is overwritten a few lines later.  It was found while reading
the SCSI midlayer and the UFS, sd and st drivers.  There is no functional
change intended; every patch is independent and can be applied on its own or
as any subset.

Patch 1 removes a repeated offset check in ufshcd_read_desc_param().  The
function already rejects param_offset >= buff_len before it reaches the
is_kmalloc block, so the inner test can never be true.

Patch 2 drops the "result < SCSI_VPD_HEADER_SIZE" branch in
scsi_get_vpd_size().  scsi_vpd_inquiry() returns the page length plus the
four-byte header, and returns -EIO when the length is zero, so the value is
always at least 5.

Patch 3 removes the first of two identical nr_blocks stores in
sd_setup_atomic_cmnd().  The second store, after the boundary field, wins.

Patch 4 drops two -EOVERFLOW branches in sd_read_capacity().  Neither
read_capacity_10() nor read_capacity_16() returns that value any more.

Patch 5 factors the duplicated end-of-medium test in st.c into a helper.
The similar test in write_behind_check() is intentionally left alone: it has
different remainder handling and only uses EOM to turn -EIO into -ENOSPC.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c

[PATCH 1/5] scsi: ufs: drop the unreachable offset check

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

ufshcd_read_desc_param() rejects param_offset >= buff_len before it
reaches the is_kmalloc block, so the repeated check there can never be
true.  Keep only the memcpy().

No functional change.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index fd52deb72d21..8f89ce52921d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3895,14 +3895,9 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		goto out;
 	}
 
-	if (is_kmalloc) {
-		/* Make sure we don't copy more data than available */
-		if (param_offset >= buff_len)
-			ret = -EINVAL;
-		else
-			memcpy(param_read_buf, &desc_buf[param_offset],
-			       min_t(u32, param_size, buff_len - param_offset));
-	}
+	if (is_kmalloc)
+		memcpy(param_read_buf, &desc_buf[param_offset],
+		       min_t(u32, param_size, buff_len - param_offset));
 out:
 	if (is_kmalloc)
 		kfree(desc_buf);

Re: [PATCH 1/5] scsi: ufs: drop the unreachable offset check

[Bart Van Assche]

On 11/5/26 3:33 AM, shao.mingyin@zte.com.cn wrote:
> ufshcd_read_desc_param() rejects param_offset >= buff_len before it
> reaches the is_kmalloc block, so the repeated check there can never be
> true.  Keep only the memcpy().
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 2/5] scsi: core: drop the dead VPD size check

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

scsi_vpd_inquiry() returns result + 4 where result is the page length
and is non-zero (otherwise it returns -EIO), so the returned value is at
least 5 bytes and the "result < SCSI_VPD_HEADER_SIZE" branch can never be
taken.

No functional change.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 05232ab6366f..fe4d748cab1a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -378,13 +378,6 @@ static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
 	if (result < 0)
 		return 0;
 
-	if (result < SCSI_VPD_HEADER_SIZE) {
-		dev_warn_once(&sdev->sdev_gendev,
-			      "%s: short VPD page 0x%02x length: %d bytes\n",
-			      __func__, page, result);
-		return 0;
-	}
-
 	return result;
 }

Re: [PATCH 2/5] scsi: core: drop the dead VPD size check

[Bart Van Assche]

On 11/5/26 3:33 AM, shao.mingyin@zte.com.cn wrote:
> scsi_vpd_inquiry() returns result + 4 where result is the page length
> and is non-zero (otherwise it returns -EIO), so the returned value is at
> least 5 bytes and the "result < SCSI_VPD_HEADER_SIZE" branch can never be
> taken.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 3/5] scsi: sd: drop the dead nr_blocks store

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

sd_setup_atomic_cmnd() writes nr_blocks to cmnd[12] and then writes it
again a few lines later, after the boundary field.  The first store is
dead; remove it.

No functional change.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index dc45df819c42..6028f9e6cf7d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1408,7 +1408,6 @@ static blk_status_t sd_setup_atomic_cmnd(struct scsi_cmnd *cmd,
 	cmd->cmnd[0]  = WRITE_ATOMIC_16;
 	cmd->cmnd[1]  = flags;
 	put_unaligned_be64(lba, &cmd->cmnd[2]);
-	put_unaligned_be16(nr_blocks, &cmd->cmnd[12]);
 	if (boundary)
 		put_unaligned_be16(nr_blocks, &cmd->cmnd[10]);
 	else

[PATCH 4/5] scsi: sd: drop the dead -EOVERFLOW branches

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

read_capacity_10() and read_capacity_16() no longer return -EOVERFLOW,
so the two branches that check for it are dead.

No functional change.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index dc45df819c42..8474ad579e90 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2916,8 +2916,6 @@ sd_read_capacity(struct scsi_disk *sdkp, struct queue_limits *lim,
 
 	if (sd_try_rc16_first(sdp)) {
 		sector_size = read_capacity_16(sdkp, sdp, lim, buffer);
-		if (sector_size == -EOVERFLOW)
-			goto got_data;
 		if (sector_size == -ENODEV)
 			return;
 		if (sector_size < 0)
@@ -2926,8 +2924,6 @@ sd_read_capacity(struct scsi_disk *sdkp, struct queue_limits *lim,
 			return;
 	} else {
 		sector_size = read_capacity_10(sdkp, sdp, buffer);
-		if (sector_size == -EOVERFLOW)
-			goto got_data;
 		if (sector_size < 0)
 			return;
 		if ((sizeof(sdkp->capacity) > 4) &&

Re: [PATCH 4/5] scsi: sd: drop the dead -EOVERFLOW branches

[Bart Van Assche]

On 11/5/26 3:33 AM, shao.mingyin@zte.com.cn wrote:
> read_capacity_10() and read_capacity_16() no longer return -EOVERFLOW,
> so the two branches that check for it are dead.

Please mention in the patch description which commit removed the
-EOVERFLOW return statements, e.g. as follows: "Commit 72deb455b5ec
("block: remove CONFIG_LBDAF") removed the last -EOVERFLOW return
statements from read_capacity_10() and read_capacity_16()."

Thanks,

Bart.

Re: [PATCH 4/5] scsi: sd: drop the dead -EOVERFLOW branches

[shao.mingyin]

On 11/5/26 09:20, Bart Van Assche wrote:
> Please mention in the patch description which commit removed the
> -EOVERFLOW return statements, e.g. as follows: "Commit 72deb455b5ec
> ("block: remove CONFIG_LBDAF") removed the last -EOVERFLOW return
> statements from read_capacity_10() and read_capacity_16()."

Agreed, I will use that wording.  I checked 72deb455b5ec in the tree: it
removes the two -EOVERFLOW returns and neither function returns that value
any more.

Thanks,
Shao

[PATCH 5/5] scsi: st: factor out the end-of-medium completion test

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

The same six-condition EOM test appears twice, once when deciding that
a write-behind finished early and once for the WRITE FILEMARKS path.
Factor it into a helper.  The related test in write_behind_check() is
intentionally left alone: it has a different remainder handling and
only uses EOM to turn -EIO into -ENOSPC.

No functional change.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index e9d41e3cc2dd..e0ed6b487a20 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -354,6 +354,15 @@ static void st_analyze_sense(struct st_request *SRpnt, struct st_cmdstatus *s)
 }
 
 
+static bool st_eom_ok(struct st_cmdstatus *cmdstatp)
+{
+	return cmdstatp->have_sense && !cmdstatp->deferred &&
+	       (cmdstatp->flags & SENSE_EOM) &&
+	       (cmdstatp->sense_hdr.sense_key == NO_SENSE ||
+		cmdstatp->sense_hdr.sense_key == RECOVERED_ERROR) &&
+	       (!cmdstatp->remainder_valid || cmdstatp->uremainder64 == 0);
+}
+
 /* Convert the result to success code */
 static int st_chk_result(struct scsi_tape *STp, struct st_request * SRpnt)
 {
@@ -815,12 +824,7 @@ static int st_flush_write_buffer(struct scsi_tape * STp)
 		if ((STp->buffer)->syscall_result != 0) {
 			struct st_cmdstatus *cmdstatp = &STp->buffer->cmdstat;
 
-			if (cmdstatp->have_sense && !cmdstatp->deferred &&
-			    (cmdstatp->flags & SENSE_EOM) &&
-			    (cmdstatp->sense_hdr.sense_key == NO_SENSE ||
-			     cmdstatp->sense_hdr.sense_key == RECOVERED_ERROR) &&
-			    (!cmdstatp->remainder_valid ||
-			     cmdstatp->uremainder64 == 0)) { /* All written at EOM early warning */
+			if (st_eom_ok(cmdstatp)) { /* All written at EOM early warning */
 				STp->dirty = 0;
 				(STp->buffer)->buffer_bytes = 0;
 				if (STps->drv_block >= 0)
@@ -1426,11 +1430,7 @@ static int st_flush(struct file *filp, fl_owner_t id)
 		}
 
 		if (STp->buffer->syscall_result == 0 ||
-		    (cmdstatp->have_sense && !cmdstatp->deferred &&
-		     (cmdstatp->flags & SENSE_EOM) &&
-		     (cmdstatp->sense_hdr.sense_key == NO_SENSE ||
-		      cmdstatp->sense_hdr.sense_key == RECOVERED_ERROR) &&
-		     (!cmdstatp->remainder_valid || cmdstatp->uremainder64 == 0))) {
+		    st_eom_ok(cmdstatp)) {
 			/* Write successful at EOM */
 			st_release_request(SRpnt);
 			SRpnt = NULL;

Re: [PATCH 5/5] scsi: st: factor out the end-of-medium completion test

[Bart Van Assche]

On 11/5/26 3:33 AM, shao.mingyin@zte.com.cn wrote:
 > The same six-condition EOM test appears twice, once when deciding that
 > a write-behind finished early and once for the WRITE FILEMARKS path.

The first occurrence is in st_flush_write_buffer(). That function
submits a synchronous WRITE(6) command and has nothing to do with
write-behind. Additionally, I count five conditions instead of six.
Please fix the patch description.

 > +static bool st_eom_ok(struct st_cmdstatus *cmdstatp)

Please declare the argument const and please choose a more descriptive
name, e.g. st_all_written_at_eom(). The "All written at EOM early
warning" comment from st_flush_write_buffer() could be moved above this
function.

Thanks,

Bart.

Re: [PATCH 5/5] scsi: st: factor out the end-of-medium completion test

[shao.mingyin]

On 11/5/26 09:21, Bart Van Assche wrote:
> The first occurrence is in st_flush_write_buffer(). That function
> submits a synchronous WRITE(6) command and has nothing to do with
> write-behind. Additionally, I count five conditions instead of six.
> Please fix the patch description.

You are right on both counts.  I will describe it as the synchronous
WRITE(6) issued by st_flush_write_buffer() and drop the condition count.

> Please declare the argument const and please choose a more descriptive
> name, e.g. st_all_written_at_eom(). The "All written at EOM early
> warning" comment from st_flush_write_buffer() could be moved above this
> function.

Will do: const struct st_cmdstatus *cmdstatp, st_all_written_at_eom(), and
the comment moved above the helper.  v2 will follow.

Thanks,
Shao

Re: [PATCH 0/5] scsi: remove dead code and redundant checks

[Bart Van Assche]

On 11/5/26 3:33 AM, shao.mingyin@zte.com.cn wrote:
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>

A cover letter doesn't need a Signed-off-by.

Thanks,

Bart.

Re: [PATCH 0/5] scsi: remove dead code and redundant checks

[shao.mingyin]

On 11/5/26 09:54, Bart Van Assche wrote:
> A cover letter doesn't need a Signed-off-by.

Right, I will drop it from the v2 cover; the per-patch Signed-off-by lines
stay.  I will resend the whole series as v2 with the changes requested for
patches 4 and 5.

Thanks,
Shao