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

[Paul Zimmerman <pauldzim@gmail.com>]

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