linux-kernel - Re: [PATCH v4 06/10] usb: gadget: uvc: set req_size once when the vb2 queue is calculated

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Zs5MHRMe1b3qDuRJ@pengutronix.de>
Date: Tue, 27 Aug 2024 23:58:53 +0200
From: Michael Grzeschik <mgr@...gutronix.de>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Daniel Scally <dan.scally@...asonboard.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Avichal Rakesh <arakesh@...gle.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 06/10] usb: gadget: uvc: set req_size once when the
 vb2 queue is calculated

On Tue, Aug 13, 2024 at 12:41:10PM +0300, Laurent Pinchart wrote:
>On Tue, Aug 13, 2024 at 11:09:30AM +0200, Michael Grzeschik wrote:
>> The uvc gadget driver is calculating the req_size on every
>> video_enable/alloc_request and is based on the fixed configfs parameters
>> maxpacket, maxburst and mult.
>>
>> As those parameters can not be changed once the gadget is started and
>> the same calculation is done already early on the
>> vb2_streamon/queue_setup path its save to remove one extra calculation
>> and reuse the calculation from uvc_queue_setup for the allocation step.
>
>Avoiding double calculations is good, but then don't compute the value
>in uvc_queue_setup(). That will also be called multiple times, and its
>timing will be controlled by userspace. Move it to a better location.

I think I was unclear regarding the avoidance of double calculations.

Actually this is the right place to calculate this, since the
resulting req_size will be dependent on the expected imagesize
and the frameduration, we have to calculate it in every queue_setup.

Since this patch is an preperation for the next change, I will leave the
codebase as is but will add the details in the patch description.

Michael

>> Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
>>
>> ---
>> v3 -> v4: -
>> v2 -> v3: -
>> v1 -> v2: -
>> ---
>>  drivers/usb/gadget/function/uvc_queue.c |  2 ++
>>  drivers/usb/gadget/function/uvc_video.c | 15 ++-------------
>>  2 files changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>> index 7995dd3fef184..2414d78b031f4 100644
>> --- a/drivers/usb/gadget/function/uvc_queue.c
>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>> @@ -63,6 +63,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>>  	 */
>>  	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
>>  	nreq = clamp(nreq, 4U, 64U);
>> +
>> +	video->req_size = req_size;
>>  	video->uvc_num_requests = nreq;
>>
>>  	return 0;
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index 259920ae36843..a6786beef91ad 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -480,7 +480,6 @@ uvc_video_free_requests(struct uvc_video *video)
>>  	INIT_LIST_HEAD(&video->ureqs);
>>  	INIT_LIST_HEAD(&video->req_free);
>>  	INIT_LIST_HEAD(&video->req_ready);
>> -	video->req_size = 0;
>>  	return 0;
>>  }
>>
>> @@ -488,16 +487,9 @@ static int
>>  uvc_video_alloc_requests(struct uvc_video *video)
>>  {
>>  	struct uvc_request *ureq;
>> -	unsigned int req_size;
>>  	unsigned int i;
>>  	int ret = -ENOMEM;
>>
>> -	BUG_ON(video->req_size);
>> -
>> -	req_size = video->ep->maxpacket
>> -		 * max_t(unsigned int, video->ep->maxburst, 1)
>> -		 * (video->ep->mult);
>> -
>>  	for (i = 0; i < video->uvc_num_requests; i++) {
>>  		ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL);
>>  		if (ureq == NULL)
>> @@ -507,7 +499,7 @@ uvc_video_alloc_requests(struct uvc_video *video)
>>
>>  		list_add_tail(&ureq->list, &video->ureqs);
>>
>> -		ureq->req_buffer = kmalloc(req_size, GFP_KERNEL);
>> +		ureq->req_buffer = kmalloc(video->req_size, GFP_KERNEL);
>>  		if (ureq->req_buffer == NULL)
>>  			goto error;
>>
>> @@ -525,12 +517,10 @@ uvc_video_alloc_requests(struct uvc_video *video)
>>  		list_add_tail(&ureq->req->list, &video->req_free);
>>  		/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
>>  		sg_alloc_table(&ureq->sgt,
>> -			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
>> +			       DIV_ROUND_UP(video->req_size - UVCG_REQUEST_HEADER_LEN,
>>  					    PAGE_SIZE) + 2, GFP_KERNEL);
>>  	}
>>
>> -	video->req_size = req_size;
>> -
>>  	return 0;
>>
>>  error:
>> @@ -663,7 +653,6 @@ uvcg_video_disable(struct uvc_video *video)
>>  	INIT_LIST_HEAD(&video->ureqs);
>>  	INIT_LIST_HEAD(&video->req_free);
>>  	INIT_LIST_HEAD(&video->req_ready);
>> -	video->req_size = 0;
>>  	spin_unlock_irqrestore(&video->req_lock, flags);
>>
>>  	/*
>
>-- 
>Regards,
>
>Laurent Pinchart
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)