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://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/commit/?h=for-usb-linus&id=cf99aef5624a592fd4f3374c7060bfa1a65f15df 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
