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

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

On 4.9.2020 13.01, Jun Li wrote:
> 
> 
>> -----Original Message-----
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Sent: Thursday, September 3, 2020 5:39 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 3.9.2020 10.46, Jun Li wrote:
>>>
>>>> -----Original Message-----
>>>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>> Sent: Thursday, September 3, 2020 3:24 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
>>>>
>>>>>> 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%2Fg
>>>>>> it
>>>>>> .kernel.o
>>>>>> rg%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmnyman%2Fxhci.git%2Fcommit%
>>>>>> 2F
>>>>>> %3Fh%3Dfo
>>>>>> r-usb-linus%26id%3Dcf99aef5624a592fd4f3374c7060bfa1a65f15df&amp;dat
>>>>>> a=
>>>>>> 02%7C01%7
>>>>>> Cjun.li%40nxp.com%7C73e4663a6f6641fbb8b308d84f3d021a%7C686ea1d3bc2b
>>>>>> 4c
>>>>>> 6fa92cd99
>>>>>> c5c301635%7C0%7C0%7C637346470803922895&amp;sdata=i4DfCW8EVUSAWnzb8Q
>>>>>> l4
>>>>>> jPjAOD5wp
>>>>>> tfbaMrO8vKvtDc%3D&amp;reserved=0
>>>>>
>>>>> This is a good place for non-zero Eps, but does not cover ep0.
>>>>>
>>>>
>>>> ep0 is special, it's not touched in these add/drop endpoint or check
>>>> bandwidth functions.
>>>>
>>>> ep0 ring is allocated earlier during slot creation in
>>>> xhci_alloc_virt_device()
>>>>   ...
>>>>   /* Allocate endpoint 0 ring */
>>>>   dev->eps[0].ring = xhci_ring_alloc(xhci, 2, 1, TYPE_CTRL, 0,
>>>> flags);
>>>>
>>>> and for debugfs ep00 is added manually together with the slot
>>>> xhci_debugfs_create_slot()
>>>>   ...
>>>>   xhci_debugfs_create_ring_dir(xhci, &dev->eps[0].ring, "ep00",
>>>> priv->root);
>>>>
>>>> So regarding ep0 the change should be ok.
>>>
>>> Sorry, I forgot debugfs of ep0 is created via xhci_debugfs_create_slot().
>>>
>>> Then I think your change is OK, also I gave a test with my stream/UAS
>>> device on top of your patch and it can work fine.
>>>
>>> Do you need I post a new version of my patch(to remove touch of .new_ring)?
>>
>> If you could yes, and also change to a double pointer:
>>
>>  struct xhci_ep_priv {
>> 	...
>> +	struct xhci_ring	**show_ring;
> 
> With current: struct xhci_ring	*show_ring;
> As I use one trb file to show different trb rings for one EP, so I need get
> the addr of trb ring pointed by show_ring(which can be updated).
> 
> If I change it to be **show_ring, then I am passing the addr of dev->eps[i].ring
> to debugfs so I can't use show_ring's update value when show trb, makes me
> not easy to get the addr of target trb ring.

Right, ok, lets not use the douple pointer.
We just have to trust epriv->show_ring is up to date.

-Mathias