On 28/06/2022 23:21, Bart Van Assche wrote:
For some technologies, e.g. an ATA bus, resuming can take multiple
seconds. Waiting for resume to finish can cause a very noticeable delay.
Hence this patch that restores the behavior from before patch "scsi:
core: pm: Rely on the device driver core for async power management" for
most SCSI devices.
This patch introduces a behavior change: if the START command fails, do
not consider this as a SCSI disk resume failure.
Cc: Ming Lei <ming.lei@xxxxxxxxxx>
Cc: Hannes Reinecke <hare@xxxxxxx>
Cc: John Garry <john.garry@xxxxxxxxxx>
Cc: ericspero@xxxxxxxxxx
Cc: jason600.groome@xxxxxxxxx
Tested-by: jason600.groome@xxxxxxxxx
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215880
Fixes: a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management")
Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
---
drivers/scsi/sd.c | 79 ++++++++++++++++++++++++++++++++++++-----------
drivers/scsi/sd.h | 5 +++
2 files changed, 66 insertions(+), 18 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 895b56c8f25e..06888b675e71 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -103,6 +103,7 @@ static void sd_config_discard(struct scsi_disk *, unsigned int);
static void sd_config_write_same(struct scsi_disk *);
static int sd_revalidate_disk(struct gendisk *);
static void sd_unlock_native_capacity(struct gendisk *disk);
+static void sd_start_done_work(struct work_struct *work);
static int sd_probe(struct device *);
static int sd_remove(struct device *);
static void sd_shutdown(struct device *);
@@ -3463,6 +3464,7 @@ static int sd_probe(struct device *dev)
sdkp->max_retries = SD_MAX_RETRIES;
atomic_set(&sdkp->openers, 0);
atomic_set(&sdkp->device->ioerr_cnt, 0);
+ INIT_WORK(&sdkp->start_done_work, sd_start_done_work);
if (!sdp->request_queue->rq_timeout) {
if (sdp->type != TYPE_MOD)
@@ -3585,12 +3587,64 @@ static void scsi_disk_release(struct device *dev)
kfree(sdkp);
}
+/* Process sense data after a START command finished. */
+static void sd_start_done_work(struct work_struct *work)
+{
+ struct scsi_disk *sdkp = container_of(work, typeof(*sdkp),
+ start_done_work);
+ struct scsi_sense_hdr sshdr;
+ int res = sdkp->start_result;
+
+ if (res == 0)
+ return;
+
+ sd_print_result(sdkp, "Start/Stop Unit failed", res);
+ if (res > 0 && scsi_normalize_sense(sdkp->start_sense_buffer,
+ sdkp->start_sense_len, &sshdr))
+ sd_print_sense_hdr(sdkp, &sshdr);
nit: maybe you can reduce indentation, like:
if (res < 0)
return;
if (scsi_normalize_sense(sdkp->start_sense_buffer,
sdkp->start_sense_len, &sshdr)) {
sd_print_sense_hdr(sdkp, &sshdr);
}
+}
+
+/* A START command finished. May be called from interrupt context. */
+static void sd_start_done(struct request *req, blk_status_t status)
+{
+ const struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
+ struct scsi_disk *sdkp = scsi_disk(req->q->disk);
+
+ sdkp->start_result = scmd->result;
+ WARN_ON_ONCE(scmd->sense_len > SCSI_SENSE_BUFFERSIZE);
If scmd->sense_len > SCSI_SENSE_BUFFERSIZE, do you really want to go on
to copy at sdkp->start_sense_buffer (which is of size
SCSI_SENSE_BUFFERSIZE)? Won't that cause a corruption?
+ sdkp->start_sense_len = scmd->sense_len;
+ memcpy(sdkp->start_sense_buffer, scmd->sense_buffer, scmd->sense_len);
+ WARN_ON_ONCE(!schedule_work(&sdkp->start_done_work));
+}
+
+/* Submit a START command asynchronously. */
+static int sd_submit_start(struct scsi_disk *sdkp, u8 cmd[], u8 cmd_len)
+{
+ struct scsi_device *sdev = sdkp->device;
+ struct request_queue *q = sdev->request_queue;
+ struct request *req;
+ struct scsi_cmnd *scmd;
+
+ req = scsi_alloc_request(q, REQ_OP_DRV_IN, BLK_MQ_REQ_PM);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
+
+ scmd = blk_mq_rq_to_pdu(req);
+ scmd->cmd_len = cmd_len;
+ memcpy(scmd->cmnd, cmd, cmd_len);
+ scmd->allowed = sdkp->max_retries;
+ req->timeout = SD_TIMEOUT;
+ req->rq_flags |= RQF_PM | RQF_QUIET;
+ req->end_io = sd_start_done;
+ blk_execute_rq_nowait(req, /*at_head=*/true);
+
+ return 0;
+}
+
static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
{
unsigned char cmd[6] = { START_STOP }; /* START_VALID */
- struct scsi_sense_hdr sshdr;
struct scsi_device *sdp = sdkp->device;
- int res;
if (start)
cmd[4] |= 1; /* START */
@@ -3601,23 +3655,10 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
if (!scsi_device_online(sdp))
return -ENODEV;
- res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
- SD_TIMEOUT, sdkp->max_retries, 0, RQF_PM, NULL);
- if (res) {
- sd_print_result(sdkp, "Start/Stop Unit failed", res);
- if (res > 0 && scsi_sense_valid(&sshdr)) {
- sd_print_sense_hdr(sdkp, &sshdr);
- /* 0x3a is medium not present */
- if (sshdr.asc == 0x3a)
- res = 0;
- }
- }
+ /* Wait until processing of sense data has finished. */
+ flush_work(&sdkp->start_done_work);
- /* SCSI error codes must not go to the generic layer */
- if (res)
- return -EIO;
-
- return 0;
+ return sd_submit_start(sdkp, cmd, sizeof(cmd));
}
/*
@@ -3644,6 +3685,8 @@ static void sd_shutdown(struct device *dev)
sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
sd_start_stop_device(sdkp, 0);
}
+
+ flush_work(&sdkp->start_done_work);
}
static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 5eea762f84d1..b89187761d61 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -150,6 +150,11 @@ struct scsi_disk {
unsigned urswrz : 1;
unsigned security : 1;
unsigned ignore_medium_access_errors : 1;
+
+ int start_result;
+ u32 start_sense_len;
+ u8 start_sense_buffer[SCSI_SENSE_BUFFERSIZE];
+ struct work_struct start_done_work;
};
#define to_scsi_disk(obj) container_of(obj, struct scsi_disk, disk_dev)
.
