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: > >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? It still adds pointless complexity to the kernel. I see no reason at all for this. You're trying to guard against userspace bugs by returning an error code to the application that has bugs in the first place. Why would it then handle the error correctly ? Please drop these changes. -- Regards, Laurent Pinchart
