Re: [PATCH v7] usb: gadget: uvc: add validate and fix function for uvc response

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Tue, Nov 29, 2022 at 04:22:59PM +0100, Michael Grzeschik wrote:
>On Tue, Nov 29, 2022 at 02:02:02PM +0200, Laurent Pinchart wrote:
>>On Tue, Nov 29, 2022 at 11:23:08AM +0100, Michael Grzeschik wrote:
>>>On Tue, Nov 29, 2022 at 05:10:24AM +0200, Laurent Pinchart wrote:
>>>> On Mon, Nov 28, 2022 at 11:31:25AM +0100, Michael Grzeschik wrote:
>>>>> When the userspace gets the setup requests for UVC_GET_CUR UVC_GET_MIN,
>>>>> UVC_GET_MAX, UVC_GET_DEF it will fill out the ctrl response. This data
>>>>> needs to be validated. Since the kernel also knows the limits for valid
>>>>> cases, it can fixup the values in case the userspace is setting invalid
>>>>> data.
>>>>
>>>> Why is this a good idea ?
>>>
>>>Why is it not? We don't want the userspace to communicate other things
>>>to the host than what is configured in the configfs. If you only object
>>>the explanation, then I will improve the commit message and send an
>>>fixed v8. If you have more objections please share your doubts, thanks.
>>
>>What bothers me is that this patch silently clamps invalid value, trying
>>to hide the gadget userspace error from the host. It may allow the host
>>to proceed one step further, but if the gadget userspace got it wrong in
>>the first place, there's a very high chance it won't do the right thing
>>in the next step anyway. This will make debugging more complicated,
>>while at the same time not bringing much value.
>
>I discussed this and we came up with a better approach. When the
>userspace will send UVCIOC_SEND_RESPONSE we can return with a negativ
>return value. Like EAGAIN if the validation was seeeing some trouble
>with the userspaces uvc_streaming_control feedback to the host.
>
>The validation code will then still fixup the data, but instead of
>transfering this manipulated answer to the host, it will return the
>changes to the application with EAGAIN. So now the userspace can
>react to it and it should even point out misconfigurations between
>kernel and userspace and so will simplify the debugging.
>
>How about that?

While implementing this I came across the problem that the
UVCIOC_SEND_RESPONSE is handled in the vidioc_default handler.
But for this handler we can not set flag INFO_FL_ALWAYS_COPY like
for common v4l2_ioctls. :(

I think this is still worth a path to go, but I am currently out
of ideas how to achieve it. Help for this is much appreciated.

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