> -----Original Message----- > From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> > Sent: Monday, August 31, 2020 9:11 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 > > On 18.8.2020 4.54, Jun Li wrote: > > > > > >> -----Original Message----- > >> From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> > >> Sent: Monday, August 17, 2020 7:48 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 > >> > >> On 13.8.2020 12.57, Jun Li wrote: > >>> Hi > >>> > >>>> -----Original Message----- > >>>> From: Jun Li <jun.li@xxxxxxx> > >>>> Sent: Thursday, July 16, 2020 8:40 PM > >>>> To: mathias.nyman@xxxxxxxxx > >>>> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > >>>> dl-linux-imx <linux-imx@xxxxxxx> > >>>> Subject: [PATCH v2] usb: xhci: add debugfs support for ep with > >>>> stream > >>>> > >>>> To show the trb ring of streams, use the exsiting ring files of > >>>> bulk ep to show trb ring of one specific stream ID, which stream > >>>> ID's trb ring will be shown, is controlled by a new debugfs file > >>>> stream_id, this is to avoid to create a large number of dir for > >>>> every allocate stream IDs, another debugfs file > >>>> stream_context_array is created to show all > >> the allocated stream context array entries. > >>>> > >>>> Signed-off-by: Li Jun <jun.li@xxxxxxx> > >>>> --- > >>>> chanages for v2: > >>>> - Drop stream files remove, the stream files will be removed > >>>> with ep dir removal, keep the ep but only remove streams > >>>> actually does not make sense in current code. > >>>> - Use the new_ring for show_ring pointer for non-zero ep. > >>>> > >>>> drivers/usb/host/xhci-debugfs.c | 112 > >>>> +++++++++++++++++++++++++++++++++++++++- > >>>> drivers/usb/host/xhci-debugfs.h | 10 ++++ > >>>> drivers/usb/host/xhci.c | 1 + > >>>> 3 files changed, 122 insertions(+), 1 deletion(-) > >>>> > >>>> 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. 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 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]; thanks Li Jun > > -Mathias
