Re: [PATCH v2 10/10] usb/msd: Add more tracing

["Nicholas Piggin" <npiggin@gmail.com>]

On Fri Apr 11, 2025 at 8:36 PM AEST, Philippe Mathieu-Daudé wrote:
> On 11/4/25 10:04, Nicholas Piggin wrote:
>> Add tracing for more received packet types, cbw_state changes, and
>> some more SCSI callbacks. These were useful in debugging relaxed
>> packet ordering support.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   hw/usb/dev-storage.c | 23 +++++++++++++++++++++--
>>   hw/usb/trace-events  |  9 ++++++++-
>>   2 files changed, 29 insertions(+), 3 deletions(-)
>
>
>>   static void usb_msd_copy_data(MSDState *s, USBPacket *p)
>>   {
>>       uint32_t len;
>> +
>>       len = p->iov.size - p->actual_length;
>> +
>> +    trace_usb_msd_copy_data(s->req->tag, len);
>> +
>>       if (len > s->scsi_len)
>>           len = s->scsi_len;
>>       usb_packet_copy(p, scsi_req_get_buf(s->req) + s->scsi_off, len);
>> @@ -264,6 +268,8 @@ 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;
>>   
>> +    trace_usb_msd_transfer_data(req->tag, len);
>> +
>>       if (s->cbw_state == USB_MSD_CBW_DATAIN) {
>>           if (req->cmd.mode == SCSI_XFER_TO_DEV) {
>>               usb_msd_fatal_error(s);
>> @@ -324,11 +330,13 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
>>           }
>>           if (s->data_len == 0) {
>>               s->cbw_state = USB_MSD_CBW_CSW;
>> +            trace_usb_msd_cbw_state(s->cbw_state);
>>           }
>>           /* USB_RET_SUCCESS status clears previous ASYNC status */
>>           usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
>>       } else if (s->data_len == 0) {
>>           s->cbw_state = USB_MSD_CBW_CSW;
>> +        trace_usb_msd_cbw_state(s->cbw_state);
>>       }
>
> Maybe helpful to log state transition?
>
>    void usb_msd_cbw_change_state(MSDState *s,
>                                  enum USBMSDCBWState cbw_state)
>    {
>        if (s->cbw_state != cbw_state) {
>            trace_usb_msd_cbw_state(s->cbw_state, cbw_state);
>            s->cbw_state = cbw_state;
>        }
>    }

Yeah that's not a bad idea. I added that. I made a few more tweaks
and added some more trace points too, but nothing major.

>
> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>