Re: [PATCH v2] usb: xhci: add debugfs support for ep with stream

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 18.8.2020 4.54, Jun Li wrote:
> 
> 
>> -----Original Message-----
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Sent: Monday, August 17, 2020 7:48 PM
>> To: Jun Li <jun.li@nxp.com>; mathias.nyman@intel.com
>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx
>> <linux-imx@nxp.com>
>> 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@nxp.com>
>>>> Sent: Thursday, July 16, 2020 8:40 PM
>>>> To: mathias.nyman@intel.com
>>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
>>>> dl-linux-imx <linux-imx@nxp.com>
>>>> 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@nxp.com>
>>>> ---
>>>> 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