Re: [PATCH v4 16/22] usb/msd: Split async packet tracking into data and csw

[Kevin Wolf <kwolf@redhat.com>]

Am 02.05.2025 um 05:30 hat Nicholas Piggin geschrieben:
> The async packet handling logic has places that infer whether the
> async packet is data or CSW, based on context. This is not wrong,
> it just makes the logic easier to follow if they are categorised
> when they are accepted.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  include/hw/usb/msd.h |   5 +-
>  hw/usb/dev-storage.c | 121 +++++++++++++++++++++++++++----------------
>  2 files changed, 79 insertions(+), 47 deletions(-)
> 
> diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
> index f9fd862b529..a40d15f5def 100644
> --- a/include/hw/usb/msd.h
> +++ b/include/hw/usb/msd.h
> @@ -33,8 +33,11 @@ struct MSDState {
>      struct usb_msd_csw csw;
>      SCSIRequest *req;
>      SCSIBus bus;
> +
>      /* For async completion.  */
> -    USBPacket *packet;
> +    USBPacket *data_packet;
> +    USBPacket *csw_in_packet;

This makes the state more complex, because there is a rule here that
isn't explicit in the code: At most one of data_packet or csw_in_packet
can be set at the same time.

Both are quite similar, so most of the patch just duplicates things that
are currently done for s->packet.

Wouldn't it make more sense to have one USBPacket pointer, but some
state that explicitly tells us what we're waiting for, data or the
status? I was thinking of a new bool at first, but on second thoughts,
s->mode looks quite similar to what we need here.

What if we just introduce a new state in the s->mode state machine for
"CSW read in progress" as opposed to USB_MSDM_CSW meaning "expecting the
host to read the CSW next"? Then the cases for which you currently set
s->csw_in_packet would instead transition to this new state, and
usb_msd_command_complete() (which has the only real change in this
patch) could just directly rely on s->mode.

> @@ -395,11 +420,15 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
>  {
>      MSDState *s = USB_STORAGE_DEV(dev);
>  
> -    assert(s->packet == p);
> -    s->packet = NULL;
> -
> -    if (s->req) {
> -        scsi_req_cancel(s->req);
> +    if (p == s->data_packet) {
> +        s->data_packet = NULL;
> +        if (s->req) {
> +            scsi_req_cancel(s->req);
> +        }
> +    } else if (p == s->csw_in_packet) {
> +        s->csw_in_packet = NULL;
> +    } else {
> +        g_assert_not_reached();
>      }
>  }

I think scsi_req_cancel() is required even in csw_in_packet case.
Whether someone already asked for the result doesn't change the state of
the in-flight SCSI request, so we shouldn't try to cancel it in one
case, but not in the other.

Kevin