| Message ID | 20210610135833.46663-1-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] virtio_scsi: do not overwrite SCSI status | expand |
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
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
FWIW, I have just bisected a virtio-scsi failure to "scsi: Kill DRIVER_SENSE" and this patch "repairs" the problem.
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 >
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.
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
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);
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(-)