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

["David Wang" <00107082@163.com>]

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