On 3.9.2020 10.46, Jun Li wrote: > >> -----Original Message----- >> From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> >> Sent: Thursday, September 3, 2020 3:24 PM >> To: Jun Li <jun.li@xxxxxxx>; mathias.nyman@xxxxxxxxx >> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx >> <linux-imx@xxxxxxx> >> Subject: Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream >> >>>> I think this debugfs code is just called too early. It shouldn't need >>>> to check new_ring pointer at all. >>>> >>>> I wrote a fix that changes the order and makes sure endpoint is >>>> enabled and ring pointer is set correctly before we call >>>> xhci_debugfs_create_endpoint() >>>> >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit >>>> .kernel.o >>>> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmnyman%2Fxhci.git%2Fcommit%2F >>>> %3Fh%3Dfo >>>> r-usb-linus%26id%3Dcf99aef5624a592fd4f3374c7060bfa1a65f15df&data= >>>> 02%7C01%7 >>>> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b4c >>>> 6fa92cd99 >>>> c5c301635%7C0%7C0%7C637346470803922895&sdata=i4DfCW8EVUSAWnzb8Ql4 >>>> jPjAOD5wp >>>> tfbaMrO8vKvtDc%3D&reserved=0 >>> >>> This is a good place for non-zero Eps, but does not cover ep0. >>> >> >> ep0 is special, it's not touched in these add/drop endpoint or check bandwidth >> functions. >> >> ep0 ring is allocated earlier during slot creation in >> xhci_alloc_virt_device() >> ... >> /* Allocate endpoint 0 ring */ >> dev->eps[0].ring = xhci_ring_alloc(xhci, 2, 1, TYPE_CTRL, 0, flags); >> >> and for debugfs ep00 is added manually together with the slot >> xhci_debugfs_create_slot() >> ... >> xhci_debugfs_create_ring_dir(xhci, &dev->eps[0].ring, "ep00", priv->root); >> >> So regarding ep0 the change should be ok. > > Sorry, I forgot debugfs of ep0 is created via xhci_debugfs_create_slot(). > > Then I think your change is OK, also I gave a test with my stream/UAS device > on top of your patch and it can work fine. > > Do you need I post a new version of my patch(to remove touch of .new_ring)? If you could yes, and also change to a double pointer: struct xhci_ep_priv { ... + struct xhci_ring **show_ring; }; And modify other places this change affects, such as: epriv->show_ring = &dev->eps[ep_index].ring; -Mathias
