diff mbox series

[v2,08/10] usb/msd: Rename mode to cbw_state, and tweak names

Message ID 20250411080431.207579-9-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
This reflects a little better what it does, particularly with a
subsequent change to relax the order packets are seen in. This
field is not the general state of the MSD state machine, rather
it follows packets that are completed as part of a CBW command.

The difference is a bit subtle, so for a concrete example, the
next change will permit the host to send a CSW packet before it
sends the associated CBW packet. In that case the CSW packet
will be tracked and the MSD state machine will move, but this
mode / cbw_state field would remain unchanged (in the "expecting
CBW" state), until the CBW packet arrives.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/hw/usb/msd.h | 12 +++++------
 hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
 2 files changed, 32 insertions(+), 30 deletions(-)

Comments

Philippe Mathieu-Daudé April 11, 2025, 10:28 a.m. UTC | #1
On 11/4/25 10:04, Nicholas Piggin wrote:
> This reflects a little better what it does, particularly with a
> subsequent change to relax the order packets are seen in. This
> field is not the general state of the MSD state machine, rather
> it follows packets that are completed as part of a CBW command.
> 
> The difference is a bit subtle, so for a concrete example, the
> next change will permit the host to send a CSW packet before it
> sends the associated CBW packet. In that case the CSW packet
> will be tracked and the MSD state machine will move, but this
> mode / cbw_state field would remain unchanged (in the "expecting
> CBW" state), until the CBW packet arrives.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   include/hw/usb/msd.h | 12 +++++------
>   hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
>   2 files changed, 32 insertions(+), 30 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Philippe Mathieu-Daudé April 11, 2025, 10:37 a.m. UTC | #2
On 11/4/25 10:04, Nicholas Piggin wrote:
> This reflects a little better what it does, particularly with a
> subsequent change to relax the order packets are seen in. This
> field is not the general state of the MSD state machine, rather
> it follows packets that are completed as part of a CBW command.
> 
> The difference is a bit subtle, so for a concrete example, the
> next change will permit the host to send a CSW packet before it
> sends the associated CBW packet. In that case the CSW packet
> will be tracked and the MSD state machine will move, but this
> mode / cbw_state field would remain unchanged (in the "expecting
> CBW" state), until the CBW packet arrives.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   include/hw/usb/msd.h | 12 +++++------
>   hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
>   2 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
> index a40d15f5def..c109544f632 100644
> --- a/include/hw/usb/msd.h
> +++ b/include/hw/usb/msd.h
> @@ -10,11 +10,11 @@
>   #include "hw/usb.h"
>   #include "hw/scsi/scsi.h"
>   
> -enum USBMSDMode {
> -    USB_MSDM_CBW, /* Command Block.  */
> -    USB_MSDM_DATAOUT, /* Transfer data to device.  */
> -    USB_MSDM_DATAIN, /* Transfer data from device.  */
> -    USB_MSDM_CSW /* Command Status.  */

Since modifying this, please add

   typedef

> +enum USBMSDCBWState {
> +    USB_MSD_CBW_NONE,    /* Ready, waiting for CBW packet. */
> +    USB_MSD_CBW_DATAOUT, /* Expecting DATA-OUT (to device) packet */
> +    USB_MSD_CBW_DATAIN,  /* Expecting DATA-IN (from device) packet */
> +    USB_MSD_CBW_CSW      /* No more data, expecting CSW packet.  */
>   }

       USBMSDCBWState;

>   
>   struct QEMU_PACKED usb_msd_csw {
> @@ -26,7 +26,7 @@ struct QEMU_PACKED usb_msd_csw {
>   
>   struct MSDState {
>       USBDevice dev;
> -    enum USBMSDMode mode;
> +    enum USBMSDCBWState cbw_state;

        USBMSDCBWState cbw_state;

>       uint32_t scsi_off;
>       uint32_t scsi_len;
>       uint32_t data_len;
Nicholas Piggin April 12, 2025, 5:33 a.m. UTC | #3
On Fri Apr 11, 2025 at 8:37 PM AEST, Philippe Mathieu-Daudé wrote:
> On 11/4/25 10:04, Nicholas Piggin wrote:
>> This reflects a little better what it does, particularly with a
>> subsequent change to relax the order packets are seen in. This
>> field is not the general state of the MSD state machine, rather
>> it follows packets that are completed as part of a CBW command.
>> 
>> The difference is a bit subtle, so for a concrete example, the
>> next change will permit the host to send a CSW packet before it
>> sends the associated CBW packet. In that case the CSW packet
>> will be tracked and the MSD state machine will move, but this
>> mode / cbw_state field would remain unchanged (in the "expecting
>> CBW" state), until the CBW packet arrives.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   include/hw/usb/msd.h | 12 +++++------
>>   hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
>>   2 files changed, 32 insertions(+), 30 deletions(-)
>> 
>> diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
>> index a40d15f5def..c109544f632 100644
>> --- a/include/hw/usb/msd.h
>> +++ b/include/hw/usb/msd.h
>> @@ -10,11 +10,11 @@
>>   #include "hw/usb.h"
>>   #include "hw/scsi/scsi.h"
>>   
>> -enum USBMSDMode {
>> -    USB_MSDM_CBW, /* Command Block.  */
>> -    USB_MSDM_DATAOUT, /* Transfer data to device.  */
>> -    USB_MSDM_DATAIN, /* Transfer data from device.  */
>> -    USB_MSDM_CSW /* Command Status.  */
>
> Since modifying this, please add
>
>    typedef

Done.

Thanks,
Nick

>
>> +enum USBMSDCBWState {
>> +    USB_MSD_CBW_NONE,    /* Ready, waiting for CBW packet. */
>> +    USB_MSD_CBW_DATAOUT, /* Expecting DATA-OUT (to device) packet */
>> +    USB_MSD_CBW_DATAIN,  /* Expecting DATA-IN (from device) packet */
>> +    USB_MSD_CBW_CSW      /* No more data, expecting CSW packet.  */
>>   }
>
>        USBMSDCBWState;
>
>>   
>>   struct QEMU_PACKED usb_msd_csw {
>> @@ -26,7 +26,7 @@ struct QEMU_PACKED usb_msd_csw {
>>   
>>   struct MSDState {
>>       USBDevice dev;
>> -    enum USBMSDMode mode;
>> +    enum USBMSDCBWState cbw_state;
>
>         USBMSDCBWState cbw_state;
>
>>       uint32_t scsi_off;
>>       uint32_t scsi_len;
>>       uint32_t data_len;
diff mbox series

Patch

diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index a40d15f5def..c109544f632 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -10,11 +10,11 @@ 
 #include "hw/usb.h"
 #include "hw/scsi/scsi.h"
 
-enum USBMSDMode {
-    USB_MSDM_CBW, /* Command Block.  */
-    USB_MSDM_DATAOUT, /* Transfer data to device.  */
-    USB_MSDM_DATAIN, /* Transfer data from device.  */
-    USB_MSDM_CSW /* Command Status.  */
+enum USBMSDCBWState {
+    USB_MSD_CBW_NONE,    /* Ready, waiting for CBW packet. */
+    USB_MSD_CBW_DATAOUT, /* Expecting DATA-OUT (to device) packet */
+    USB_MSD_CBW_DATAIN,  /* Expecting DATA-IN (from device) packet */
+    USB_MSD_CBW_CSW      /* No more data, expecting CSW packet.  */
 };
 
 struct QEMU_PACKED usb_msd_csw {
@@ -26,7 +26,7 @@  struct QEMU_PACKED usb_msd_csw {
 
 struct MSDState {
     USBDevice dev;
-    enum USBMSDMode mode;
+    enum USBMSDCBWState cbw_state;
     uint32_t scsi_off;
     uint32_t scsi_len;
     uint32_t data_len;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 3b806872587..ed6d9b70b96 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -264,12 +264,12 @@  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_DATAIN) {
+    if (s->cbw_state == USB_MSD_CBW_DATAIN) {
         if (req->cmd.mode == SCSI_XFER_TO_DEV) {
             usb_msd_fatal_error(s);
             return;
         }
-    } else if (s->mode == USB_MSDM_DATAOUT) {
+    } else if (s->cbw_state == USB_MSD_CBW_DATAOUT) {
         if (req->cmd.mode != SCSI_XFER_TO_DEV) {
             usb_msd_fatal_error(s);
             return;
@@ -301,7 +301,7 @@  void usb_msd_command_complete(SCSIRequest *req, size_t resid)
 
     g_assert(s->req);
     /* The CBW is what starts the SCSI request */
-    g_assert(s->mode != USB_MSDM_CBW);
+    g_assert(s->cbw_state != USB_MSD_CBW_NONE);
 
     s->csw.sig = cpu_to_le32(0x53425355);
     s->csw.tag = cpu_to_le32(req->tag);
@@ -312,7 +312,8 @@  void usb_msd_command_complete(SCSIRequest *req, size_t resid)
     s->req = NULL;
 
     if (p) {
-        g_assert(s->mode == USB_MSDM_DATAIN || s->mode == USB_MSDM_DATAOUT);
+        g_assert(s->cbw_state == USB_MSD_CBW_DATAIN ||
+                 s->cbw_state == USB_MSD_CBW_DATAOUT);
         if (s->data_len) {
             int len = (p->iov.size - p->actual_length);
             usb_packet_skip(p, len);
@@ -322,19 +323,19 @@  void usb_msd_command_complete(SCSIRequest *req, size_t resid)
             s->data_len -= len;
         }
         if (s->data_len == 0) {
-            s->mode = USB_MSDM_CSW;
+            s->cbw_state = USB_MSD_CBW_CSW;
         }
         /* USB_RET_SUCCESS status clears previous ASYNC status */
         usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
     } else if (s->data_len == 0) {
-        s->mode = USB_MSDM_CSW;
+        s->cbw_state = USB_MSD_CBW_CSW;
     }
 
-    if (s->mode == USB_MSDM_CSW) {
+    if (s->cbw_state == USB_MSD_CBW_CSW) {
         p = s->csw_in_packet;
         if (p) {
             usb_msd_send_status(s, p);
-            s->mode = USB_MSDM_CBW;
+            s->cbw_state = USB_MSD_CBW_NONE;
             /* USB_RET_SUCCESS status clears previous ASYNC status */
             usb_msd_csw_packet_complete(s, USB_RET_SUCCESS);
         }
@@ -377,7 +378,7 @@  void usb_msd_handle_reset(USBDevice *dev)
     }
 
     memset(&s->csw, 0, sizeof(s->csw));
-    s->mode = USB_MSDM_CBW;
+    s->cbw_state = USB_MSD_CBW_NONE;
 
     s->needs_reset = false;
 }
@@ -478,8 +479,8 @@  static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
     SCSIDevice *scsi_dev;
     int len;
 
-    switch (s->mode) {
-    case USB_MSDM_CBW:
+    switch (s->cbw_state) {
+    case USB_MSD_CBW_NONE:
         if (!try_get_valid_cbw(p, &cbw)) {
             goto fail;
         }
@@ -492,11 +493,11 @@  static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
         tag = le32_to_cpu(cbw.tag);
         s->data_len = le32_to_cpu(cbw.data_len);
         if (s->data_len == 0) {
-            s->mode = USB_MSDM_CSW;
+            s->cbw_state = USB_MSD_CBW_CSW;
         } else if (cbw.flags & 0x80) {
-            s->mode = USB_MSDM_DATAIN;
+            s->cbw_state = USB_MSD_CBW_DATAIN;
         } else {
-            s->mode = USB_MSDM_DATAOUT;
+            s->cbw_state = USB_MSD_CBW_DATAOUT;
         }
         trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
                                  cbw.cmd_len, s->data_len);
@@ -513,7 +514,7 @@  static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
         }
         break;
 
-    case USB_MSDM_DATAOUT:
+    case USB_MSD_CBW_DATAOUT:
         trace_usb_msd_data_out(p->iov.size, s->data_len);
         if (p->iov.size > s->data_len) {
             goto fail;
@@ -531,7 +532,7 @@  static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
                 }
                 s->data_len -= len;
                 if (s->data_len == 0) {
-                    s->mode = USB_MSDM_CSW;
+                    s->cbw_state = USB_MSD_CBW_CSW;
                 }
             }
         }
@@ -556,8 +557,8 @@  static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
     MSDState *s = (MSDState *)dev;
     int len;
 
-    switch (s->mode) {
-    case USB_MSDM_DATAOUT:
+    switch (s->cbw_state) {
+    case USB_MSD_CBW_DATAOUT:
         if (!check_valid_csw(p)) {
             goto fail;
         }
@@ -573,7 +574,7 @@  static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
         p->status = USB_RET_ASYNC;
         break;
 
-    case USB_MSDM_CSW:
+    case USB_MSD_CBW_CSW:
         if (!check_valid_csw(p)) {
             goto fail;
         }
@@ -585,11 +586,11 @@  static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
             p->status = USB_RET_ASYNC;
         } else {
             usb_msd_send_status(s, p);
-            s->mode = USB_MSDM_CBW;
+            s->cbw_state = USB_MSD_CBW_NONE;
         }
         break;
 
-    case USB_MSDM_DATAIN:
+    case USB_MSD_CBW_DATAIN:
         trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
         if (s->scsi_len) {
             usb_msd_copy_data(s, p);
@@ -603,11 +604,12 @@  static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
                 }
                 s->data_len -= len;
                 if (s->data_len == 0) {
-                    s->mode = USB_MSDM_CSW;
+                    s->cbw_state = USB_MSD_CBW_CSW;
                 }
             }
         }
-        if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
+        if (p->actual_length < p->iov.size &&
+                s->cbw_state == USB_MSD_CBW_DATAIN) {
             trace_usb_msd_packet_async();
             s->data_packet = p;
             p->status = USB_RET_ASYNC;
@@ -672,7 +674,7 @@  static const VMStateDescription vmstate_usb_msd = {
     .minimum_version_id = 1,
     .fields = (const VMStateField[]) {
         VMSTATE_USB_DEVICE(dev, MSDState),
-        VMSTATE_UINT32(mode, MSDState),
+        VMSTATE_UINT32(cbw_state, MSDState),
         VMSTATE_UINT32(scsi_len, MSDState),
         VMSTATE_UINT32(scsi_off, MSDState),
         VMSTATE_UINT32(data_len, MSDState),