diff mbox series

[v3,12/15] scsi: virtio_scsi: Convert to scsi_execute_cmd

Message ID 20221214235001.57267-13-michael.christie@oracle.com
State Superseded
Headers show
Series scsi: Add struct for args to execution functions | expand

Commit Message

Mike Christie Dec. 14, 2022, 11:49 p.m. UTC 
scsi_execute_req is going to be removed. Convert virtio_scsi to
scsi_execute_cmd.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/virtio_scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Dec. 15, 2022, 8:15 a.m. UTC | #1 
The conversion itself looks good, but how the f**k did we end up with
a LLDD calling unquiry below the SCSI midlayer?
Mike Christie Dec. 18, 2022, 9:40 p.m. UTC | #2 
On 12/15/22 2:15 AM, Christoph Hellwig wrote:
> The conversion itself looks good, but how the f**k did we end up with
> a LLDD calling unquiry below the SCSI midlayer?

I don't know how it happened.

It looks like a hack around scsi_scan_host not removing devices.
Going forward, it looks like we can remove the inquiry code by having
scsi_scan_host be able to remove devices that are no longer returned.

However, there's an extra catch because they have that DID_BAD_TARGET
case where qemu returns VIRTIO_SCSI_S_BAD_TARGET if there are no devices
on the host, but other drivers uses that error code for a bunch of
different reasons.

I was thinking to handle the DID_BAD_TARGET use case above and this type
of issue:

https://lore.kernel.org/linux-scsi/CA+PODjqrRzyJnOKoabMOV4EPByNnL1LgTi+QAKENP3NwUq5YCw@mail.gmail.com/

maybe we want to have a driver level BLIST like:

static struct {
        char *driver;
        blist_flags_t flags;
} scsi_static_driver_list[] __initdata = {
        {"virtio_scsi", BLIST_REMOVE_ON_BAD_TARGET},
	{"aacard", BLIST_SHOW_DEV_PQ1},


One other question, can I do this work after the patchset in this email, the
scsi_cmnd retry patches and the actual PR ones? I keep going off track on these
side adventures.
Christoph Hellwig Dec. 23, 2022, 3:57 p.m. UTC | #3 
On Sun, Dec 18, 2022 at 03:40:48PM -0600, Mike Christie wrote:
> It looks like a hack around scsi_scan_host not removing devices.
> Going forward, it looks like we can remove the inquiry code by having
> scsi_scan_host be able to remove devices that are no longer returned.

Yes, that's the place to do it.  I can see arguments for and against
that, but doing it from and LLDD (and including sd.h in the LLDD
implementation!) just doesn't make sense.

> I was thinking to handle the DID_BAD_TARGET use case above and this type
> of issue:
> 
> https://lore.kernel.org/linux-scsi/CA+PODjqrRzyJnOKoabMOV4EPByNnL1LgTi+QAKENP3NwUq5YCw@mail.gmail.com/
> 
> maybe we want to have a driver level BLIST like:

Maybe instead of a blist we just need better way to communicate
this rather than abusing DID_BAD_TARGET?

> One other question, can I do this work after the patchset in this email, the
> scsi_cmnd retry patches and the actual PR ones? I keep going off track on these
> side adventures.

Yes, please.  I think we need to finish this series first.
Mike Christie Dec. 23, 2022, 6:43 p.m. UTC | #4 
On 12/23/22 9:57 AM, Christoph Hellwig wrote:
> On Sun, Dec 18, 2022 at 03:40:48PM -0600, Mike Christie wrote:
>> It looks like a hack around scsi_scan_host not removing devices.
>> Going forward, it looks like we can remove the inquiry code by having
>> scsi_scan_host be able to remove devices that are no longer returned.
> 
> Yes, that's the place to do it.  I can see arguments for and against
> that, but doing it from and LLDD (and including sd.h in the LLDD
> implementation!) just doesn't make sense.
> 
>> I was thinking to handle the DID_BAD_TARGET use case above and this type
>> of issue:
>>
>> https://lore.kernel.org/linux-scsi/CA+PODjqrRzyJnOKoabMOV4EPByNnL1LgTi+QAKENP3NwUq5YCw@mail.gmail.com/
>>
>> maybe we want to have a driver level BLIST like:
> 
> Maybe instead of a blist we just need better way to communicate
> this rather than abusing DID_BAD_TARGET?

Yeah, after I sent the blist email I looked into qemu and the DID_BAD_TARGET uses
more and we can do more what you are thinking.

I'm going to do:

0. qemu only uses VIRTIO_SCSI_S_BAD_TARGET (this gets converted to DID_BAD_TARGET)
when the device does not exist, or if you are using a sg device and it returns
DID_BAD_TARGET.

1. Most driver uses of DID_BAD_TARGET are when the device is missing, disconnected,
timedout, or the bus:target_id:LUN value was not supported by the driver. So I was going
to rename DID_BAD_TARGET to DID_INVALID_DEVICE/DID_DEVICE_INACCESSIBLE for those cases.

2. Create a new DID_ error code for the other use cases where the driver meant that the
target did something incorrect/invalid like send a invalid response.

In the scan code then we know if we got a DID_INVALID_DEVICE the device is not there
anymore so we can remove it.
diff mbox series

Patch

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index d07d24c06b54..b221c3c99320 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -347,8 +347,8 @@  static void virtscsi_rescan_hotunplug(struct virtio_scsi *vscsi)
 
 		memset(inq_result, 0, inq_result_len);
 
-		result = scsi_execute_req(sdev, scsi_cmd, DMA_FROM_DEVICE,
-					  inq_result, inquiry_len, NULL,
+		result = scsi_execute_cmd(sdev, scsi_cmd, REQ_OP_DRV_IN,
+					  inq_result, inquiry_len,
 					  SD_TIMEOUT, SD_MAX_RETRIES, NULL);
 
 		if (result == 0 && inq_result[0] >> 5) {