[PATCH 1/2] virtio_scsi: do not overwrite SCSI status

[PATCH 1/2] virtio_scsi: do not overwrite SCSI status

[Hannes Reinecke]

When a sense code is present we should not override the scsi status;
the driver already sets it based on the response from the hypervisor.

Fixes:  464a00c9e0ad ("scsi: core: Kill DRIVER_SENSE")
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/virtio_scsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index fd69a03d6137..43177a62916a 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -161,7 +161,6 @@ 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));
-		set_status_byte(sc, SAM_STAT_CHECK_CONDITION);
 	}
 
 	sc->scsi_done(sc);
-- 
2.26.2

Re: [PATCH 1/2] virtio_scsi: do not overwrite SCSI status

[Jiri Slaby]

On 10. 06. 21, 15:58, Hannes Reinecke wrote:
> When a sense code is present we should not override the scsi status;
> the driver already sets it based on the response from the hypervisor.
> 
> Fixes:  464a00c9e0ad ("scsi: core: Kill DRIVER_SENSE")
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Tested-by: Jiri Slaby <jirislaby@kernel.org>

> ---
>   drivers/scsi/virtio_scsi.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index fd69a03d6137..43177a62916a 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -161,7 +161,6 @@ 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));
> -		set_status_byte(sc, SAM_STAT_CHECK_CONDITION);
>   	}
>   
>   	sc->scsi_done(sc);
> 


-- 
js
suse labs

Re: [PATCH 1/2] virtio_scsi: do not overwrite SCSI status

[Martin K. Petersen]

Hannes,

> When a sense code is present we should not override the scsi status;
> the driver already sets it based on the response from the hypervisor.

Color me confused. The code looks like this:

        if (sc->sense_buffer) {
                memcpy(sc->sense_buffer, resp->sense,
                       min_t(u32,
                             virtio32_to_cpu(vscsi->vdev, resp->sense_len),
                             VIRTIO_SCSI_SENSE_SIZE));
                set_status_byte(sc, SAM_STAT_CHECK_CONDITION);
        }

But sc->sense_buffer is always true, the scsi_cmnd obviously has a sense
buffer.

Shouldn't that be looking at the returned response instead?

	if (resp->sense_len) {
                memcpy(sc->sense_buffer, resp->sense,
                       min_t(u32,
                             virtio32_to_cpu(vscsi->vdev, resp->sense_len),
                             VIRTIO_SCSI_SENSE_SIZE));
                set_status_byte(sc, SAM_STAT_CHECK_CONDITION);
        }

What am I missing?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [Patch 1/2] virtio_scsi: do not overwrite SCSI status

[Christian Borntraeger]

FWIW,
I have just bisected a virtio-scsi failure to "scsi: Kill DRIVER_SENSE"
and this patch "repairs" the problem.

Re: [PATCH 1/2] virtio_scsi: do not overwrite SCSI status

[Guenter Roeck]

On Thu, Jun 10, 2021 at 03:58:33PM +0200, Hannes Reinecke wrote:
> When a sense code is present we should not override the scsi status;
> the driver already sets it based on the response from the hypervisor.
> 
> Fixes:  464a00c9e0ad ("scsi: core: Kill DRIVER_SENSE")
> Signed-off-by: Hannes Reinecke <hare@suse.de>

As may be known by now, scsi-virtio support in linux-next is broken.
The problem bisects to "scsi: core: Kill DRIVER_SENSE", and this patch
fixes the problem.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>  drivers/scsi/virtio_scsi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index fd69a03d6137..43177a62916a 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -161,7 +161,6 @@ 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));
> -		set_status_byte(sc, SAM_STAT_CHECK_CONDITION);
>  	}
>  
>  	sc->scsi_done(sc);
> -- 
> 2.26.2
> 

Re: [Patch 1/2] virtio_scsi: do not overwrite SCSI status

[Christian Borntraeger]

On 18.06.21 14:14, Christian Borntraeger wrote:
> FWIW,
> I have just bisected a virtio-scsi failure to "scsi: Kill DRIVER_SENSE"
> and this patch "repairs" the problem.
> 

Any outlook when this lands in next? Having a broken virtio-scsi in next for 2 weeks now is
kind of painful.

Re: [PATCH 1/2] virtio_scsi: do not overwrite SCSI status

[Hannes Reinecke]

On 6/15/21 4:59 AM, Martin K. Petersen wrote:
> 
> Hannes,
> 
>> When a sense code is present we should not override the scsi status;
>> the driver already sets it based on the response from the hypervisor.
> 
> Color me confused. The code looks like this:
> 
>         if (sc->sense_buffer) {
>                 memcpy(sc->sense_buffer, resp->sense,
>                        min_t(u32,
>                              virtio32_to_cpu(vscsi->vdev, resp->sense_len),
>                              VIRTIO_SCSI_SENSE_SIZE));
>                 set_status_byte(sc, SAM_STAT_CHECK_CONDITION);
>         }
> 
> But sc->sense_buffer is always true, the scsi_cmnd obviously has a sense
> buffer.
> 
> Shouldn't that be looking at the returned response instead?
> 
> 	if (resp->sense_len) {
>                 memcpy(sc->sense_buffer, resp->sense,
>                        min_t(u32,
>                              virtio32_to_cpu(vscsi->vdev, resp->sense_len),
>                              VIRTIO_SCSI_SENSE_SIZE));
>                 set_status_byte(sc, SAM_STAT_CHECK_CONDITION);
>         }
> 
> What am I missing?
> 
Indeed, you are right. Will be fixing it up.

Cheers,

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