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 <dgilbert@xxxxxxxxxxxx>
> ---
>
> 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
