Hi Andrzej,
On Friday 05 December 2014 15:17:43 Andrzej Pietrasiewicz wrote:
> Hi Laurent,
>
> thank you for your review.
>
> I generally agree with your comments but please see inline
> for some explanations.
>
> W dniu 28.11.2014 o 00:40, Laurent Pinchart pisze:
> > Hi Andrzej,
> >
> > Thank you for the patch.
> >
> > On Wednesday 24 September 2014 15:26:43 Andrzej Pietrasiewicz wrote:
> >> Add support for using the uvc function as a component of USB gadgets
> >> composed with configfs.
> >>
> >> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> >> ---
>
> <snip>
>
> > This should be 3.19. My fault, sorry :-/
>
> I'm glad you have managed to review.
>
> Now it looks like
> all other functions which are available as separate f_xyz.c files are
> already converted, so it would be nice if uvc made its way into 3.20.
>
> <snip>
>
> >> +What: /config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg
> >> +Date: Nov 2014
> >> +KernelVersion: 3.18
> >> +Description: MJPEG format descriptors
> >> +
> >> +What: /config/usb
> >> -gadget/gadget/functions/uvc.name/streaming/mjpeg/name
> >
> > I'm not too familiar with configfs, should the attribute description
> > specify what values are recommended and/or acceptable for "name" ?
>
> Anything? The video format is determined by the parent directory name
> (mjpeg in this case). The name can be pretty much anything a filesystem
> permits. The association of formats is done with symbolic links.
>
> <snip>
>
> >> @@ -778,6 +873,7 @@ static struct usb_function *uvc_alloc(struct
> >> usb_function_instance *fi)
> >> {
> >> struct uvc_device *uvc;
> >> struct f_uvc_opts *opts;
> >> + struct uvc_descriptor_header **strm_cls;
> >>
> >> uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
> >>
> >> @@ -786,11 +882,29 @@ static struct usb_function *uvc_alloc(struct
> >> usb_function_instance *fi)
> >> uvc->state = UVC_STATE_DISCONNECTED;
> >>
> >> opts = fi_to_f_uvc_opts(fi);
> >>
> >> + mutex_lock(&opts->lock);
> >
> > What does the mutex protect against ? Modifications of opts->
> > uvc_*_streaming_cls by configfs ? You only copy pointers around, what
> > prevents the descriptors from being modified after you unlock the mutex ?
> >
> >> + if (opts->uvc_fs_streaming_cls) {
> >> + strm_cls = opts->uvc_fs_streaming_cls;
> >> + opts->fs_streaming =
> >> + (const struct uvc_descriptor_header * const *)strm_cls;
> >> + }
> >> + if (opts->uvc_hs_streaming_cls) {
> >> + strm_cls = opts->uvc_hs_streaming_cls;
> >> + opts->hs_streaming =
> >> + (const struct uvc_descriptor_header * const *)strm_cls;
> >> + }
> >> + if (opts->uvc_ss_streaming_cls) {
> >> + strm_cls = opts->uvc_ss_streaming_cls;
> >> + opts->ss_streaming =
> >> + (const struct uvc_descriptor_header * const *)strm_cls;
> >> + }
> >> +
>
> Also modifications of opts->fs_streaming & friends.
My point is that the above code only copies pointers to descriptors created
through configfs, not their content. There doesn't seem to be anything
preventing the user from modifying the descriptors after the pointers have
been copied. Maybe I'm missing something obvious.
> <snip>
>
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> >> b/drivers/usb/gadget/function/uvc_configfs.c new file mode 100644
> >> index 0000000..33d92ab
> >> --- /dev/null
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -0,0 +1,2439 @@
> >
> > I can't really review this in a reasonable time frame at the moment I'm
> > afraid>
> > :-/ We'll have to fix bugs as we go.
> >
> > [snip]
>
> <snip>
>
> You never know with a company what you are assigned to, but I'm really
> hoping to stick around usb gadgets next year so I intend to fix any bugs
> that are discovered, especially if they are found in my code ;-) So I
> think your suggestion seems reasonable, given the amount of time which
> has passed since we first talked in New Orleans about the idea of adding
> configfs support to the uvc function.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
