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 <[email protected]>
> ---
> 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
