[v2,3/7] sd: Cleanup sd_done sense data handling

Commit Message

Christoph Hellwig April 24, 2017, 3:35 p.m. UTC

I would just use a switch and kill the op and unmap variables entirely,
e.g. something like the patch below:

---
From: Christoph Hellwig <[e-mail]>
Subject: sd: Cleanup sd_done sense data handling

Use a switch for the sense key, and remove two pointless variables
that are only used once.

Signed-off-by: Christoph Hellwig <[e-mail]>

Comments

Martin K. Petersen April 24, 2017, 11:11 p.m. UTC | #1

Christoph,

> Use a switch for the sense key, and remove two pointless variables
> that are only used once.

> -				if (unmap)

The rationale behind the unmap variable was clarity and avoiding magic
values.

I'm OK with this, however:

> +				if (SCpnt->cmnd[1] & 8) { /* UNMAP */

So I committed with that change.

Damien Le Moal April 25, 2017, 1:54 a.m. UTC | #2

Martin,

On 4/25/17 08:11, Martin K. Petersen wrote:
> 
> Christoph,
> 
>> Use a switch for the sense key, and remove two pointless variables
>> that are only used once.
> 
>> -				if (unmap)
> 
> The rationale behind the unmap variable was clarity and avoiding magic
> values.
> 
> I'm OK with this, however:
> 
>> +				if (SCpnt->cmnd[1] & 8) { /* UNMAP */
> 
> So I committed with that change.

Thank you for the commit.

Just one remark:
You left the "good_bytes = 0;" in the else statement of the
ILLEGAL_REQUEST && asc == 0x24 && command == WRITE_SAME case. Is it
really necessary ?

good_bytes is already set to 0 at the beginning of sd_done() when result
!= 0 and again within the initial switch-case for REQ_OP_DISCARD and
REQ_OP_WRITE_SAME cases, which are the only commands that can lead to
hitting that "else" part in the sense data processing.
I may be missing something, but I think that that assignment is redundant.

Best regards.

Martin K. Petersen April 25, 2017, 5:03 p.m. UTC | #3

Damien,

> good_bytes is already set to 0 at the beginning of sd_done() when
> result != 0 and again within the initial switch-case for
> REQ_OP_DISCARD and REQ_OP_WRITE_SAME cases, which are the only
> commands that can lead to hitting that "else" part in the sense data
> processing.  I may be missing something, but I think that that
> assignment is redundant.

Fixed this up, thanks!

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8cf34a8e3eea..eec695107c99 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1866,8 +1866,6 @@  static int sd_done(struct scsi_cmnd *SCpnt)
 	struct request *req = SCpnt->request;
 	int sense_valid = 0;
 	int sense_deferred = 0;
-	unsigned char op = SCpnt->cmnd[0];
-	unsigned char unmap = SCpnt->cmnd[1] & 8;
 
 	switch (req_op(req)) {
 	case REQ_OP_DISCARD:
@@ -1941,19 +1939,21 @@  static int sd_done(struct scsi_cmnd *SCpnt)
 			good_bytes = sd_completed_bytes(SCpnt);
 		break;
 	case ILLEGAL_REQUEST:
-		if (sshdr.asc == 0x10)  /* DIX: Host detected corruption */
+		switch (sshdr.asc) {
+		case 0x10:	/* DIX: Host detected corruption */
 			good_bytes = sd_completed_bytes(SCpnt);
-		/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
-		if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
-			switch (op) {
+			break;
+		case 0x20:	/* INVALID COMMAND OPCODE */
+		case 0x24:	/* INVALID FIELD IN CDB */
+			switch (SCpnt->cmnd[0]) {
 			case UNMAP:
 				sd_config_discard(sdkp, SD_LBP_DISABLE);
 				break;
 			case WRITE_SAME_16:
 			case WRITE_SAME:
-				if (unmap)
+				if (SCpnt->cmnd[1] & 8) {
 					sd_config_discard(sdkp, SD_LBP_DISABLE);
-				else {
+				} else {
 					sdkp->device->no_write_same = 1;
 					sd_config_write_same(sdkp);
 
@@ -1961,6 +1961,7 @@  static int sd_done(struct scsi_cmnd *SCpnt)
 					req->__data_len = blk_rq_bytes(req);
 					req->rq_flags |= RQF_QUIET;
 				}
+				break;
 			}
 		}
 		break;