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. 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. 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]) -Mathias
