| Message ID | 20130906004425.GA31998@linux.intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 06-09-2013 4:44, Todd E Brandt wrote: > Part 2 of the hard disk resume optimization patch, this one applies > to the scsi subsystem. Don't give the same name to both patches. In this case, you could prefix the patch name with e.g. "sd: ". > Signed-off-by: Todd Brandt <todd.e.brandt@intel.com> > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com> > drivers/scsi/sd.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 81 insertions(+), 1 deletion(-) > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 86fcf2c..d4bf784 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c [...] > @@ -3137,6 +3138,85 @@ done: > return ret; > } > > +static void sd_resume_async_end(struct request *rq, int error) > +{ > + struct scsi_sense_hdr sshdr; > + struct scsi_disk *sdkp = rq->end_io_data; > + char *sense = rq->sense; > + > + if (error) { > + sd_printk(KERN_WARNING, sdkp, "START FAILED\n"); > + sd_print_result(sdkp, error); > + if (sense && (driver_byte(error) & DRIVER_SENSE)) { > + scsi_normalize_sense(sense, > + SCSI_SENSE_BUFFERSIZE, &sshdr); > + sd_print_sense_hdr(sdkp, &sshdr); > + } > + } else > + sd_printk(KERN_NOTICE, sdkp, "START SUCCESS\n"); According to Documentation/CodingStyle, both arms of the *if* statement should have {} when one arm has them. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-09-05 at 17:44 -0700, Todd E Brandt wrote: > Part 2 of the hard disk resume optimization patch, this one applies > to the scsi subsystem. This is a horrible patch description, it won't tell the next reader anything about what is going on and why. Give the patches two separate summaries and refer back by summary explaining what this one does. Also, having two code paths do the same thing is always a bad idea: someone will update one and not the other. sd_resume() should be come sd_resume_async(); wait for async event James -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thursday, September 05, 2013 05:44:25 PM Todd E Brandt wrote: > Part 2 of the hard disk resume optimization patch, this one applies > to the scsi subsystem. Please update the patch description to say what the patch does (and why). > Signed-off-by: Todd Brandt <todd.e.brandt@intel.com> > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com> > > drivers/scsi/sd.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 81 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 86fcf2c..d4bf784 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -107,6 +107,7 @@ static int sd_remove(struct device *); > static void sd_shutdown(struct device *); > static int sd_suspend(struct device *); > static int sd_resume(struct device *); > +static int sd_resume_async(struct device *); > static void sd_rescan(struct device *); > static int sd_done(struct scsi_cmnd *); > static int sd_eh_action(struct scsi_cmnd *, unsigned char *, int, int); > @@ -484,7 +485,7 @@ static struct class sd_disk_class = { > > static const struct dev_pm_ops sd_pm_ops = { > .suspend = sd_suspend, > - .resume = sd_resume, > + .resume = sd_resume_async, Same comment as with patch #1. When the command execution later fails the error will be now lost (->resume returned 0 already and nothing can be done to fix it). Sorry but this looks like a wrong approach and you should fix the PM layer to do async ->resume when possible instead. > .poweroff = sd_suspend, > .restore = sd_resume, > .runtime_suspend = sd_suspend, > @@ -3137,6 +3138,85 @@ done: > return ret; > } > > +static void sd_resume_async_end(struct request *rq, int error) > +{ > + struct scsi_sense_hdr sshdr; > + struct scsi_disk *sdkp = rq->end_io_data; > + char *sense = rq->sense; > + > + if (error) { > + sd_printk(KERN_WARNING, sdkp, "START FAILED\n"); > + sd_print_result(sdkp, error); > + if (sense && (driver_byte(error) & DRIVER_SENSE)) { > + scsi_normalize_sense(sense, > + SCSI_SENSE_BUFFERSIZE, &sshdr); > + sd_print_sense_hdr(sdkp, &sshdr); > + } > + } else > + sd_printk(KERN_NOTICE, sdkp, "START SUCCESS\n"); > + > + kfree(sense); > + rq->sense = NULL; > + rq->end_io_data = NULL; > + __blk_put_request(rq->q, rq); > + scsi_disk_put(sdkp); > +} > + > +static int sd_resume_async(struct device *dev) > +{ > + unsigned char cmd[6] = { START_STOP }; > + struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev); > + struct request *req; > + char *sense = NULL; > + int ret = 0; > + > + if (!sdkp->device->manage_start_stop) > + goto error; > + > + sd_printk(KERN_NOTICE, sdkp, "Starting disk\n"); > + > + cmd[4] |= 1; > + > + if (sdkp->device->start_stop_pwr_cond) > + cmd[4] |= 1 << 4; /* Active or Standby */ > + > + if (!scsi_device_online(sdkp->device)) { > + ret = -ENODEV; > + goto error; > + } > + > + req = blk_get_request(sdkp->device->request_queue, 0, __GFP_WAIT); > + if (!req) { > + ret = -ENOMEM; > + goto error; > + } > + > + sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO); > + if (!sense) { > + ret = -ENOMEM; > + goto error_sense; > + } > + > + req->cmd_len = COMMAND_SIZE(cmd[0]); > + memcpy(req->cmd, cmd, req->cmd_len); > + req->sense = sense; > + req->sense_len = 0; > + req->retries = SD_MAX_RETRIES; > + req->timeout = SD_TIMEOUT; > + req->cmd_type = REQ_TYPE_BLOCK_PC; > + req->cmd_flags |= REQ_PM | REQ_QUIET | REQ_PREEMPT; > + > + req->end_io_data = sdkp; > + blk_execute_rq_nowait(req->q, NULL, req, 1, sd_resume_async_end); > + return 0; > + > + error_sense: > + __blk_put_request(req->q, req); > + error: > + scsi_disk_put(sdkp); > + return ret; > +} > + > /** > * init_sd - entry point for this driver (both when built in or when > * a module). Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 86fcf2c..d4bf784 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -107,6 +107,7 @@ static int sd_remove(struct device *); static void sd_shutdown(struct device *); static int sd_suspend(struct device *); static int sd_resume(struct device *); +static int sd_resume_async(struct device *); static void sd_rescan(struct device *); static int sd_done(struct scsi_cmnd *); static int sd_eh_action(struct scsi_cmnd *, unsigned char *, int, int); @@ -484,7 +485,7 @@ static struct class sd_disk_class = { static const struct dev_pm_ops sd_pm_ops = { .suspend = sd_suspend, - .resume = sd_resume, + .resume = sd_resume_async, .poweroff = sd_suspend, .restore = sd_resume, .runtime_suspend = sd_suspend, @@ -3137,6 +3138,85 @@ done: return ret; } +static void sd_resume_async_end(struct request *rq, int error) +{ + struct scsi_sense_hdr sshdr; + struct scsi_disk *sdkp = rq->end_io_data; + char *sense = rq->sense; + + if (error) { + sd_printk(KERN_WARNING, sdkp, "START FAILED\n"); + sd_print_result(sdkp, error); + if (sense && (driver_byte(error) & DRIVER_SENSE)) { + scsi_normalize_sense(sense, + SCSI_SENSE_BUFFERSIZE, &sshdr); + sd_print_sense_hdr(sdkp, &sshdr); + } + } else + sd_printk(KERN_NOTICE, sdkp, "START SUCCESS\n"); + + kfree(sense); + rq->sense = NULL; + rq->end_io_data = NULL; + __blk_put_request(rq->q, rq); + scsi_disk_put(sdkp); +} + +static int sd_resume_async(struct device *dev) +{ + unsigned char cmd[6] = { START_STOP }; + struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev); + struct request *req; + char *sense = NULL; + int ret = 0; + + if (!sdkp->device->manage_start_stop) + goto error; + + sd_printk(KERN_NOTICE, sdkp, "Starting disk\n"); + + cmd[4] |= 1; + + if (sdkp->device->start_stop_pwr_cond) + cmd[4] |= 1 << 4; /* Active or Standby */ + + if (!scsi_device_online(sdkp->device)) { + ret = -ENODEV; + goto error; + } + + req = blk_get_request(sdkp->device->request_queue, 0, __GFP_WAIT); + if (!req) { + ret = -ENOMEM; + goto error; + } + + sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO); + if (!sense) { + ret = -ENOMEM; + goto error_sense; + } + + req->cmd_len = COMMAND_SIZE(cmd[0]); + memcpy(req->cmd, cmd, req->cmd_len); + req->sense = sense; + req->sense_len = 0; + req->retries = SD_MAX_RETRIES; + req->timeout = SD_TIMEOUT; + req->cmd_type = REQ_TYPE_BLOCK_PC; + req->cmd_flags |= REQ_PM | REQ_QUIET | REQ_PREEMPT; + + req->end_io_data = sdkp; + blk_execute_rq_nowait(req->q, NULL, req, 1, sd_resume_async_end); + return 0; + + error_sense: + __blk_put_request(req->q, req); + error: + scsi_disk_put(sdkp); + return ret; +} + /** * init_sd - entry point for this driver (both when built in or when * a module).