Re: [PATCH v4 2/2] USB: xhci: use urb hcpriv mempool for private data

David Wang
2025-05-22T09:56:59Z

At 2025-05-22 16:32:40, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Thu, May 22, 2025 at 03:10:10PM +0800, David Wang wrote:
>> xhci keeps alloc/free private data for each enqueue/dequeue cycles,
>> when using a USB webcam, allocation rate is ~250/s;
>> when using a USB mic, allocation rate reaches ~1k/s;
>> The more usb device in use, the higher allocation rate.
>>
>> URB objects have longer lifespan than private data, hand over ownership
>> of private data to urb can save lots of memory allocations over time.
>> With this change, no extra memory allocation is needed during usages of
>> USB webcam/mic.
>>
>> Signed-off-by: David Wang <00107082@163.com>
>> ---
>> drivers/usb/host/xhci-mem.c | 1 +
>> drivers/usb/host/xhci-ring.c | 3 +--
>> drivers/usb/host/xhci.c | 8 +++-----
>> 3 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index d698095fc88d..b19e41cf1c4c 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -1745,6 +1745,7 @@ struct xhci_command *xhci_alloc_command_with_ctx(struct xhci_hcd *xhci,
>>
>> void xhci_urb_free_priv(struct urb_priv *urb_priv)
>> {
>> + WARN_ONCE(1, "xhci private data should be managed by urb");
>
>You just crashed the kernel if this ever gets hit. As you are saying
>this should never be called, why is this function even present anymore?
>
>This makes no sense :(

I meant to warn further changes to xhci: better not manage private data .
I don't think it would crash, xhci_urb_free_priv should not be paired with
urb_hcpriv_mempool_zalloc. (But nothing prevent it though, same as nothing
prevents urb_hcpriv_mempool_zalloc being paired with kfree....)
It would be better to remove the whole function.

>
>Again, NEVER add a WARN*() call to the kernel for something that it
>should be handling properly on its own. Otherwise you just lost all the
>user's data when the box got rebooted (and if userspace can trigger
>this, you just created a new CVE...)

Copy that~!

Thanks~
David
>
>thanks,
>
>greg k-h