On 2018/08/06 13:51, Douglas Gilbert wrote: > Break out the sense key handling in the sd_done() (response) path into > its own function. Note that the sense key only needs to be inspected > when a SCSI status of Check Condition is returned.
It looks like sd_done_sense() is called for any command that has sense data. Check Condition or not. This should be clarified. Other than that, this looks OK to me. > > > Signed-off-by: Douglas Gilbert <[email protected]> > --- > > No speed up here, just a clean up. There could possibly be a speed > improvement (not observed in tests) from lessening the cache footprint > of the sd_done() function which is on the fast path. > > > drivers/scsi/sd.c | 112 ++++++++++++++++++++++++---------------------- > 1 file changed, 59 insertions(+), 53 deletions(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index b17b8c66881d..4b1402633c36 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -1898,6 +1898,62 @@ static unsigned int sd_completed_bytes(struct > scsi_cmnd *scmd) > return min(good_bytes, transferred); > } > > +/* Helper for scsi_done() when there is a sense buffer */ > +static int sd_done_sense(struct scsi_cmnd *SCpnt, unsigned int good_bytes, > + struct scsi_sense_hdr *sshdrp) > +{ > + struct request *req = SCpnt->request; > + struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk); > + > + switch (sshdrp->sense_key) { > + case HARDWARE_ERROR: > + case MEDIUM_ERROR: > + return sd_completed_bytes(SCpnt); > + case RECOVERED_ERROR: > + return scsi_bufflen(SCpnt); > + case NO_SENSE: > + /* This indicates a false check condition, so ignore it. An > + * unknown amount of data was transferred so treat it as an > + * error. > + */ > + SCpnt->result = 0; > + memset(SCpnt->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE); > + break; > + case ABORTED_COMMAND: > + if (sshdrp->asc == 0x10) /* DIF: Target detected corruption */ > + good_bytes = sd_completed_bytes(SCpnt); > + break; > + case ILLEGAL_REQUEST: > + switch (sshdrp->asc) { > + case 0x10: /* DIX: Host detected corruption */ > + good_bytes = sd_completed_bytes(SCpnt); > + 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 (SCpnt->cmnd[1] & 8) { /* UNMAP */ > + sd_config_discard(sdkp, SD_LBP_DISABLE); > + } else { > + sdkp->device->no_write_same = 1; > + sd_config_write_same(sdkp); > + req->__data_len = blk_rq_bytes(req); > + req->rq_flags |= RQF_QUIET; > + } > + break; > + } > + } > + break; > + default: > + break; > + } > + return good_bytes; > +} > + > /** > * sd_done - bottom half handler: called when the lower level > * driver has completed (successfully or otherwise) a scsi command. > @@ -1964,60 +2020,10 @@ static int sd_done(struct scsi_cmnd *SCpnt) > } > sdkp->medium_access_timed_out = 0; > > - if (driver_byte(result) != DRIVER_SENSE && > - (!sense_valid || sense_deferred)) > - goto out; > + if (unlikely(driver_byte(result) == DRIVER_SENSE || > + (sense_valid && !sense_deferred))) > + good_bytes = sd_done_sense(SCpnt, good_bytes, &sshdr); > > - switch (sshdr.sense_key) { > - case HARDWARE_ERROR: > - case MEDIUM_ERROR: > - good_bytes = sd_completed_bytes(SCpnt); > - break; > - case RECOVERED_ERROR: > - good_bytes = scsi_bufflen(SCpnt); > - break; > - case NO_SENSE: > - /* This indicates a false check condition, so ignore it. An > - * unknown amount of data was transferred so treat it as an > - * error. > - */ > - SCpnt->result = 0; > - memset(SCpnt->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE); > - break; > - case ABORTED_COMMAND: > - if (sshdr.asc == 0x10) /* DIF: Target detected corruption */ > - good_bytes = sd_completed_bytes(SCpnt); > - break; > - case ILLEGAL_REQUEST: > - switch (sshdr.asc) { > - case 0x10: /* DIX: Host detected corruption */ > - good_bytes = sd_completed_bytes(SCpnt); > - 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 (SCpnt->cmnd[1] & 8) { /* UNMAP */ > - sd_config_discard(sdkp, SD_LBP_DISABLE); > - } else { > - sdkp->device->no_write_same = 1; > - sd_config_write_same(sdkp); > - req->__data_len = blk_rq_bytes(req); > - req->rq_flags |= RQF_QUIET; > - } > - break; > - } > - } > - break; > - default: > - break; > - } > - > - out: > if (sd_is_zoned(sdkp)) > sd_zbc_complete(SCpnt, good_bytes, &sshdr); > > -- Damien Le Moal Western Digital Research
