Re: [PATCH v10 17/33] scsi: sd: Fix sshdr use in sd_suspend_common

[John Garry <john.g.garry@xxxxxxxxxx>]

> -
>   	for (retries = 3; retries > 0; --retries) {
>   		unsigned char cmd[16] = { 0 };
>   
> @@ -1645,15 +1642,19 @@ static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr *sshdr)
>   			return res;
>   
>   		if (scsi_status_is_check_condition(res) &&
> -		    scsi_sense_valid(sshdr)) {
> -			sd_print_sense_hdr(sdkp, sshdr);
> +		    scsi_sense_valid(&sshdr)) {
> +			sd_print_sense_hdr(sdkp, &sshdr);
>   
>   			/* we need to evaluate the error return  */
> -			if (sshdr->asc == 0x3a ||	/* medium not present */
> -			    sshdr->asc == 0x20 ||	/* invalid command */
> -			    (sshdr->asc == 0x74 && sshdr->ascq == 0x71))	/* drive is password locked */
> +			if (sshdr.asc == 0x3a ||	/* medium not present */
> +			    sshdr.asc == 0x20 ||	/* invalid command */
> +			    (sshdr.asc == 0x74 && sshdr.ascq == 0x71))	/* drive is password locked */
>   				/* this is no error here */
>   				return 0;
> +
> +			/* This drive doesn't support sync. */
> +			if (sshdr.sense_key == ILLEGAL_REQUEST)
> +				return -EOPNOTSUPP;

Only sd_suspend_common() interprets this, right? And in that case, it 
treats as if no error, so could you just return 0 here (and drop the 
-EOPNOTSUPP check in sd_suspend_common())?

Thanks,
John

>   		}
>   
>   		switch (host_byte(res)) {
> @@ -3848,7 +3849,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, NULL);
> +		sd_sync_cache(sdkp);
>   	}
>   
>   	if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
> @@ -3860,7 +3861,6 @@ 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() */
> @@ -3869,21 +3869,18 @@ static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>   	if (sdkp->WCE && sdkp->media_present) {
>   		if (!sdkp->device->silence_suspend)
>   			sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> -		ret = sd_sync_cache(sdkp, &sshdr);
> +		ret = sd_sync_cache(sdkp);
>   
>   		if (ret) {
>   			/* ignore OFFLINE device */
>   			if (ret == -ENODEV)
>   				return 0;
>   
> -			if (!scsi_sense_valid(&sshdr) ||
> -			    sshdr.sense_key != ILLEGAL_REQUEST)
> +			if (ret != -EOPNOTSUPP)
>   				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.
> +			 * The drive doesn't support sync. There's not much to
> +			 * do and suspend shouldn't fail.
>   			 */
>   			ret = 0;
>   		}