Re: [PATCH v4] sd: Ignore sync cache failures when not supported

[Ewan D. Milne]

On Thu, 2017-05-11 at 14:34 +0200, Thierry Escande wrote:
> From: Derek Basehore <dbasehore@xxxxxxxxxxxx>
> 
> Some external hard drives don't support the sync command even though the
> hard drive has write cache enabled. In this case, upon suspend request,
> sync cache failures are ignored if the error code in the sense header is
> ILLEGAL_REQUEST. There's not much we can do for these drives, so we
> shouldn't fail to suspend for this error case. The drive may stay
> powered if that's the setup for the port it's plugged into.
> 
> Signed-off-by: Derek Basehore <dbasehore@xxxxxxxxxxxx>
> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxxxxx>
> ---
> 
> v4 changes:
> - Check sense header validity before checking the sense_key field
> - Get rid of both goto statements (the one in the previous patch and the
>   one in the existing code)
> 
> v3 changes:
> - Pass the sense_hdr structure to sd_sync_cache() instead of the
>   lonely sense_key field
> 
> v2 changes:
> - Change sense_key type to u8 in sd_sync_cache()
> 
>  drivers/scsi/sd.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index fcfeddc..823ab8b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1489,17 +1489,21 @@ static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
>  	return retval;
>  }
>  
> -static int sd_sync_cache(struct scsi_disk *sdkp)
> +static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr *sshdr)
>  {
>  	int retries, res;
>  	struct scsi_device *sdp = sdkp->device;
>  	const int timeout = sdp->request_queue->rq_timeout
>  		* SD_FLUSH_TIMEOUT_MULTIPLIER;
> -	struct scsi_sense_hdr sshdr;
> +	struct scsi_sense_hdr my_sshdr;
>  
>  	if (!scsi_device_online(sdp))
>  		return -ENODEV;
>  
> +	/* caller might not be interested in sense, but we need it */
> +	if (!sshdr)
> +		sshdr = &my_sshdr;
> +
>  	for (retries = 3; retries > 0; --retries) {
>  		unsigned char cmd[10] = { 0 };
>  
> @@ -1508,7 +1512,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>  		 * Leave the rest of the command zero to indicate
>  		 * flush everything.
>  		 */
> -		res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
> +		res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, sshdr,
>  				timeout, SD_MAX_RETRIES, 0, RQF_PM, NULL);
>  		if (res == 0)
>  			break;
> @@ -1518,11 +1522,12 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>  		sd_print_result(sdkp, "Synchronize Cache(10) failed", res);
>  
>  		if (driver_byte(res) & DRIVER_SENSE)
> -			sd_print_sense_hdr(sdkp, &sshdr);
> +			sd_print_sense_hdr(sdkp, sshdr);
> +
>  		/* we need to evaluate the error return  */
> -		if (scsi_sense_valid(&sshdr) &&
> -			(sshdr.asc == 0x3a ||	/* medium not present */
> -			 sshdr.asc == 0x20))	/* invalid command */
> +		if (scsi_sense_valid(sshdr) &&
> +			(sshdr->asc == 0x3a ||	/* medium not present */
> +			 sshdr->asc == 0x20))	/* invalid command */
>  				/* this is no error here */
>  				return 0;
>  
> @@ -3323,7 +3328,7 @@ static void sd_shutdown(struct device *dev)
>  
>  	if (sdkp->WCE && sdkp->media_present) {
>  		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> -		sd_sync_cache(sdkp);
> +		sd_sync_cache(sdkp, NULL);
>  	}
>  
>  	if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
> @@ -3335,6 +3340,7 @@ static void sd_shutdown(struct device *dev)
>  static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
> +	struct scsi_sense_hdr sshdr;
>  	int ret = 0;
>  
>  	if (!sdkp)	/* E.g.: runtime suspend following sd_remove() */
> @@ -3342,12 +3348,23 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  
>  	if (sdkp->WCE && sdkp->media_present) {
>  		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> -		ret = sd_sync_cache(sdkp);
> +		ret = sd_sync_cache(sdkp, &sshdr);
> +
>  		if (ret) {
>  			/* ignore OFFLINE device */
>  			if (ret == -ENODEV)
> -				ret = 0;
> -			goto done;
> +				return 0;
> +
> +			if (!scsi_sense_valid(&sshdr) ||
> +			    sshdr.sense_key != ILLEGAL_REQUEST)
> +				return ret;
> +
> +			/*
> +			 * sshdr.sense_key == ILLEGAL_REQUEST means this drive
> +			 * doesn't support sync. There's not much to do and
> +			 * suspend shouldn't fail.
> +			 */
> +			 ret = 0;
>  		}
>  	}
>  
> @@ -3359,7 +3376,6 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  			ret = 0;
>  	}
>  
> -done:
>  	return ret;
>  }
>  

Reviewed-by: Ewan D. Milne <emilne@xxxxxxxxxx>