Re: Failure prints during format or mounting a usb storage device

[Paul Zimmerman]

On 7/6/20 3:21 PM, Paul Zimmerman wrote:
> On Sat, Jul 4, 2020 at 11:24 AM Paul Zimmerman <pauldzim@gmail.com 
> <mailto:pauldzim@gmail.com>> wrote:
>
>
>
>     On Sat, Jul 4, 2020 at 11:21 AM Sai Pavan Boddu <saipava@xilinx.com 
> <mailto:saipava@xilinx.com>> wrote:
>
>         Hi,____
>
>         __ __
>
>         We are seeing some errors when a usb-storage device is formatted or 
> mounted on the guest. Below is commit I have bisected it.____
>
>         __ __
>
>         **************____
>
>         Errors:____
>
>         __ __
>
>         / # mount /dev/sda /mnt____
>
>         [New Thread 0x7fffd4680700 (LWP 23270)]____
>
>         [   33.258454] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 
> using xhci_hcd____
>
>         [   33.399528] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 
> using xhci_hcd____
>
>         [   33.544621] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 
> using xhci_hcd____
>
>         [   33.560460] sd 2:0:0:0: [sda] tag#0 FAILED Result: 
> hostbyte=DID_ERROR driverbyte=DRIVER_OK____
>
>         [   33.562405] sd 2:0:0:0: [sda] tag#0 CDB: Read(10) 28 00 00 00 10 00 
> 00 00 01 00____
>
>         [   33.563389] blk_update_request: I/O error, dev sda, sector 4096 op 
> 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0____
>
>         / # [Thread 0x7fffd4680700 (LWP 23270) exited]____
>
>         __ __
>
>         ================____
>
>         Bisect commit :____
>
>         __ __
>
>         commit 7ad3d51ebb8a522ffcad391c4bef281245739dde____
>
>         Author: Paul Zimmerman <pauldzim@gmail.com 
> <mailto:pauldzim@gmail.com>>____
>
>         Date:   Wed May 20 16:53:47 2020 -0700____
>
>         __ __
>
>              usb: add short-packet handling to usb-storage driver____
>
>         __ __
>
>              The dwc-hsotg (dwc2) USB host depends on a short packet to____
>
>              indicate the end of an IN transfer. The usb-storage driver____
>
>              currently doesn't provide this, so fix it.____
>
>         __ __
>
>              I have tested this change rather extensively using a PC____
>
>              emulation with xhci, ehci, and uhci controllers, and have____
>
>              not observed any regressions.____
>
>         __ __
>
>              Signed-off-by: Paul Zimmerman <pauldzim@gmail.com 
> <mailto:pauldzim@gmail.com>>____
>
>              Message-id: 20200520235349.21215-6-pauldzim@gmail.com 
> <mailto:20200520235349.21215-6-pauldzim@gmail.com>____
>
>              Signed-off-by: Peter Maydell peter.maydell@linaro.org 
> <mailto:peter.maydell@linaro.org>____
>
>         __ __
>
>         =====================____
>
>         Steps to reproduce:____
>
>          1. x86_64-softmmu/qemu-system-x86_64 -kernel bzImage -nographic -append 
> "console=ttyS0" -m 512M -initrd initramfs.cpio.gz -device qemu-xhci,id=xhci1 
> -drive file=./usb.img,if=none,id=stick____
>          2. Hotplug usb-storage:____
>
>                                          device_add 
> usb-storage,bus=xhci1.0,port=1,id=usbdev1,drive=stick____
>
>          3. Format &  mount the detected device____
>
>         mkfs.vfat -F 32 /dev/sda
>         mount /dev/sda /mnt____
>
>         You can find the similar errors mentioned above at this stage.____
>
>         ____
>
>         Test Environment:____
>
>                 Host:  Ubuntu 16.04 LTS____
>
>                 Guest:  kernel version: 5.4.0 & BusyBox v1.31.1____
>
>         __ __
>
>         Thanks & Regards,____
>
>         Sai Pavan____
>
>         __ __
>
>     I can try to reproduce this on Monday, if no one beats me to it.
>
>
>
> I am able to reproduce this. Despite the errors in dmesg, the drive
> does end up mounting and working OK, which is probably why I didn’t
> spot it during testing. Sai, does the drive work OK for you too
> despite the errors?
>
> Thanks,
> Paul

Gerd, do you know the purpose of the 'short_not_ok' parameter to
usb_packet_setup()? The simple patch below fixes the reported problem,
but I don't know if it could cause some other problems for XHCI.
hcd-ehci, hcd-ohci, hcd-uhci all set the parameter conditionally,
but hcd-xhci never sets it. I don't understand the purpose of the
parameter myself.

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index b330e36fe6..9fb96fdd66 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1614,7 +1614,7 @@ static int xhci_setup_packet(XHCITransfer *xfer)
 
     xhci_xfer_create_sgl(xfer, dir == USB_TOKEN_IN); /* Also sets int_req */
 
     xhci_xfer_create_sgl(xfer, dir == USB_TOKEN_IN); /* Also sets int_req */
     usb_packet_setup(&xfer->packet, dir, ep, xfer->streamid,
-                     xfer->trbs[0].addr, false, xfer->int_req);
+                     xfer->trbs[0].addr, dir == USB_TOKEN_IN, xfer->int_req);
     usb_packet_map(&xfer->packet, &xfer->sgl);
     DPRINTF("xhci: setup packet pid 0x%x addr %d ep %d\n",
             xfer->packet.pid, ep->dev->addr, ep->nr);

Thanks,
Paul