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

Message ID 20210610135833.46663-1-hare@suse.de
State New
Headers show
Series
  • [1/2] virtio_scsi: do not overwrite SCSI status
Related show

Commit Message

Hannes Reinecke June 10, 2021, 1:58 p.m.
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(-)

Comments

Jiri Slaby June 14, 2021, 7:21 a.m. | #1
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
Martin K. Petersen June 15, 2021, 2:59 a.m. | #2
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
Christian Borntraeger June 18, 2021, 12:14 p.m. | #3
FWIW,
I have just bisected a virtio-scsi failure to "scsi: Kill DRIVER_SENSE"
and this patch "repairs" the problem.
Guenter Roeck June 19, 2021, 8:05 p.m. | #4
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

>
Christian Borntraeger June 22, 2021, 8:56 a.m. | #5
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.
Hannes Reinecke June 22, 2021, 9:09 a.m. | #6
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

Patch

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);