Message ID | 20170424153535.GE895@lst.de (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
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.
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.
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!
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;