Re: [PATCH 13/24] scsi: Kill DRIVER_SENSE

[Hannes Reinecke <hare@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

On 6/10/21 12:52 PM, Jiri Slaby wrote:
> On 07. 06. 21, 15:02, Hannes Reinecke wrote:
>> On 6/7/21 2:30 PM, Martin K. Petersen wrote:
>>>
>>> Hannes,
>>>
>>>>> Any ideas?
>>>
>>>>> Can you enable SCSI logging via
>>>>
>>>> scsi.scsi_logging_level=216
>>>>
>>>> on the kernel commandline and send me the output?
>>>
>>> You now effectively set SAM_STAT_CHECK_CONDITION if the scsi_cmnd has a
>>> sense buffer.
>>>
>>> The original code only set DRIVER_SENSE if the adapter response actually
>>> contained sense information:
>>>
>>> @@ -161,8 +161,7 @@ static void virtscsi_complete_cmd(struct
>>> virtio_scsi *vscsi, void *buf)
>>>                         min_t(u32,
>>>                               virtio32_to_cpu(vscsi->vdev,
>>> resp->sense_len),
>>>                               VIRTIO_SCSI_SENSE_SIZE));
>>> -               if (resp->sense_len)
>>> -                       set_driver_byte(sc, DRIVER_SENSE);
>>> +               set_status_byte(sc, SAM_STAT_CHECK_CONDITION);
>>>          }
>>>
>> Oh, I know. But we're checking for a valid sense code during scanning:
>>
>>             if (scsi_status_is_check_condition(result) &&
>>                 scsi_sense_valid(&sshdr)) {
>>
>> so if that makes a difference it would mean that the virtio driver has
>> some stale sense data which then gets copied over.
>> Anyway.
>> Can you test with this patch?
> 
> Yes, that boots, but is somehow sloooow (hard to tell what is causing
> this).
> 
> Anyway, the new print is still there with the patch:
> [   11.549986] sd 0:0:0:0: Power-on or device reset occurred
> 
> Cool; one step further.
Can you check if the attached patch helps, too?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		        Kernel Storage Architect
hare@suse.de			               +49 911 74053 688
SUSE Software Solutions Germany GmbH, 90409 Nürnberg
GF: F. Imendörffer, HRB 36809 (AG Nürnberg)

[-- Attachment #2: 0002-scsi-do-not-assume-CHECK_CONDITION-is-set-for-valid-.patch --]
[-- Type: text/x-patch, Size: 2519 bytes --]

From f60b3ab985a555cd623b77f6da95cb094da08d2a Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Thu, 10 Jun 2021 15:46:56 +0200
Subject: [PATCH 2/2] scsi: do not assume CHECK_CONDITION is set for valid
 sense code

While the standard implies that a sense code should be returned in
response to CHECK CONDITION status, it _might_ be returned on other
status codes, too.
At least, that's what the original code assumed. So it's arguably
wrong to assume we only will have a valid sense code when CHECK
CONDITION is set.

Fixes: 464a00c9e0ad ("scsi: core: Kill DRIVER_SENSE")
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/sd.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 20739072f21a..821bbcfe68c9 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1722,8 +1722,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr *sshdr)
 		if (res < 0)
 			return res;
 
-		if (scsi_status_is_check_condition(res) &&
-		    scsi_sense_valid(sshdr)) {
+		if (scsi_sense_valid(sshdr)) {
 			sd_print_sense_hdr(sdkp, sshdr);
 
 			/* we need to evaluate the error return  */
@@ -1829,10 +1828,10 @@ static int sd_pr_command(struct block_device *bdev, u8 sa,
 	result = scsi_execute_req(sdev, cmd, DMA_TO_DEVICE, &data, sizeof(data),
 			&sshdr, SD_TIMEOUT, sdkp->max_retries, NULL);
 
-	if (scsi_status_is_check_condition(result) &&
-	    scsi_sense_valid(&sshdr)) {
+	if (result) {
 		sdev_printk(KERN_INFO, sdev, "PR command failed: %d\n", result);
-		scsi_print_sense_hdr(sdev, NULL, &sshdr);
+		if (scsi_sense_valid(&sshdr))
+			scsi_print_sense_hdr(sdev, NULL, &sshdr);
 	}
 
 	return result;
@@ -2073,7 +2072,7 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	}
 	sdkp->medium_access_timed_out = 0;
 
-	if (!scsi_status_is_check_condition(result) &&
+	if (result &&
 	    (!sense_valid || sense_deferred))
 		goto out;
 
@@ -2178,10 +2177,9 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 			retries++;
 		} while (retries < 3 &&
 			 (!scsi_status_is_good(the_result) ||
-			  (scsi_status_is_check_condition(the_result) &&
-			  sense_valid && sshdr.sense_key == UNIT_ATTENTION)));
+			  (sense_valid && sshdr.sense_key == UNIT_ATTENTION)));
 
-		if (!scsi_status_is_check_condition(the_result)) {
+		if (the_result < 0 || !sense_valid) {
 			/* no sense, TUR either succeeded or failed
 			 * with a status error */
 			if(!spintime && !scsi_status_is_good(the_result)) {
-- 
2.26.2