diff mbox series

[v2,07/10] usb/msd: Add some additional assertions

Message ID 20250411080431.207579-8-npiggin@gmail.com (mailing list archive)
State New
Headers show
Series usb/msd: Permit relaxed ordering of IN packets | expand

Commit Message

Nicholas Piggin April 11, 2025, 8:04 a.m. UTC 
Add more assertions to help verify internal logic.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/usb/dev-storage.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Philippe Mathieu-Daudé April 11, 2025, 10:27 a.m. UTC | #1 
On 11/4/25 10:04, Nicholas Piggin wrote:
> Add more assertions to help verify internal logic.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/usb/dev-storage.c | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index a9d8d4e8618..3b806872587 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -264,13 +264,24 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
>       MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
>       USBPacket *p = s->data_packet;
>   
> -    if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) {
> -        usb_msd_fatal_error(s);
> -        return;
> +    if (s->mode == USB_MSDM_DATAIN) {

Or switch().

> +        if (req->cmd.mode == SCSI_XFER_TO_DEV) {
> +            usb_msd_fatal_error(s);
> +            return;
> +        }
> +    } else if (s->mode == USB_MSDM_DATAOUT) {
> +        if (req->cmd.mode != SCSI_XFER_TO_DEV) {
> +            usb_msd_fatal_error(s);
> +            return;
> +        }
> +    } else {
> +        g_assert_not_reached();
>       }
>   
> +    assert(s->scsi_len == 0);
>       s->scsi_len = len;
>       s->scsi_off = 0;
> +
>       if (p) {
>           usb_msd_copy_data(s, p);
>           p = s->data_packet;
> @@ -288,6 +299,10 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
>   
>       trace_usb_msd_cmd_complete(req->status, req->tag);
>   
> +    g_assert(s->req);
> +    /* The CBW is what starts the SCSI request */
> +    g_assert(s->mode != USB_MSDM_CBW);
> +
>       s->csw.sig = cpu_to_le32(0x53425355);
>       s->csw.tag = cpu_to_le32(req->tag);
>       s->csw.residue = cpu_to_le32(s->data_len);
> @@ -486,7 +501,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
>           trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
>                                    cbw.cmd_len, s->data_len);
>           assert(le32_to_cpu(s->csw.residue) == 0);
> -        s->scsi_len = 0;
> +        assert(s->scsi_len == 0);

Preferably having scsi_len changes in a distinct patch,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>           s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
>                                 cbw.cmd, cbw.cmd_len, NULL);
>           if (s->commandlog) {
diff mbox series

Patch

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index a9d8d4e8618..3b806872587 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -264,13 +264,24 @@  void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
     MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
     USBPacket *p = s->data_packet;
 
-    if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) {
-        usb_msd_fatal_error(s);
-        return;
+    if (s->mode == USB_MSDM_DATAIN) {
+        if (req->cmd.mode == SCSI_XFER_TO_DEV) {
+            usb_msd_fatal_error(s);
+            return;
+        }
+    } else if (s->mode == USB_MSDM_DATAOUT) {
+        if (req->cmd.mode != SCSI_XFER_TO_DEV) {
+            usb_msd_fatal_error(s);
+            return;
+        }
+    } else {
+        g_assert_not_reached();
     }
 
+    assert(s->scsi_len == 0);
     s->scsi_len = len;
     s->scsi_off = 0;
+
     if (p) {
         usb_msd_copy_data(s, p);
         p = s->data_packet;
@@ -288,6 +299,10 @@  void usb_msd_command_complete(SCSIRequest *req, size_t resid)
 
     trace_usb_msd_cmd_complete(req->status, req->tag);
 
+    g_assert(s->req);
+    /* The CBW is what starts the SCSI request */
+    g_assert(s->mode != USB_MSDM_CBW);
+
     s->csw.sig = cpu_to_le32(0x53425355);
     s->csw.tag = cpu_to_le32(req->tag);
     s->csw.residue = cpu_to_le32(s->data_len);
@@ -486,7 +501,7 @@  static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
         trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
                                  cbw.cmd_len, s->data_len);
         assert(le32_to_cpu(s->csw.residue) == 0);
-        s->scsi_len = 0;
+        assert(s->scsi_len == 0);
         s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
                               cbw.cmd, cbw.cmd_len, NULL);
         if (s->commandlog) {