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: 2332 bytes --]

On Tue, Nov 29, 2022 at 02:02:02PM +0200, Laurent Pinchart wrote:
>Hi Michael,
>
>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?

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