> -----Original Message----- > From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> > Sent: Wednesday, September 2, 2020 8:41 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 > > Hi > > >>>>>> diff --git a/drivers/usb/host/xhci-debugfs.c > >>>>>> b/drivers/usb/host/xhci-debugfs.c index 65d8de4..708585c 100644 > >>>>>> --- a/drivers/usb/host/xhci-debugfs.c > >>>>>> +++ b/drivers/usb/host/xhci-debugfs.c > >>>>>> @@ -450,9 +450,14 @@ void xhci_debugfs_create_endpoint(struct xhci_hcd > *xhci, > >>>>>> if (!epriv) > >>>>>> return; > >>>>>> > >>>>>> + if (dev->eps[ep_index].ring) > >>>>>> + epriv->show_ring = dev->eps[ep_index].ring; > >>>>>> + else > >>>>>> + epriv->show_ring = dev->eps[ep_index].new_ring; > >>>>>> + > >> > >> Ran some tests and the I suspect the above code causes issues. > >> > >> If an endpoint is dropped and added back the above code will store > >> the old ring in epriv->show_ring as we have both a .ring and .new_ring present > at that moment. > >> Soon after this the old ring pointed to by .ring will be freed, and > >> .ring = .new_ring will be set. > > > > Yes, in this case, eps[ep_index].ring still keeps the old ring > > address, but eps[ep_index].new_ring is pointing to the new/correct > > ring, my patch will store the old ring address. > > > >> > >> Old code showed whatever ring buffer eps[i].ring pointer pointed ,to > >> when the sysfs file was read, (as we saved the address, > >> &eps[i].ring). I see now that it in theory it had a small gap before > >> .ring = .new_ring was set where user could read the ring buffer and .ring would > still be a NULL pointer. > >> That needs to be fixed as well. > > > > Yes, also in above drop-then-add-back case the old code eps[i].ring > > still points to the old ring. > > yes, but only until for a short while until .ring = .new_ring is set. > > > > > So considering we are creating debugfs file for ep before .ring = > > .new_ring; .new_ring = NULL; > > > > A solution of firstly check .new_ring seems can resolve the problems here: > > > > if (dev->eps[ep_index].new_ring) > > /* For first add of EP; or drop-then-add-back EP */ > > epriv->show_ring = dev->eps[ep_index].new_ring; else > > /* For EP0 */ > > epriv->show_ring = dev->eps[ep_index].ring; > > > > 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%7C686ea1d3bc2b4c6fa92cd99 > c5c301635%7C0%7C0%7C637346470803922895&sdata=i4DfCW8EVUSAWnzb8Ql4jPjAOD5wp > tfbaMrO8vKvtDc%3D&reserved=0 This is a good place for non-zero Eps, but does not cover ep0. Li Jun > > I think this streams support should be added on top of that > >> > >> I still like the old way of using double pointer more. > >> xhci driver will also dig out the current ring from eps[i].ring, so I > >> think we should as well. > >> (in stream case it would be > >> &ep->stream_info->stream_rings[stream_id]) > > > > stream case should no problem as it is > > epriv->show_ring = ep->stream_info->stream_rings[stream_id]; > > Sounds good > > -Mathias
