Message-ID: <1435250830.9377.25.camel@perches.com>
Subject: Re: Stop SSD from waiting for "Spinning up disk..."
From: Joe Perches <joe@perches.com>
To: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Jeff Chua <jeff.chua.linux@gmail.com>,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Thu, 25 Jun 2015 09:47:10 -0700
In-Reply-To: <20150625162143.GA27230@khazad-dum.debian.net>
References: <Pine.LNX.4.64.1506221521480.7601@boston.corp.fedex.com>
	 <20150625162143.GA27230@khazad-dum.debian.net>
Content-Type: text/plain; charset="ISO-8859-1"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4947
Lines: 157

On Thu, 2015-06-25 at 13:21 -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 22 Jun 2015, Jeff Chua wrote:
> > There's no need to wait for disk spin-up for USB SSD devices. This patch
> 
> No, you have to, instead, wait for SSD firmware startup.
> 
> And looking at the contents of sd_spinup_disk(), I don't think it is safe to
> just skip it, either.  It would be be better to call it sd_start_device()...
> 
> sd_spinup_disk() should be really fast on anything that properly implements
> TEST_UNIT_READY and returns "ok, I am ready" when it doesn't need further
> waits or START_STOP, etc...
> 
> Anyway, if you get to see the "Spinning up disk..." printk, your unit did
> not report it was ready, and sd_spinup_disk tried to issue a START_STOP
> command to signal it to get ready for real work.
> 
> There's at least one msleep(1000) in the START_STOP path, though.

It might be good to change the msleep(1000) there
to a shorter value like 100ms (or maybe less).

Perhaps something like this:

(with miscellaneous neatening)

o Remove unnecessary '.' continuation printk on delays
o Remove trailing whitespace
o Neaten whitespace and alignment
o Convert int spintime to bool and move for better alignment
o Remove "only do this at boot time" comment
o Remove unnecessary memset casts
o Move int retries declaration to inner loop
---
 drivers/scsi/sd.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3b2fcb4..c59ed65 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1744,22 +1744,20 @@ static void
 sd_spinup_disk(struct scsi_disk *sdkp)
 {
 	unsigned char cmd[10];
+	bool spintime = false;
 	unsigned long spintime_expire = 0;
-	int retries, spintime;
 	unsigned int the_result;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 
-	spintime = 0;
-
-	/* Spin up drives, as required.  Only do this at boot time */
+	/* Spin up drives, as required. */
 	/* Spinup needs to be done for module loads too. */
 	do {
-		retries = 0;
+		int retries = 0;
 
 		do {
 			cmd[0] = TEST_UNIT_READY;
-			memset((void *) &cmd[1], 0, 9);
+			memset(&cmd[1], 0, 9);
 
 			the_result = scsi_execute_req(sdkp->device, cmd,
 						      DMA_NONE, NULL, 0,
@@ -1777,15 +1775,15 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 			if (the_result)
 				sense_valid = scsi_sense_valid(&sshdr);
 			retries++;
-		} while (retries < 3 && 
+		} while (retries < 3 &&
 			 (!scsi_status_is_good(the_result) ||
 			  ((driver_byte(the_result) & DRIVER_SENSE) &&
-			  sense_valid && sshdr.sense_key == UNIT_ATTENTION)));
+			   sense_valid && sshdr.sense_key == UNIT_ATTENTION)));
 
 		if ((driver_byte(the_result) & DRIVER_SENSE) == 0) {
 			/* no sense, TUR either succeeded or failed
 			 * with a status error */
-			if(!spintime && !scsi_status_is_good(the_result)) {
+			if (!spintime && !scsi_status_is_good(the_result)) {
 				sd_print_result(sdkp, "Test Unit Ready failed",
 						the_result);
 			}
@@ -1812,7 +1810,7 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 				sd_printk(KERN_NOTICE, sdkp, "Spinning up disk...");
 				cmd[0] = START_STOP;
 				cmd[1] = 1;	/* Return immediately */
-				memset((void *) &cmd[2], 0, 8);
+				memset(&cmd[2], 0, 8);
 				cmd[4] = 1;	/* Start spin cycle */
 				if (sdkp->device->start_stop_pwr_cond)
 					cmd[4] |= 1 << 4;
@@ -1821,11 +1819,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 						 SD_TIMEOUT, SD_MAX_RETRIES,
 						 NULL);
 				spintime_expire = jiffies + 100 * HZ;
-				spintime = 1;
+				spintime = true;
 			}
-			/* Wait 1 second for next try */
-			msleep(1000);
-			printk(".");
 
 		/*
 		 * Wait for USB flash devices with slow firmware.
@@ -1833,24 +1828,25 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 		 * occur here.  It's characteristic of these devices.
 		 */
 		} else if (sense_valid &&
-				sshdr.sense_key == UNIT_ATTENTION &&
-				sshdr.asc == 0x28) {
+			   sshdr.sense_key == UNIT_ATTENTION &&
+			   sshdr.asc == 0x28) {
 			if (!spintime) {
 				spintime_expire = jiffies + 5 * HZ;
-				spintime = 1;
+				spintime = true;
 			}
-			/* Wait 1 second for next try */
-			msleep(1000);
 		} else {
 			/* we don't understand the sense code, so it's
 			 * probably pointless to loop */
-			if(!spintime) {
+			if (!spintime) {
 				sd_printk(KERN_NOTICE, sdkp, "Unit Not Ready\n");
 				sd_print_sense_hdr(sdkp, &sshdr);
 			}
 			break;
 		}
-				
+
+		/* Wait a bit for next try */
+		msleep(100);
+
 	} while (spintime && time_before_eq(jiffies, spintime_expire));
 
 	if (spintime) {
@@ -1861,7 +1857,6 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 	}
 }
 
-
 /*
  * Determine whether disk supports Data Integrity Field.
  */


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/