[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 5/5] scsi: st: factor out the end-of-medium completion test

[Laurence Oberman]

On Fri, 2026-11-06 at 15:42 +0800, shao.mingyin@zte.com.cn wrote:
> 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

Hi,

Thanks for the patch.

The refactor looks correct, but I don't think it buys enough to justify
touching this code. The EOM handling in st is mature and subtle around
buffered writes and residuals, and having the condition inline at each
call site, with its comment, makes it clear what "success at EOM" means
in that context. A helper named st_eom_ok() hides that.

Unless this is a prerequisite for a functional fix, I'd prefer to leave
it as is.

NAK for now.

Regards,
Laurence

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

[shao.mingyin]

On Fri, 2026-11-06 at 21:42 +0800, Laurence Oberman wrote:
> The refactor looks correct, but I don't think it buys enough to justify
> touching this code. [...] NAK for now.

Agreed — it is a pure refactor without a functional need, and keeping the
condition inline does carry context.  I am dropping it; v2 will contain
only the four scsi cleanup patches.

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