[PATCH] usb: gadget: uvc: don't put item still in use

[PATCH] usb: gadget: uvc: don't put item still in use

[Michael Grzeschik]

With the patch "15a286a0bb08 (usb: gadget: uvc: add v4l2 enumeration api
calls)" the driver is keeping a list of configfs entries currently
configured. The list is used in uvc_v4l2 on runtime.

The driver now is giving back the list item just after it was referenced
with config_item_put. It also calls config_item_put on uvc_free, which
is the only and right place to give back the reference. This patch fixes
the issue by removing the extra config_item_put in uvc_alloc.

Fixes: 15a286a0bb08 (usb: gadget: uvc: add v4l2 enumeration api calls)
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/f_uvc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index e6948cf8def30b..8bc591431d153e 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -995,7 +995,6 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
 		goto err_config;
 
 	uvc->header = to_uvcg_streaming_header(h);
-	config_item_put(h);
 	if (!uvc->header->linked) {
 		mutex_unlock(&opts->lock);
 		kfree(uvc);
-- 
2.30.2

Re: [PATCH] usb: gadget: uvc: don't put item still in use

[Greg KH]

On Thu, Sep 29, 2022 at 04:41:24PM +0200, Michael Grzeschik wrote:
> With the patch "15a286a0bb08 (usb: gadget: uvc: add v4l2 enumeration api
> calls)" the driver is keeping a list of configfs entries currently
> configured. The list is used in uvc_v4l2 on runtime.
> 
> The driver now is giving back the list item just after it was referenced
> with config_item_put. It also calls config_item_put on uvc_free, which
> is the only and right place to give back the reference. This patch fixes
> the issue by removing the extra config_item_put in uvc_alloc.
> 
> Fixes: 15a286a0bb08 (usb: gadget: uvc: add v4l2 enumeration api calls)

This is not a commit in any of my trees.  Are you sure it's a valid one?

thanks,

greg k-h

[PATCH v2] usb: gadget: uvc: don't put item still in use

[Michael Grzeschik]

With the patch "588b9e85609b (usb: gadget: uvc: add v4l2 enumeration api
calls)" the driver is keeping a list of configfs entries currently
configured. The list is used in uvc_v4l2 on runtime.

The driver now is giving back the list item just after it was referenced
with config_item_put. It also calls config_item_put on uvc_free, which
is the only and right place to give back the reference. This patch fixes
the issue by removing the extra config_item_put in uvc_alloc.

Fixes: 588b9e85609b (usb: gadget: uvc: add v4l2 enumeration api calls)
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v1 -> v2: - fixed commitish to valid one from usb-next tree

 drivers/usb/gadget/function/f_uvc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index e6948cf8def30b..8bc591431d153e 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -995,7 +995,6 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
 		goto err_config;
 
 	uvc->header = to_uvcg_streaming_header(h);
-	config_item_put(h);
 	if (!uvc->header->linked) {
 		mutex_unlock(&opts->lock);
 		kfree(uvc);
-- 
2.30.2

Re: [PATCH v2] usb: gadget: uvc: don't put item still in use

[Laurent Pinchart]

Hi Michael,

(Cc'ing Kieran and Dan)

Thank you for the patch.

On Fri, Sep 30, 2022 at 02:28:39PM +0200, Michael Grzeschik wrote:
> With the patch "588b9e85609b (usb: gadget: uvc: add v4l2 enumeration api
> calls)" the driver is keeping a list of configfs entries currently
> configured. The list is used in uvc_v4l2 on runtime.

s/on runtime/at runtime/

> The driver now is giving back the list item just after it was referenced
> with config_item_put. It also calls config_item_put on uvc_free, which
> is the only and right place to give back the reference. This patch fixes
> the issue by removing the extra config_item_put in uvc_alloc.
> 
> Fixes: 588b9e85609b (usb: gadget: uvc: add v4l2 enumeration api calls)

I still don't like this much :-( As shown by this fix, the additional
complexity that it incurs on the kernel side can cause bugs, and the
gain for userspace is quite minimal in my opinion, as parsing configfs
(or obtaining that information out-of-band through other means) will
still be needed anyway to handle controls properly (I think we have
agreed that userspace needs to handle the UVC requests in any case).

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v1 -> v2: - fixed commitish to valid one from usb-next tree
> 
>  drivers/usb/gadget/function/f_uvc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index e6948cf8def30b..8bc591431d153e 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -995,7 +995,6 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
>  		goto err_config;
>  
>  	uvc->header = to_uvcg_streaming_header(h);
> -	config_item_put(h);

Assuming we want to keep 588b9e85609b, the fix seems right.

>  	if (!uvc->header->linked) {
>  		mutex_unlock(&opts->lock);
>  		kfree(uvc);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] usb: gadget: uvc: don't put item still in use

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]

Hi Laurent,

On Fri, Sep 30, 2022 at 07:17:38PM +0300, Laurent Pinchart wrote:
>On Fri, Sep 30, 2022 at 02:28:39PM +0200, Michael Grzeschik wrote:
>> With the patch "588b9e85609b (usb: gadget: uvc: add v4l2 enumeration api
>> calls)" the driver is keeping a list of configfs entries currently
>> configured. The list is used in uvc_v4l2 on runtime.
>
>s/on runtime/at runtime/
>
>> The driver now is giving back the list item just after it was referenced
>> with config_item_put. It also calls config_item_put on uvc_free, which
>> is the only and right place to give back the reference. This patch fixes
>> the issue by removing the extra config_item_put in uvc_alloc.
>>
>> Fixes: 588b9e85609b (usb: gadget: uvc: add v4l2 enumeration api calls)
>
>I still don't like this much :-( As shown by this fix, the additional
>complexity that it incurs on the kernel side can cause bugs, and the
>gain for userspace is quite minimal in my opinion, as parsing configfs
>(or obtaining that information out-of-band through other means) will
>still be needed anyway to handle controls properly (I think we have
>agreed that userspace needs to handle the UVC requests in any case).

I understand your objections regarding the out-of-band configfs data.
While implementing the parser in the gstreamer uvcsink element I just
stumbled over this. It still needs to parse configfs just for
bInteraceNumber of the config and streaming interfaces. So actually with
the parser in the kernel, this was no issue, since this information is
already present there. I am more and more overthinking my latest
conviction of moving the whole parsing code to userspace.

Although, I understand that an partial parsing of the events in the
kernel for the format negotiaton is not ideal, the kernel parser is
still an valid option.

In our other discussion I mentioned a proper uvc-events API where the
kernel is already preparsing the gadget messages and creates events with
well defined event types. The userspace could then just run an simple
select over the event types and decide what to do.

What do you think about that? Any other ideas what we could do to
improve the uvc gadget?

>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>> v1 -> v2: - fixed commitish to valid one from usb-next tree
>>
>>  drivers/usb/gadget/function/f_uvc.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>> index e6948cf8def30b..8bc591431d153e 100644
>> --- a/drivers/usb/gadget/function/f_uvc.c
>> +++ b/drivers/usb/gadget/function/f_uvc.c
>> @@ -995,7 +995,6 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
>>  		goto err_config;
>>
>>  	uvc->header = to_uvcg_streaming_header(h);
>> -	config_item_put(h);
>
>Assuming we want to keep 588b9e85609b, the fix seems right.

In my opinion with this fix the topic to enumerate the formats,
framesizes and intervals with the v4l2 api is now functional and
everything but complex. So the patch 588b9e85609b is valid and should
stay.

>>  	if (!uvc->header->linked) {
>>  		mutex_unlock(&opts->lock);
>>  		kfree(uvc);
>
>-- 

You also mentioned code to merge SETUP and DATA events for control OUT
requests. Is this code anywhere available? Are you planing to send
this code anytime soon?

Thanks,
Michael

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] usb: gadget: uvc: don't put item still in use

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3225 bytes --]

Hi Laurent,

On Fri, Sep 30, 2022 at 10:44:49PM +0200, Michael Grzeschik wrote:
>On Fri, Sep 30, 2022 at 07:17:38PM +0300, Laurent Pinchart wrote:
>>On Fri, Sep 30, 2022 at 02:28:39PM +0200, Michael Grzeschik wrote:
>>>With the patch "588b9e85609b (usb: gadget: uvc: add v4l2 enumeration api
>>>calls)" the driver is keeping a list of configfs entries currently
>>>configured. The list is used in uvc_v4l2 on runtime.
>>
>>s/on runtime/at runtime/
>>
>>>The driver now is giving back the list item just after it was referenced
>>>with config_item_put. It also calls config_item_put on uvc_free, which
>>>is the only and right place to give back the reference. This patch fixes
>>>the issue by removing the extra config_item_put in uvc_alloc.
>>>
>>>Fixes: 588b9e85609b (usb: gadget: uvc: add v4l2 enumeration api calls)
>>
>>I still don't like this much :-( As shown by this fix, the additional
>>complexity that it incurs on the kernel side can cause bugs, and the
>>gain for userspace is quite minimal in my opinion, as parsing configfs
>>(or obtaining that information out-of-band through other means) will
>>still be needed anyway to handle controls properly (I think we have
>>agreed that userspace needs to handle the UVC requests in any case).
>
>I understand your objections regarding the out-of-band configfs data.
>While implementing the parser in the gstreamer uvcsink element I just
>stumbled over this. It still needs to parse configfs just for
>bInteraceNumber of the config and streaming interfaces. So actually with
>the parser in the kernel, this was no issue, since this information is
>already present there. I am more and more overthinking my latest
>conviction of moving the whole parsing code to userspace.
>
>Although, I understand that an partial parsing of the events in the
>kernel for the format negotiaton is not ideal, the kernel parser is
>still an valid option.
>
>In our other discussion I mentioned a proper uvc-events API where the
>kernel is already preparsing the gadget messages and creates events with
>well defined event types. The userspace could then just run an simple
>select over the event types and decide what to do.

I looked into the v4l2_events and its subscription options. Since the
UVC spec for class events only distinguishes requests for the control or
the streaming interface, we could stuff this information in the
v4l2_event.id field .

The kernel would only need to preparse the setup events and check for
wIndex of the ctrl message and set the v4l2_events id accordingly.

This id could always be fixed numbers like streaming (1) and control (0).
The userspace then would not need to parse the configfs for the
interface numbers anymore.

Since the uvc gadget is using private v4l2_event types anyway, using the
id field would make sense in my opinion.

Does this sound sane?

Thanks
Michael


-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]