* [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS @ 2021-07-02 7:13 Bjørn Mork 2021-07-02 8:57 ` kernel test robot 2021-07-02 12:21 ` kernel test robot 0 siblings, 2 replies; 11+ messages in thread From: Bjørn Mork @ 2021-07-02 7:13 UTC (permalink / raw) To: Mathias Nyman; +Cc: linux-usb, Jonathan Bell, stable, Bjørn Mork From: Jonathan Bell <jonathan@raspberrypi.org> Seen on a VLI VL805 PCIe to USB controller. For non-stream endpoints at least, if the xHC halts on a particular TRB due to an error then the DCS field in the Out Endpoint Context maintained by the hardware is not updated with the current cycle state. Using the quirk XHCI_EP_CTX_BROKEN_DCS and instead fetch the DCS bit from the TRB that the xHC stopped on. Cc: stable@vger.kernel.org Link: https://github.com/raspberrypi/linux/issues/3060 Signed-off-by: Jonathan Bell <jonathan@raspberrypi.org> Signed-off-by: Bjørn Mork <bjorn@mork.no> --- Ran into this issue on an RPi4 running Debian bullseye, having mostly a plain v5.10.40 kernel. Using an RTL2838 (0bda:2838) with rtl-sdr just did not work, showing all the issues described on the above link. This quirk found in https://github.com/raspberrypi/linux.git solves the problem for me. I don't see why it shouldn't be in mainline. And I propose adding it to stable as well, since it solves a real problem. Mostly for my own convenience as I'd prefer just using a Debian built kernel ;-) Did not check this submission with Jonathan - hoping it is OK... Bjørn drivers/usb/host/xhci-pci.c | 4 +++- drivers/usb/host/xhci-ring.c | 26 ++++++++++++++++++++++++++ drivers/usb/host/xhci.h | 1 + 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 18c2bbddf080..6f3bed09028c 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -279,8 +279,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) pdev->device == 0x3432) xhci->quirks |= XHCI_BROKEN_STREAMS; - if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483) + if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483) { xhci->quirks |= XHCI_LPM_SUPPORT; + xhci->quirks |= XHCI_EP_CTX_BROKEN_DCS; + } if (pdev->vendor == PCI_VENDOR_ID_ASMEDIA && pdev->device == PCI_DEVICE_ID_ASMEDIA_1042_XHCI) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 8fea44bbc266..a9c860ff5177 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -559,8 +559,11 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, struct xhci_ring *ep_ring; struct xhci_command *cmd; struct xhci_segment *new_seg; + struct xhci_segment *halted_seg = NULL; union xhci_trb *new_deq; int new_cycle; + union xhci_trb *halted_trb; + int index = 0; dma_addr_t addr; u64 hw_dequeue; bool cycle_found = false; @@ -600,6 +603,29 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, new_deq = ep_ring->dequeue; new_cycle = hw_dequeue & 0x1; + /* + * Quirk: xHC write-back of the DCS field in the hardware dequeue + * pointer is wrong - use the cycle state of the TRB pointed to by + * the dequeue pointer. + */ + if (xhci->quirks & XHCI_EP_CTX_BROKEN_DCS && + !(ep->ep_state & EP_HAS_STREAMS)) + halted_seg = trb_in_td(xhci, cur_td->start_seg, + cur_td->first_trb, cur_td->last_trb, + hw_dequeue & ~0xf, false); + if (halted_seg) { + index = ((dma_addr_t)(hw_dequeue & ~0xf) - halted_seg->dma) / + sizeof(*halted_trb); + halted_trb = &halted_seg->trbs[index]; + state->new_cycle_state = halted_trb->generic.field[3] & 0x1; + xhci_dbg(xhci, "Endpoint DCS = %d TRB index = %d cycle = %d\n", + (u8)(hw_dequeue & 0x1), index, + state->new_cycle_state); + } else { + state->new_cycle_state = hw_dequeue & 0x1; + } + state->stream_id = stream_id; + /* * We want to find the pointer, segment and cycle state of the new trb * (the one after current TD's last_trb). We know the cycle state at diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 3c7d281672ae..911aeb7d8a19 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1896,6 +1896,7 @@ struct xhci_hcd { #define XHCI_SG_TRB_CACHE_SIZE_QUIRK BIT_ULL(39) #define XHCI_NO_SOFT_RETRY BIT_ULL(40) #define XHCI_BROKEN_D3COLD BIT_ULL(41) +#define XHCI_EP_CTX_BROKEN_DCS BIT_ULL(42) unsigned int num_active_eps; unsigned int limit_active_eps; -- 2.30.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS 2021-07-02 7:13 [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS Bjørn Mork @ 2021-07-02 8:57 ` kernel test robot 2021-07-02 12:21 ` kernel test robot 1 sibling, 0 replies; 11+ messages in thread From: kernel test robot @ 2021-07-02 8:57 UTC (permalink / raw) To: Bjørn Mork, Mathias Nyman Cc: kbuild-all, linux-usb, Jonathan Bell, stable, Bjørn Mork [-- Attachment #1: Type: text/plain, Size: 8537 bytes --] Hi "Bjørn, Thank you for the patch! Yet something to improve: [auto build test ERROR on usb/usb-testing] [also build test ERROR on v5.13 next-20210701] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Bj-rn-Mork/xhci-add-quirk-for-host-controllers-that-don-t-update-endpoint-DCS/20210702-151445 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing config: riscv-randconfig-r012-20210702 (attached as .config) compiler: riscv32-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/61088f366a5c42caf8ce20c87355b61efc1b175d git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Bj-rn-Mork/xhci-add-quirk-for-host-controllers-that-don-t-update-endpoint-DCS/20210702-151445 git checkout 61088f366a5c42caf8ce20c87355b61efc1b175d # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash drivers/usb/host/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): drivers/usb/host/xhci-ring.c: In function 'xhci_move_dequeue_past_td': >> drivers/usb/host/xhci-ring.c:613:32: error: 'cur_td' undeclared (first use in this function) 613 | halted_seg = trb_in_td(xhci, cur_td->start_seg, | ^~~~~~ drivers/usb/host/xhci-ring.c:613:32: note: each undeclared identifier is reported only once for each function it appears in >> drivers/usb/host/xhci-ring.c:620:3: error: 'state' undeclared (first use in this function); did you mean 'statx'? 620 | state->new_cycle_state = halted_trb->generic.field[3] & 0x1; | ^~~~~ | statx Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for LOCKDEP Depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && (FRAME_POINTER || MIPS || PPC || S390 || MICROBLAZE || ARM || ARC || X86) Selected by - DEBUG_LOCK_ALLOC && DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT vim +/cur_td +613 drivers/usb/host/xhci-ring.c 552 553 static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, 554 unsigned int slot_id, unsigned int ep_index, 555 unsigned int stream_id, struct xhci_td *td) 556 { 557 struct xhci_virt_device *dev = xhci->devs[slot_id]; 558 struct xhci_virt_ep *ep = &dev->eps[ep_index]; 559 struct xhci_ring *ep_ring; 560 struct xhci_command *cmd; 561 struct xhci_segment *new_seg; 562 struct xhci_segment *halted_seg = NULL; 563 union xhci_trb *new_deq; 564 int new_cycle; 565 union xhci_trb *halted_trb; 566 int index = 0; 567 dma_addr_t addr; 568 u64 hw_dequeue; 569 bool cycle_found = false; 570 bool td_last_trb_found = false; 571 u32 trb_sct = 0; 572 int ret; 573 574 ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id, 575 ep_index, stream_id); 576 if (!ep_ring) { 577 xhci_warn(xhci, "WARN can't find new dequeue, invalid stream ID %u\n", 578 stream_id); 579 return -ENODEV; 580 } 581 /* 582 * A cancelled TD can complete with a stall if HW cached the trb. 583 * In this case driver can't find td, but if the ring is empty we 584 * can move the dequeue pointer to the current enqueue position. 585 * We shouldn't hit this anymore as cached cancelled TRBs are given back 586 * after clearing the cache, but be on the safe side and keep it anyway 587 */ 588 if (!td) { 589 if (list_empty(&ep_ring->td_list)) { 590 new_seg = ep_ring->enq_seg; 591 new_deq = ep_ring->enqueue; 592 new_cycle = ep_ring->cycle_state; 593 xhci_dbg(xhci, "ep ring empty, Set new dequeue = enqueue"); 594 goto deq_found; 595 } else { 596 xhci_warn(xhci, "Can't find new dequeue state, missing td\n"); 597 return -EINVAL; 598 } 599 } 600 601 hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id); 602 new_seg = ep_ring->deq_seg; 603 new_deq = ep_ring->dequeue; 604 new_cycle = hw_dequeue & 0x1; 605 606 /* 607 * Quirk: xHC write-back of the DCS field in the hardware dequeue 608 * pointer is wrong - use the cycle state of the TRB pointed to by 609 * the dequeue pointer. 610 */ 611 if (xhci->quirks & XHCI_EP_CTX_BROKEN_DCS && 612 !(ep->ep_state & EP_HAS_STREAMS)) > 613 halted_seg = trb_in_td(xhci, cur_td->start_seg, 614 cur_td->first_trb, cur_td->last_trb, 615 hw_dequeue & ~0xf, false); 616 if (halted_seg) { 617 index = ((dma_addr_t)(hw_dequeue & ~0xf) - halted_seg->dma) / 618 sizeof(*halted_trb); 619 halted_trb = &halted_seg->trbs[index]; > 620 state->new_cycle_state = halted_trb->generic.field[3] & 0x1; 621 xhci_dbg(xhci, "Endpoint DCS = %d TRB index = %d cycle = %d\n", 622 (u8)(hw_dequeue & 0x1), index, 623 state->new_cycle_state); 624 } else { 625 state->new_cycle_state = hw_dequeue & 0x1; 626 } 627 state->stream_id = stream_id; 628 629 /* 630 * We want to find the pointer, segment and cycle state of the new trb 631 * (the one after current TD's last_trb). We know the cycle state at 632 * hw_dequeue, so walk the ring until both hw_dequeue and last_trb are 633 * found. 634 */ 635 do { 636 if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq) 637 == (dma_addr_t)(hw_dequeue & ~0xf)) { 638 cycle_found = true; 639 if (td_last_trb_found) 640 break; 641 } 642 if (new_deq == td->last_trb) 643 td_last_trb_found = true; 644 645 if (cycle_found && trb_is_link(new_deq) && 646 link_trb_toggles_cycle(new_deq)) 647 new_cycle ^= 0x1; 648 649 next_trb(xhci, ep_ring, &new_seg, &new_deq); 650 651 /* Search wrapped around, bail out */ 652 if (new_deq == ep->ring->dequeue) { 653 xhci_err(xhci, "Error: Failed finding new dequeue state\n"); 654 return -EINVAL; 655 } 656 657 } while (!cycle_found || !td_last_trb_found); 658 659 deq_found: 660 661 /* Don't update the ring cycle state for the producer (us). */ 662 addr = xhci_trb_virt_to_dma(new_seg, new_deq); 663 if (addr == 0) { 664 xhci_warn(xhci, "Can't find dma of new dequeue ptr\n"); 665 xhci_warn(xhci, "deq seg = %p, deq ptr = %p\n", new_seg, new_deq); 666 return -EINVAL; 667 } 668 669 if ((ep->ep_state & SET_DEQ_PENDING)) { 670 xhci_warn(xhci, "Set TR Deq already pending, don't submit for 0x%pad\n", 671 &addr); 672 return -EBUSY; 673 } 674 675 /* This function gets called from contexts where it cannot sleep */ 676 cmd = xhci_alloc_command(xhci, false, GFP_ATOMIC); 677 if (!cmd) { 678 xhci_warn(xhci, "Can't alloc Set TR Deq cmd 0x%pad\n", &addr); 679 return -ENOMEM; 680 } 681 682 if (stream_id) 683 trb_sct = SCT_FOR_TRB(SCT_PRI_TR); 684 ret = queue_command(xhci, cmd, 685 lower_32_bits(addr) | trb_sct | new_cycle, 686 upper_32_bits(addr), 687 STREAM_ID_FOR_TRB(stream_id), SLOT_ID_FOR_TRB(slot_id) | 688 EP_ID_FOR_TRB(ep_index) | TRB_TYPE(TRB_SET_DEQ), false); 689 if (ret < 0) { 690 xhci_free_command(xhci, cmd); 691 return ret; 692 } 693 ep->queued_deq_seg = new_seg; 694 ep->queued_deq_ptr = new_deq; 695 696 xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, 697 "Set TR Deq ptr 0x%llx, cycle %u\n", addr, new_cycle); 698 699 /* Stop the TD queueing code from ringing the doorbell until 700 * this command completes. The HC won't set the dequeue pointer 701 * if the ring is running, and ringing the doorbell starts the 702 * ring running. 703 */ 704 ep->ep_state |= SET_DEQ_PENDING; 705 xhci_ring_cmd_db(xhci); 706 return 0; 707 } 708 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 35791 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS @ 2021-07-02 8:57 ` kernel test robot 0 siblings, 0 replies; 11+ messages in thread From: kernel test robot @ 2021-07-02 8:57 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 8750 bytes --] Hi "Bjørn, Thank you for the patch! Yet something to improve: [auto build test ERROR on usb/usb-testing] [also build test ERROR on v5.13 next-20210701] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Bj-rn-Mork/xhci-add-quirk-for-host-controllers-that-don-t-update-endpoint-DCS/20210702-151445 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing config: riscv-randconfig-r012-20210702 (attached as .config) compiler: riscv32-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/61088f366a5c42caf8ce20c87355b61efc1b175d git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Bj-rn-Mork/xhci-add-quirk-for-host-controllers-that-don-t-update-endpoint-DCS/20210702-151445 git checkout 61088f366a5c42caf8ce20c87355b61efc1b175d # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash drivers/usb/host/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): drivers/usb/host/xhci-ring.c: In function 'xhci_move_dequeue_past_td': >> drivers/usb/host/xhci-ring.c:613:32: error: 'cur_td' undeclared (first use in this function) 613 | halted_seg = trb_in_td(xhci, cur_td->start_seg, | ^~~~~~ drivers/usb/host/xhci-ring.c:613:32: note: each undeclared identifier is reported only once for each function it appears in >> drivers/usb/host/xhci-ring.c:620:3: error: 'state' undeclared (first use in this function); did you mean 'statx'? 620 | state->new_cycle_state = halted_trb->generic.field[3] & 0x1; | ^~~~~ | statx Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for LOCKDEP Depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && (FRAME_POINTER || MIPS || PPC || S390 || MICROBLAZE || ARM || ARC || X86) Selected by - DEBUG_LOCK_ALLOC && DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT vim +/cur_td +613 drivers/usb/host/xhci-ring.c 552 553 static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, 554 unsigned int slot_id, unsigned int ep_index, 555 unsigned int stream_id, struct xhci_td *td) 556 { 557 struct xhci_virt_device *dev = xhci->devs[slot_id]; 558 struct xhci_virt_ep *ep = &dev->eps[ep_index]; 559 struct xhci_ring *ep_ring; 560 struct xhci_command *cmd; 561 struct xhci_segment *new_seg; 562 struct xhci_segment *halted_seg = NULL; 563 union xhci_trb *new_deq; 564 int new_cycle; 565 union xhci_trb *halted_trb; 566 int index = 0; 567 dma_addr_t addr; 568 u64 hw_dequeue; 569 bool cycle_found = false; 570 bool td_last_trb_found = false; 571 u32 trb_sct = 0; 572 int ret; 573 574 ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id, 575 ep_index, stream_id); 576 if (!ep_ring) { 577 xhci_warn(xhci, "WARN can't find new dequeue, invalid stream ID %u\n", 578 stream_id); 579 return -ENODEV; 580 } 581 /* 582 * A cancelled TD can complete with a stall if HW cached the trb. 583 * In this case driver can't find td, but if the ring is empty we 584 * can move the dequeue pointer to the current enqueue position. 585 * We shouldn't hit this anymore as cached cancelled TRBs are given back 586 * after clearing the cache, but be on the safe side and keep it anyway 587 */ 588 if (!td) { 589 if (list_empty(&ep_ring->td_list)) { 590 new_seg = ep_ring->enq_seg; 591 new_deq = ep_ring->enqueue; 592 new_cycle = ep_ring->cycle_state; 593 xhci_dbg(xhci, "ep ring empty, Set new dequeue = enqueue"); 594 goto deq_found; 595 } else { 596 xhci_warn(xhci, "Can't find new dequeue state, missing td\n"); 597 return -EINVAL; 598 } 599 } 600 601 hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id); 602 new_seg = ep_ring->deq_seg; 603 new_deq = ep_ring->dequeue; 604 new_cycle = hw_dequeue & 0x1; 605 606 /* 607 * Quirk: xHC write-back of the DCS field in the hardware dequeue 608 * pointer is wrong - use the cycle state of the TRB pointed to by 609 * the dequeue pointer. 610 */ 611 if (xhci->quirks & XHCI_EP_CTX_BROKEN_DCS && 612 !(ep->ep_state & EP_HAS_STREAMS)) > 613 halted_seg = trb_in_td(xhci, cur_td->start_seg, 614 cur_td->first_trb, cur_td->last_trb, 615 hw_dequeue & ~0xf, false); 616 if (halted_seg) { 617 index = ((dma_addr_t)(hw_dequeue & ~0xf) - halted_seg->dma) / 618 sizeof(*halted_trb); 619 halted_trb = &halted_seg->trbs[index]; > 620 state->new_cycle_state = halted_trb->generic.field[3] & 0x1; 621 xhci_dbg(xhci, "Endpoint DCS = %d TRB index = %d cycle = %d\n", 622 (u8)(hw_dequeue & 0x1), index, 623 state->new_cycle_state); 624 } else { 625 state->new_cycle_state = hw_dequeue & 0x1; 626 } 627 state->stream_id = stream_id; 628 629 /* 630 * We want to find the pointer, segment and cycle state of the new trb 631 * (the one after current TD's last_trb). We know the cycle state at 632 * hw_dequeue, so walk the ring until both hw_dequeue and last_trb are 633 * found. 634 */ 635 do { 636 if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq) 637 == (dma_addr_t)(hw_dequeue & ~0xf)) { 638 cycle_found = true; 639 if (td_last_trb_found) 640 break; 641 } 642 if (new_deq == td->last_trb) 643 td_last_trb_found = true; 644 645 if (cycle_found && trb_is_link(new_deq) && 646 link_trb_toggles_cycle(new_deq)) 647 new_cycle ^= 0x1; 648 649 next_trb(xhci, ep_ring, &new_seg, &new_deq); 650 651 /* Search wrapped around, bail out */ 652 if (new_deq == ep->ring->dequeue) { 653 xhci_err(xhci, "Error: Failed finding new dequeue state\n"); 654 return -EINVAL; 655 } 656 657 } while (!cycle_found || !td_last_trb_found); 658 659 deq_found: 660 661 /* Don't update the ring cycle state for the producer (us). */ 662 addr = xhci_trb_virt_to_dma(new_seg, new_deq); 663 if (addr == 0) { 664 xhci_warn(xhci, "Can't find dma of new dequeue ptr\n"); 665 xhci_warn(xhci, "deq seg = %p, deq ptr = %p\n", new_seg, new_deq); 666 return -EINVAL; 667 } 668 669 if ((ep->ep_state & SET_DEQ_PENDING)) { 670 xhci_warn(xhci, "Set TR Deq already pending, don't submit for 0x%pad\n", 671 &addr); 672 return -EBUSY; 673 } 674 675 /* This function gets called from contexts where it cannot sleep */ 676 cmd = xhci_alloc_command(xhci, false, GFP_ATOMIC); 677 if (!cmd) { 678 xhci_warn(xhci, "Can't alloc Set TR Deq cmd 0x%pad\n", &addr); 679 return -ENOMEM; 680 } 681 682 if (stream_id) 683 trb_sct = SCT_FOR_TRB(SCT_PRI_TR); 684 ret = queue_command(xhci, cmd, 685 lower_32_bits(addr) | trb_sct | new_cycle, 686 upper_32_bits(addr), 687 STREAM_ID_FOR_TRB(stream_id), SLOT_ID_FOR_TRB(slot_id) | 688 EP_ID_FOR_TRB(ep_index) | TRB_TYPE(TRB_SET_DEQ), false); 689 if (ret < 0) { 690 xhci_free_command(xhci, cmd); 691 return ret; 692 } 693 ep->queued_deq_seg = new_seg; 694 ep->queued_deq_ptr = new_deq; 695 696 xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, 697 "Set TR Deq ptr 0x%llx, cycle %u\n", addr, new_cycle); 698 699 /* Stop the TD queueing code from ringing the doorbell until 700 * this command completes. The HC won't set the dequeue pointer 701 * if the ring is running, and ringing the doorbell starts the 702 * ring running. 703 */ 704 ep->ep_state |= SET_DEQ_PENDING; 705 xhci_ring_cmd_db(xhci); 706 return 0; 707 } 708 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 35791 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS 2021-07-02 8:57 ` kernel test robot (?) @ 2021-07-02 9:10 ` Bjørn Mork 2021-07-20 15:09 ` [PATCH v2] " Bjørn Mork 2021-09-01 15:30 ` [PATCH v2 RESEND] " Bjørn Mork -1 siblings, 2 replies; 11+ messages in thread From: Bjørn Mork @ 2021-07-02 9:10 UTC (permalink / raw) To: Mathias Nyman, linux-usb, Jonathan Bell kernel test robot <lkp@intel.com> writes: > All errors (new ones prefixed by >>): > > drivers/usb/host/xhci-ring.c: In function 'xhci_move_dequeue_past_td': >>> drivers/usb/host/xhci-ring.c:613:32: error: 'cur_td' undeclared (first use in this function) > 613 | halted_seg = trb_in_td(xhci, cur_td->start_seg, > | ^~~~~~ > drivers/usb/host/xhci-ring.c:613:32: note: each undeclared identifier is reported only once for each function it appears in >>> drivers/usb/host/xhci-ring.c:620:3: error: 'state' undeclared (first use in this function); did you mean 'statx'? > 620 | state->new_cycle_state = halted_trb->generic.field[3] & 0x1; > | ^~~~~ > | statx Ouch. Sorry, this is embarrassing. What you get for testing on stable kernels and submitting to master. Please ignore this patch. Will fix. Bjørn ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] xhci: add quirk for host controllers that don't update endpoint DCS 2021-07-02 9:10 ` Bjørn Mork @ 2021-07-20 15:09 ` Bjørn Mork 2021-07-20 16:43 ` Rik van Riel 2021-09-01 15:30 ` [PATCH v2 RESEND] " Bjørn Mork 1 sibling, 1 reply; 11+ messages in thread From: Bjørn Mork @ 2021-07-20 15:09 UTC (permalink / raw) To: Mathias Nyman; +Cc: linux-usb, Jonathan Bell, stable, Bjørn Mork From: Jonathan Bell <jonathan@raspberrypi.org> Seen on a VLI VL805 PCIe to USB controller. For non-stream endpoints at least, if the xHC halts on a particular TRB due to an error then the DCS field in the Out Endpoint Context maintained by the hardware is not updated with the current cycle state. Using the quirk XHCI_EP_CTX_BROKEN_DCS and instead fetch the DCS bit from the TRB that the xHC stopped on. [ bjorn: rebased to v5.14-rc2 ] Cc: stable@vger.kernel.org Link: https://github.com/raspberrypi/linux/issues/3060 Signed-off-by: Jonathan Bell <jonathan@raspberrypi.org> Signed-off-by: Bjørn Mork <bjorn@mork.no> --- This took some time... But now it is at least build and runtime tested on top of v5.14-rc2, using an RPi4 and a generic RTL2838 DVB-T radio. Running rtl_test from rtl-sdr on an unpatched v5.14-rc2: root@idefix:/home/bjorn# rtl_test Found 1 device(s): 0: Realtek, RTL2838UHIDIR, SN: 00000001 Using device 0: Generic RTL2832U OEM rtlsdr_read_reg failed with -7 rtlsdr_write_reg failed with -7 rtlsdr_read_reg failed with -7 rtlsdr_write_reg failed with -7 rtlsdr_read_reg failed with -7 rtlsdr_write_reg failed with -7 rtlsdr_read_reg failed with -7 rtlsdr_write_reg failed with -7 No supported tuner found rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 Enabled direct sampling mode, input 1 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 Supported gain values (1): 0.0 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 WARNING: Failed to set sample rate. rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_write_reg failed with -7 rtlsdr_write_reg failed with -7 Info: This tool will continuously read from the device, and report if samples get lost. If you observe no further output, everything is fine. Reading samples in async mode... Allocating 15 zero-copy buffers And with this fix in place: root@idefix:/home/bjorn# rtl_test Found 1 device(s): 0: Realtek, RTL2838UHIDIR, SN: 00000001 Using device 0: Generic RTL2832U OEM Detached kernel driver Found Fitipower FC0012 tuner Supported gain values (5): -9.9 -4.0 7.1 17.9 19.2 Sampling at 2048000 S/s. Info: This tool will continuously read from the device, and report if samples get lost. If you observe no further output, everything is fine. Reading samples in async mode... Allocating 15 zero-copy buffers lost at least 88 bytes Please apply to stable as well. I'd really like to see this fixed in my favourite distro kernel. You'll probably want Jonathans original patch, as posted in <20210702071338.42777-1-bjorn@mork.no>, for anything older than v5.12. I'll resend that for stable once/if this is accepted in mainline. Bjørn drivers/usb/host/xhci-pci.c | 4 +++- drivers/usb/host/xhci-ring.c | 25 ++++++++++++++++++++++++- drivers/usb/host/xhci.h | 1 + 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 18c2bbddf080..6f3bed09028c 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -279,8 +279,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) pdev->device == 0x3432) xhci->quirks |= XHCI_BROKEN_STREAMS; - if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483) + if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483) { xhci->quirks |= XHCI_LPM_SUPPORT; + xhci->quirks |= XHCI_EP_CTX_BROKEN_DCS; + } if (pdev->vendor == PCI_VENDOR_ID_ASMEDIA && pdev->device == PCI_DEVICE_ID_ASMEDIA_1042_XHCI) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 8fea44bbc266..ba978a8fa414 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -559,8 +559,11 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, struct xhci_ring *ep_ring; struct xhci_command *cmd; struct xhci_segment *new_seg; + struct xhci_segment *halted_seg = NULL; union xhci_trb *new_deq; int new_cycle; + union xhci_trb *halted_trb; + int index = 0; dma_addr_t addr; u64 hw_dequeue; bool cycle_found = false; @@ -598,7 +601,27 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id); new_seg = ep_ring->deq_seg; new_deq = ep_ring->dequeue; - new_cycle = hw_dequeue & 0x1; + + /* + * Quirk: xHC write-back of the DCS field in the hardware dequeue + * pointer is wrong - use the cycle state of the TRB pointed to by + * the dequeue pointer. + */ + if (xhci->quirks & XHCI_EP_CTX_BROKEN_DCS && + !(ep->ep_state & EP_HAS_STREAMS)) + halted_seg = trb_in_td(xhci, td->start_seg, + td->first_trb, td->last_trb, + hw_dequeue & ~0xf, false); + if (halted_seg) { + index = ((dma_addr_t)(hw_dequeue & ~0xf) - halted_seg->dma) / + sizeof(*halted_trb); + halted_trb = &halted_seg->trbs[index]; + new_cycle = halted_trb->generic.field[3] & 0x1; + xhci_dbg(xhci, "Endpoint DCS = %d TRB index = %d cycle = %d\n", + (u8)(hw_dequeue & 0x1), index, new_cycle); + } else { + new_cycle = hw_dequeue & 0x1; + } /* * We want to find the pointer, segment and cycle state of the new trb diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 3c7d281672ae..911aeb7d8a19 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1896,6 +1896,7 @@ struct xhci_hcd { #define XHCI_SG_TRB_CACHE_SIZE_QUIRK BIT_ULL(39) #define XHCI_NO_SOFT_RETRY BIT_ULL(40) #define XHCI_BROKEN_D3COLD BIT_ULL(41) +#define XHCI_EP_CTX_BROKEN_DCS BIT_ULL(42) unsigned int num_active_eps; unsigned int limit_active_eps; -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] xhci: add quirk for host controllers that don't update endpoint DCS 2021-07-20 15:09 ` [PATCH v2] " Bjørn Mork @ 2021-07-20 16:43 ` Rik van Riel 0 siblings, 0 replies; 11+ messages in thread From: Rik van Riel @ 2021-07-20 16:43 UTC (permalink / raw) To: Bjørn Mork, Mathias Nyman; +Cc: linux-usb, Jonathan Bell, stable [-- Attachment #1: Type: text/plain, Size: 2645 bytes --] On Tue, 2021-07-20 at 17:09 +0200, Bjørn Mork wrote: > From: Jonathan Bell <jonathan@raspberrypi.org> > > Seen on a VLI VL805 PCIe to USB controller. For non-stream endpoints > at least, if the xHC halts on a particular TRB due to an error then > the DCS field in the Out Endpoint Context maintained by the hardware > is not updated with the current cycle state. I wonder if "some things getting out of sync" (probably not the same things) are the cause of the USB issues I see here with a noisy bus and the PCM2309B chip... > @@ -598,7 +601,27 @@ static int xhci_move_dequeue_past_td(struct > xhci_hcd *xhci, > hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id); > new_seg = ep_ring->deq_seg; > new_deq = ep_ring->dequeue; > - new_cycle = hw_dequeue & 0x1; > + > + /* > + * Quirk: xHC write-back of the DCS field in the hardware > dequeue > + * pointer is wrong - use the cycle state of the TRB pointed > to by > + * the dequeue pointer. > + */ > + if (xhci->quirks & XHCI_EP_CTX_BROKEN_DCS && > + !(ep->ep_state & EP_HAS_STREAMS)) > + halted_seg = trb_in_td(xhci, td->start_seg, > + td->first_trb, td->last_trb, > + hw_dequeue & ~0xf, false); > + if (halted_seg) { > + index = ((dma_addr_t)(hw_dequeue & ~0xf) - > halted_seg->dma) / > + sizeof(*halted_trb); > + halted_trb = &halted_seg->trbs[index]; > + new_cycle = halted_trb->generic.field[3] & 0x1; > + xhci_dbg(xhci, "Endpoint DCS = %d TRB index = %d > cycle = %d\n", > + (u8)(hw_dequeue & 0x1), index, new_cycle); > + } else { > + new_cycle = hw_dequeue & 0x1; > + } Is there ever a case where the cycle state is incorrect, and we should be using the DCS field, instead? I wonder if this is a quirk that should just be used everywhere, instead of only on a few systems where we know the hardware doesn't always behave right? Are there other places where the hardware is supposed to track the same information in multiple places, but might sometimes get them out of sync? If so, does the code have any detection of such issues? -- All Rights Reversed. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 RESEND] xhci: add quirk for host controllers that don't update endpoint DCS 2021-07-02 9:10 ` Bjørn Mork 2021-07-20 15:09 ` [PATCH v2] " Bjørn Mork @ 2021-09-01 15:30 ` Bjørn Mork 2021-09-27 18:30 ` Bjørn Mork 1 sibling, 1 reply; 11+ messages in thread From: Bjørn Mork @ 2021-09-01 15:30 UTC (permalink / raw) To: Mathias Nyman; +Cc: linux-usb, Jonathan Bell, stable, Bjørn Mork From: Jonathan Bell <jonathan@raspberrypi.org> Seen on a VLI VL805 PCIe to USB controller. For non-stream endpoints at least, if the xHC halts on a particular TRB due to an error then the DCS field in the Out Endpoint Context maintained by the hardware is not updated with the current cycle state. Using the quirk XHCI_EP_CTX_BROKEN_DCS and instead fetch the DCS bit from the TRB that the xHC stopped on. [ bjorn: rebased to v5.14-rc2 ] Cc: stable@vger.kernel.org Link: https://github.com/raspberrypi/linux/issues/3060 Signed-off-by: Jonathan Bell <jonathan@raspberrypi.org> Signed-off-by: Bjørn Mork <bjorn@mork.no> --- [ resending this as I haven't seen any ack/nak and wonder if it might have gotten lost in a large pile of vacation backlog ] This took some time... But now it is at least build and runtime tested on top of v5.14-rc2, using an RPi4 and a generic RTL2838 DVB-T radio. Running rtl_test from rtl-sdr on an unpatched v5.14-rc2: root@idefix:/home/bjorn# rtl_test Found 1 device(s): 0: Realtek, RTL2838UHIDIR, SN: 00000001 Using device 0: Generic RTL2832U OEM rtlsdr_read_reg failed with -7 rtlsdr_write_reg failed with -7 rtlsdr_read_reg failed with -7 rtlsdr_write_reg failed with -7 rtlsdr_read_reg failed with -7 rtlsdr_write_reg failed with -7 rtlsdr_read_reg failed with -7 rtlsdr_write_reg failed with -7 No supported tuner found rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 Enabled direct sampling mode, input 1 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 Supported gain values (1): 0.0 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 WARNING: Failed to set sample rate. rtlsdr_demod_write_reg failed with -7 rtlsdr_demod_read_reg failed with -7 rtlsdr_write_reg failed with -7 rtlsdr_write_reg failed with -7 Info: This tool will continuously read from the device, and report if samples get lost. If you observe no further output, everything is fine. Reading samples in async mode... Allocating 15 zero-copy buffers And with this fix in place: root@idefix:/home/bjorn# rtl_test Found 1 device(s): 0: Realtek, RTL2838UHIDIR, SN: 00000001 Using device 0: Generic RTL2832U OEM Detached kernel driver Found Fitipower FC0012 tuner Supported gain values (5): -9.9 -4.0 7.1 17.9 19.2 Sampling at 2048000 S/s. Info: This tool will continuously read from the device, and report if samples get lost. If you observe no further output, everything is fine. Reading samples in async mode... Allocating 15 zero-copy buffers lost at least 88 bytes Please apply to stable as well. I'd really like to see this fixed in my favourite distro kernel. You'll probably want Jonathans original patch, as posted in <20210702071338.42777-1-bjorn@mork.no>, for anything older than v5.12. I'll resend that for stable once/if this is accepted in mainline. Bjørn drivers/usb/host/xhci-pci.c | 4 +++- drivers/usb/host/xhci-ring.c | 25 ++++++++++++++++++++++++- drivers/usb/host/xhci.h | 1 + 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 18c2bbddf080..6f3bed09028c 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -279,8 +279,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) pdev->device == 0x3432) xhci->quirks |= XHCI_BROKEN_STREAMS; - if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483) + if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483) { xhci->quirks |= XHCI_LPM_SUPPORT; + xhci->quirks |= XHCI_EP_CTX_BROKEN_DCS; + } if (pdev->vendor == PCI_VENDOR_ID_ASMEDIA && pdev->device == PCI_DEVICE_ID_ASMEDIA_1042_XHCI) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 8fea44bbc266..ba978a8fa414 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -559,8 +559,11 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, struct xhci_ring *ep_ring; struct xhci_command *cmd; struct xhci_segment *new_seg; + struct xhci_segment *halted_seg = NULL; union xhci_trb *new_deq; int new_cycle; + union xhci_trb *halted_trb; + int index = 0; dma_addr_t addr; u64 hw_dequeue; bool cycle_found = false; @@ -598,7 +601,27 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id); new_seg = ep_ring->deq_seg; new_deq = ep_ring->dequeue; - new_cycle = hw_dequeue & 0x1; + + /* + * Quirk: xHC write-back of the DCS field in the hardware dequeue + * pointer is wrong - use the cycle state of the TRB pointed to by + * the dequeue pointer. + */ + if (xhci->quirks & XHCI_EP_CTX_BROKEN_DCS && + !(ep->ep_state & EP_HAS_STREAMS)) + halted_seg = trb_in_td(xhci, td->start_seg, + td->first_trb, td->last_trb, + hw_dequeue & ~0xf, false); + if (halted_seg) { + index = ((dma_addr_t)(hw_dequeue & ~0xf) - halted_seg->dma) / + sizeof(*halted_trb); + halted_trb = &halted_seg->trbs[index]; + new_cycle = halted_trb->generic.field[3] & 0x1; + xhci_dbg(xhci, "Endpoint DCS = %d TRB index = %d cycle = %d\n", + (u8)(hw_dequeue & 0x1), index, new_cycle); + } else { + new_cycle = hw_dequeue & 0x1; + } /* * We want to find the pointer, segment and cycle state of the new trb diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 3c7d281672ae..911aeb7d8a19 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1896,6 +1896,7 @@ struct xhci_hcd { #define XHCI_SG_TRB_CACHE_SIZE_QUIRK BIT_ULL(39) #define XHCI_NO_SOFT_RETRY BIT_ULL(40) #define XHCI_BROKEN_D3COLD BIT_ULL(41) +#define XHCI_EP_CTX_BROKEN_DCS BIT_ULL(42) unsigned int num_active_eps; unsigned int limit_active_eps; -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 RESEND] xhci: add quirk for host controllers that don't update endpoint DCS 2021-09-01 15:30 ` [PATCH v2 RESEND] " Bjørn Mork @ 2021-09-27 18:30 ` Bjørn Mork 2021-09-29 14:24 ` Mathias Nyman 0 siblings, 1 reply; 11+ messages in thread From: Bjørn Mork @ 2021-09-27 18:30 UTC (permalink / raw) To: Mathias Nyman; +Cc: linux-usb Bjørn Mork <bjorn@mork.no> writes: > From: Jonathan Bell <jonathan@raspberrypi.org> > > Seen on a VLI VL805 PCIe to USB controller. For non-stream endpoints > at least, if the xHC halts on a particular TRB due to an error then > the DCS field in the Out Endpoint Context maintained by the hardware > is not updated with the current cycle state. > > Using the quirk XHCI_EP_CTX_BROKEN_DCS and instead fetch the DCS bit > from the TRB that the xHC stopped on. > > [ bjorn: rebased to v5.14-rc2 ] > Cc: stable@vger.kernel.org > Link: https://github.com/raspberrypi/linux/issues/3060 > Signed-off-by: Jonathan Bell <jonathan@raspberrypi.org> > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > [ resending this as I haven't seen any ack/nak and wonder if it might > have gotten lost in a large pile of vacation backlog ] ping? Bjørn ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 RESEND] xhci: add quirk for host controllers that don't update endpoint DCS 2021-09-27 18:30 ` Bjørn Mork @ 2021-09-29 14:24 ` Mathias Nyman 0 siblings, 0 replies; 11+ messages in thread From: Mathias Nyman @ 2021-09-29 14:24 UTC (permalink / raw) To: Bjørn Mork, Mathias Nyman; +Cc: linux-usb On 27.9.2021 21.30, Bjørn Mork wrote: > Bjørn Mork <bjorn@mork.no> writes: > >> From: Jonathan Bell <jonathan@raspberrypi.org> >> >> Seen on a VLI VL805 PCIe to USB controller. For non-stream endpoints >> at least, if the xHC halts on a particular TRB due to an error then >> the DCS field in the Out Endpoint Context maintained by the hardware >> is not updated with the current cycle state. >> >> Using the quirk XHCI_EP_CTX_BROKEN_DCS and instead fetch the DCS bit >> from the TRB that the xHC stopped on. >> >> [ bjorn: rebased to v5.14-rc2 ] >> Cc: stable@vger.kernel.org >> Link: https://github.com/raspberrypi/linux/issues/3060 >> Signed-off-by: Jonathan Bell <jonathan@raspberrypi.org> >> Signed-off-by: Bjørn Mork <bjorn@mork.no> >> --- >> [ resending this as I haven't seen any ack/nak and wonder if it might >> have gotten lost in a large pile of vacation backlog ] > > ping? > Sorry about the delay, looks good to me. I'll let it go through some testing, then add it. As this goes to stable it makes sense to add it as is, but this again shows I really need to write a xhci_dma_to_trb(ring, dma) helper Thanks -Mathias ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS 2021-07-02 7:13 [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS Bjørn Mork @ 2021-07-02 12:21 ` kernel test robot 2021-07-02 12:21 ` kernel test robot 1 sibling, 0 replies; 11+ messages in thread From: kernel test robot @ 2021-07-02 12:21 UTC (permalink / raw) To: Bjørn Mork, Mathias Nyman Cc: clang-built-linux, kbuild-all, linux-usb, Jonathan Bell, stable, Bjørn Mork [-- Attachment #1: Type: text/plain, Size: 9878 bytes --] Hi "Bjørn, Thank you for the patch! Yet something to improve: [auto build test ERROR on usb/usb-testing] [also build test ERROR on v5.13 next-20210701] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Bj-rn-Mork/xhci-add-quirk-for-host-controllers-that-don-t-update-endpoint-DCS/20210702-151445 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing config: powerpc64-randconfig-r013-20210630 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9eb613b2de3163686b1a4bd1160f15ac56a4b083) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install powerpc64 cross compiling tool for clang build # apt-get install binutils-powerpc64-linux-gnu # https://github.com/0day-ci/linux/commit/61088f366a5c42caf8ce20c87355b61efc1b175d git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Bj-rn-Mork/xhci-add-quirk-for-host-controllers-that-don-t-update-endpoint-DCS/20210702-151445 git checkout 61088f366a5c42caf8ce20c87355b61efc1b175d # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/usb/host/ fs/kernfs/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from drivers/usb/host/xhci-ring.c:55: In file included from include/linux/scatterlist.h:7: In file included from include/linux/bug.h:5: In file included from arch/powerpc/include/asm/bug.h:109: In file included from include/asm-generic/bug.h:20: In file included from include/linux/kernel.h:12: In file included from include/linux/bitops.h:32: In file included from arch/powerpc/include/asm/bitops.h:62: arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined] #define __lwsync() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory") ^ <built-in>:308:9: note: previous definition is here #define __lwsync __builtin_ppc_lwsync ^ >> drivers/usb/host/xhci-ring.c:613:32: error: use of undeclared identifier 'cur_td' halted_seg = trb_in_td(xhci, cur_td->start_seg, ^ drivers/usb/host/xhci-ring.c:614:12: error: use of undeclared identifier 'cur_td' cur_td->first_trb, cur_td->last_trb, ^ drivers/usb/host/xhci-ring.c:614:31: error: use of undeclared identifier 'cur_td' cur_td->first_trb, cur_td->last_trb, ^ >> drivers/usb/host/xhci-ring.c:620:3: error: use of undeclared identifier 'state' state->new_cycle_state = halted_trb->generic.field[3] & 0x1; ^ drivers/usb/host/xhci-ring.c:623:5: error: use of undeclared identifier 'state' state->new_cycle_state); ^ drivers/usb/host/xhci-ring.c:625:3: error: use of undeclared identifier 'state' state->new_cycle_state = hw_dequeue & 0x1; ^ drivers/usb/host/xhci-ring.c:627:2: error: use of undeclared identifier 'state' state->stream_id = stream_id; ^ 1 warning and 7 errors generated. vim +/cur_td +613 drivers/usb/host/xhci-ring.c 552 553 static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, 554 unsigned int slot_id, unsigned int ep_index, 555 unsigned int stream_id, struct xhci_td *td) 556 { 557 struct xhci_virt_device *dev = xhci->devs[slot_id]; 558 struct xhci_virt_ep *ep = &dev->eps[ep_index]; 559 struct xhci_ring *ep_ring; 560 struct xhci_command *cmd; 561 struct xhci_segment *new_seg; 562 struct xhci_segment *halted_seg = NULL; 563 union xhci_trb *new_deq; 564 int new_cycle; 565 union xhci_trb *halted_trb; 566 int index = 0; 567 dma_addr_t addr; 568 u64 hw_dequeue; 569 bool cycle_found = false; 570 bool td_last_trb_found = false; 571 u32 trb_sct = 0; 572 int ret; 573 574 ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id, 575 ep_index, stream_id); 576 if (!ep_ring) { 577 xhci_warn(xhci, "WARN can't find new dequeue, invalid stream ID %u\n", 578 stream_id); 579 return -ENODEV; 580 } 581 /* 582 * A cancelled TD can complete with a stall if HW cached the trb. 583 * In this case driver can't find td, but if the ring is empty we 584 * can move the dequeue pointer to the current enqueue position. 585 * We shouldn't hit this anymore as cached cancelled TRBs are given back 586 * after clearing the cache, but be on the safe side and keep it anyway 587 */ 588 if (!td) { 589 if (list_empty(&ep_ring->td_list)) { 590 new_seg = ep_ring->enq_seg; 591 new_deq = ep_ring->enqueue; 592 new_cycle = ep_ring->cycle_state; 593 xhci_dbg(xhci, "ep ring empty, Set new dequeue = enqueue"); 594 goto deq_found; 595 } else { 596 xhci_warn(xhci, "Can't find new dequeue state, missing td\n"); 597 return -EINVAL; 598 } 599 } 600 601 hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id); 602 new_seg = ep_ring->deq_seg; 603 new_deq = ep_ring->dequeue; 604 new_cycle = hw_dequeue & 0x1; 605 606 /* 607 * Quirk: xHC write-back of the DCS field in the hardware dequeue 608 * pointer is wrong - use the cycle state of the TRB pointed to by 609 * the dequeue pointer. 610 */ 611 if (xhci->quirks & XHCI_EP_CTX_BROKEN_DCS && 612 !(ep->ep_state & EP_HAS_STREAMS)) > 613 halted_seg = trb_in_td(xhci, cur_td->start_seg, 614 cur_td->first_trb, cur_td->last_trb, 615 hw_dequeue & ~0xf, false); 616 if (halted_seg) { 617 index = ((dma_addr_t)(hw_dequeue & ~0xf) - halted_seg->dma) / 618 sizeof(*halted_trb); 619 halted_trb = &halted_seg->trbs[index]; > 620 state->new_cycle_state = halted_trb->generic.field[3] & 0x1; 621 xhci_dbg(xhci, "Endpoint DCS = %d TRB index = %d cycle = %d\n", 622 (u8)(hw_dequeue & 0x1), index, 623 state->new_cycle_state); 624 } else { 625 state->new_cycle_state = hw_dequeue & 0x1; 626 } 627 state->stream_id = stream_id; 628 629 /* 630 * We want to find the pointer, segment and cycle state of the new trb 631 * (the one after current TD's last_trb). We know the cycle state at 632 * hw_dequeue, so walk the ring until both hw_dequeue and last_trb are 633 * found. 634 */ 635 do { 636 if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq) 637 == (dma_addr_t)(hw_dequeue & ~0xf)) { 638 cycle_found = true; 639 if (td_last_trb_found) 640 break; 641 } 642 if (new_deq == td->last_trb) 643 td_last_trb_found = true; 644 645 if (cycle_found && trb_is_link(new_deq) && 646 link_trb_toggles_cycle(new_deq)) 647 new_cycle ^= 0x1; 648 649 next_trb(xhci, ep_ring, &new_seg, &new_deq); 650 651 /* Search wrapped around, bail out */ 652 if (new_deq == ep->ring->dequeue) { 653 xhci_err(xhci, "Error: Failed finding new dequeue state\n"); 654 return -EINVAL; 655 } 656 657 } while (!cycle_found || !td_last_trb_found); 658 659 deq_found: 660 661 /* Don't update the ring cycle state for the producer (us). */ 662 addr = xhci_trb_virt_to_dma(new_seg, new_deq); 663 if (addr == 0) { 664 xhci_warn(xhci, "Can't find dma of new dequeue ptr\n"); 665 xhci_warn(xhci, "deq seg = %p, deq ptr = %p\n", new_seg, new_deq); 666 return -EINVAL; 667 } 668 669 if ((ep->ep_state & SET_DEQ_PENDING)) { 670 xhci_warn(xhci, "Set TR Deq already pending, don't submit for 0x%pad\n", 671 &addr); 672 return -EBUSY; 673 } 674 675 /* This function gets called from contexts where it cannot sleep */ 676 cmd = xhci_alloc_command(xhci, false, GFP_ATOMIC); 677 if (!cmd) { 678 xhci_warn(xhci, "Can't alloc Set TR Deq cmd 0x%pad\n", &addr); 679 return -ENOMEM; 680 } 681 682 if (stream_id) 683 trb_sct = SCT_FOR_TRB(SCT_PRI_TR); 684 ret = queue_command(xhci, cmd, 685 lower_32_bits(addr) | trb_sct | new_cycle, 686 upper_32_bits(addr), 687 STREAM_ID_FOR_TRB(stream_id), SLOT_ID_FOR_TRB(slot_id) | 688 EP_ID_FOR_TRB(ep_index) | TRB_TYPE(TRB_SET_DEQ), false); 689 if (ret < 0) { 690 xhci_free_command(xhci, cmd); 691 return ret; 692 } 693 ep->queued_deq_seg = new_seg; 694 ep->queued_deq_ptr = new_deq; 695 696 xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, 697 "Set TR Deq ptr 0x%llx, cycle %u\n", addr, new_cycle); 698 699 /* Stop the TD queueing code from ringing the doorbell until 700 * this command completes. The HC won't set the dequeue pointer 701 * if the ring is running, and ringing the doorbell starts the 702 * ring running. 703 */ 704 ep->ep_state |= SET_DEQ_PENDING; 705 xhci_ring_cmd_db(xhci); 706 return 0; 707 } 708 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 28342 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS @ 2021-07-02 12:21 ` kernel test robot 0 siblings, 0 replies; 11+ messages in thread From: kernel test robot @ 2021-07-02 12:21 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 10114 bytes --] Hi "Bjørn, Thank you for the patch! Yet something to improve: [auto build test ERROR on usb/usb-testing] [also build test ERROR on v5.13 next-20210701] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Bj-rn-Mork/xhci-add-quirk-for-host-controllers-that-don-t-update-endpoint-DCS/20210702-151445 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing config: powerpc64-randconfig-r013-20210630 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9eb613b2de3163686b1a4bd1160f15ac56a4b083) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install powerpc64 cross compiling tool for clang build # apt-get install binutils-powerpc64-linux-gnu # https://github.com/0day-ci/linux/commit/61088f366a5c42caf8ce20c87355b61efc1b175d git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Bj-rn-Mork/xhci-add-quirk-for-host-controllers-that-don-t-update-endpoint-DCS/20210702-151445 git checkout 61088f366a5c42caf8ce20c87355b61efc1b175d # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/usb/host/ fs/kernfs/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from drivers/usb/host/xhci-ring.c:55: In file included from include/linux/scatterlist.h:7: In file included from include/linux/bug.h:5: In file included from arch/powerpc/include/asm/bug.h:109: In file included from include/asm-generic/bug.h:20: In file included from include/linux/kernel.h:12: In file included from include/linux/bitops.h:32: In file included from arch/powerpc/include/asm/bitops.h:62: arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined] #define __lwsync() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory") ^ <built-in>:308:9: note: previous definition is here #define __lwsync __builtin_ppc_lwsync ^ >> drivers/usb/host/xhci-ring.c:613:32: error: use of undeclared identifier 'cur_td' halted_seg = trb_in_td(xhci, cur_td->start_seg, ^ drivers/usb/host/xhci-ring.c:614:12: error: use of undeclared identifier 'cur_td' cur_td->first_trb, cur_td->last_trb, ^ drivers/usb/host/xhci-ring.c:614:31: error: use of undeclared identifier 'cur_td' cur_td->first_trb, cur_td->last_trb, ^ >> drivers/usb/host/xhci-ring.c:620:3: error: use of undeclared identifier 'state' state->new_cycle_state = halted_trb->generic.field[3] & 0x1; ^ drivers/usb/host/xhci-ring.c:623:5: error: use of undeclared identifier 'state' state->new_cycle_state); ^ drivers/usb/host/xhci-ring.c:625:3: error: use of undeclared identifier 'state' state->new_cycle_state = hw_dequeue & 0x1; ^ drivers/usb/host/xhci-ring.c:627:2: error: use of undeclared identifier 'state' state->stream_id = stream_id; ^ 1 warning and 7 errors generated. vim +/cur_td +613 drivers/usb/host/xhci-ring.c 552 553 static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, 554 unsigned int slot_id, unsigned int ep_index, 555 unsigned int stream_id, struct xhci_td *td) 556 { 557 struct xhci_virt_device *dev = xhci->devs[slot_id]; 558 struct xhci_virt_ep *ep = &dev->eps[ep_index]; 559 struct xhci_ring *ep_ring; 560 struct xhci_command *cmd; 561 struct xhci_segment *new_seg; 562 struct xhci_segment *halted_seg = NULL; 563 union xhci_trb *new_deq; 564 int new_cycle; 565 union xhci_trb *halted_trb; 566 int index = 0; 567 dma_addr_t addr; 568 u64 hw_dequeue; 569 bool cycle_found = false; 570 bool td_last_trb_found = false; 571 u32 trb_sct = 0; 572 int ret; 573 574 ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id, 575 ep_index, stream_id); 576 if (!ep_ring) { 577 xhci_warn(xhci, "WARN can't find new dequeue, invalid stream ID %u\n", 578 stream_id); 579 return -ENODEV; 580 } 581 /* 582 * A cancelled TD can complete with a stall if HW cached the trb. 583 * In this case driver can't find td, but if the ring is empty we 584 * can move the dequeue pointer to the current enqueue position. 585 * We shouldn't hit this anymore as cached cancelled TRBs are given back 586 * after clearing the cache, but be on the safe side and keep it anyway 587 */ 588 if (!td) { 589 if (list_empty(&ep_ring->td_list)) { 590 new_seg = ep_ring->enq_seg; 591 new_deq = ep_ring->enqueue; 592 new_cycle = ep_ring->cycle_state; 593 xhci_dbg(xhci, "ep ring empty, Set new dequeue = enqueue"); 594 goto deq_found; 595 } else { 596 xhci_warn(xhci, "Can't find new dequeue state, missing td\n"); 597 return -EINVAL; 598 } 599 } 600 601 hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id); 602 new_seg = ep_ring->deq_seg; 603 new_deq = ep_ring->dequeue; 604 new_cycle = hw_dequeue & 0x1; 605 606 /* 607 * Quirk: xHC write-back of the DCS field in the hardware dequeue 608 * pointer is wrong - use the cycle state of the TRB pointed to by 609 * the dequeue pointer. 610 */ 611 if (xhci->quirks & XHCI_EP_CTX_BROKEN_DCS && 612 !(ep->ep_state & EP_HAS_STREAMS)) > 613 halted_seg = trb_in_td(xhci, cur_td->start_seg, 614 cur_td->first_trb, cur_td->last_trb, 615 hw_dequeue & ~0xf, false); 616 if (halted_seg) { 617 index = ((dma_addr_t)(hw_dequeue & ~0xf) - halted_seg->dma) / 618 sizeof(*halted_trb); 619 halted_trb = &halted_seg->trbs[index]; > 620 state->new_cycle_state = halted_trb->generic.field[3] & 0x1; 621 xhci_dbg(xhci, "Endpoint DCS = %d TRB index = %d cycle = %d\n", 622 (u8)(hw_dequeue & 0x1), index, 623 state->new_cycle_state); 624 } else { 625 state->new_cycle_state = hw_dequeue & 0x1; 626 } 627 state->stream_id = stream_id; 628 629 /* 630 * We want to find the pointer, segment and cycle state of the new trb 631 * (the one after current TD's last_trb). We know the cycle state at 632 * hw_dequeue, so walk the ring until both hw_dequeue and last_trb are 633 * found. 634 */ 635 do { 636 if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq) 637 == (dma_addr_t)(hw_dequeue & ~0xf)) { 638 cycle_found = true; 639 if (td_last_trb_found) 640 break; 641 } 642 if (new_deq == td->last_trb) 643 td_last_trb_found = true; 644 645 if (cycle_found && trb_is_link(new_deq) && 646 link_trb_toggles_cycle(new_deq)) 647 new_cycle ^= 0x1; 648 649 next_trb(xhci, ep_ring, &new_seg, &new_deq); 650 651 /* Search wrapped around, bail out */ 652 if (new_deq == ep->ring->dequeue) { 653 xhci_err(xhci, "Error: Failed finding new dequeue state\n"); 654 return -EINVAL; 655 } 656 657 } while (!cycle_found || !td_last_trb_found); 658 659 deq_found: 660 661 /* Don't update the ring cycle state for the producer (us). */ 662 addr = xhci_trb_virt_to_dma(new_seg, new_deq); 663 if (addr == 0) { 664 xhci_warn(xhci, "Can't find dma of new dequeue ptr\n"); 665 xhci_warn(xhci, "deq seg = %p, deq ptr = %p\n", new_seg, new_deq); 666 return -EINVAL; 667 } 668 669 if ((ep->ep_state & SET_DEQ_PENDING)) { 670 xhci_warn(xhci, "Set TR Deq already pending, don't submit for 0x%pad\n", 671 &addr); 672 return -EBUSY; 673 } 674 675 /* This function gets called from contexts where it cannot sleep */ 676 cmd = xhci_alloc_command(xhci, false, GFP_ATOMIC); 677 if (!cmd) { 678 xhci_warn(xhci, "Can't alloc Set TR Deq cmd 0x%pad\n", &addr); 679 return -ENOMEM; 680 } 681 682 if (stream_id) 683 trb_sct = SCT_FOR_TRB(SCT_PRI_TR); 684 ret = queue_command(xhci, cmd, 685 lower_32_bits(addr) | trb_sct | new_cycle, 686 upper_32_bits(addr), 687 STREAM_ID_FOR_TRB(stream_id), SLOT_ID_FOR_TRB(slot_id) | 688 EP_ID_FOR_TRB(ep_index) | TRB_TYPE(TRB_SET_DEQ), false); 689 if (ret < 0) { 690 xhci_free_command(xhci, cmd); 691 return ret; 692 } 693 ep->queued_deq_seg = new_seg; 694 ep->queued_deq_ptr = new_deq; 695 696 xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, 697 "Set TR Deq ptr 0x%llx, cycle %u\n", addr, new_cycle); 698 699 /* Stop the TD queueing code from ringing the doorbell until 700 * this command completes. The HC won't set the dequeue pointer 701 * if the ring is running, and ringing the doorbell starts the 702 * ring running. 703 */ 704 ep->ep_state |= SET_DEQ_PENDING; 705 xhci_ring_cmd_db(xhci); 706 return 0; 707 } 708 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 28342 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-09-29 14:22 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-02 7:13 [PATCH] xhci: add quirk for host controllers that don't update endpoint DCS Bjørn Mork 2021-07-02 8:57 ` kernel test robot 2021-07-02 8:57 ` kernel test robot 2021-07-02 9:10 ` Bjørn Mork 2021-07-20 15:09 ` [PATCH v2] " Bjørn Mork 2021-07-20 16:43 ` Rik van Riel 2021-09-01 15:30 ` [PATCH v2 RESEND] " Bjørn Mork 2021-09-27 18:30 ` Bjørn Mork 2021-09-29 14:24 ` Mathias Nyman 2021-07-02 12:21 ` [PATCH] " kernel test robot 2021-07-02 12:21 ` kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.