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@xxxxxxx +49 911 74053 688
SUSE Software Solutions Germany GmbH, 90409 Nürnberg
GF: F. Imendörffer, HRB 36809 (AG Nürnberg)
>From f60b3ab985a555cd623b77f6da95cb094da08d2a Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@xxxxxxx>
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@xxxxxxx>
---
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
