Re: [PATCH v2 0/4] usb: gadget: uvc: parse configfs entries and implement v4l2 enum api calls

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, May 12, 2025 at 12:19:07PM +0200, Krzysztof Opasiak wrote:
> Hi Greg,
> 
> On 4.12.2022 09:29, Greg KH wrote:
> > On Sat, Dec 03, 2022 at 11:26:14PM +0200, Laurent Pinchart wrote:
> > > Hi Michael,
> > > 
> > > On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote:
> > > > This series improves the uvc video gadget by parsing the configfs
> > > > entries. With the configfs data, the userspace now is able to use simple
> > > > v4l2 api calls like enum and try_format to check for valid configurations
> > > > initially set by configfs.
> > > 
> > > I've realized that this whole series got merged, despite my multiple
> > > attempts to explain why I think it's not a good idea. The UVC gadget
> > > requires userspace support, and there's no point in trying to move all
> > > these things to the kernel side. It only bloats the kernel, makes the
> > > code more complex, more difficult to maintain, and will make UVC 1.5
> > > support more difficult.
> > 
> > I can easily revert them, but I did not see any objections to them
> > originally and so I merged them as is the normal method :)
> > 
> 
> I know that it's already 2025 and I'm very late to the game but this series
> breaks our userspace scripts as it implicitly adds a requirement to name a
> streaming header directory as "h" which previously was a user-selected name.
> This requirement is coming from here:
> 
> +
> +       streaming = config_group_find_item(&opts->func_inst.group,
> "streaming");
> +       if (!streaming)
> +               goto err_config;
> +
> +       header = config_group_find_item(to_config_group(streaming),
> "header");
> +       config_item_put(streaming);
> +       if (!header)
> +               goto err_config;
> +
> +       h = config_group_find_item(to_config_group(header), "h");
> +       config_item_put(header);
> +       if (!h)
> +               goto err_config;
> 
> If you name this directory as "header" gadget just fails to link to a
> configuration. Although this isn't a big deal on its own as we could simply
> rename the dir in our code but this is just the tip of the iceberg.
> 
> To my understanding, this patch broke an important feature of UVC ConfigFS
> interface which is allowing to present different list of available formats
> for different USB speeds as for example, it does not make sense to expose
> 1080p30 in high speed as it simply just does not fit into the ISO bandwidth
> of HS. For example what we've been using previously:
> 
> mkdir uvc.nf/streaming/uncompressed/hsu
> mkdir uvc.nf/streaming/uncompressed/hsu/360p
> 
> mkdir uvc.nf/streaming/uncompressed/ssu
> mkdir uvc.nf/streaming/uncompressed/ssu/360p
> mkdir uvc.nf/streaming/uncompressed/ssu/720p
> mkdir uvc.nf/streaming/uncompressed/ssu/1080p
> 
> symlink uvc.nf/streaming/uncompressed/hsu \
>         uvc.nf/streaming/header/hsh/hsu
> 
> symlink uvc.nf/streaming/uncompressed/ssu \
>         uvc.nf/streaming/header/ssh/hsu
> 
> symlink uvc.nf/streaming/header/hsh \
>         uvc.nf/streaming/class/hs/h
> symlink uvc.nf/streaming/header/ssh \
>         uvc.nf/streaming/class/ss/h
> 
> no longer works as the patch requires presence of "h" directory inside
> "streaming/header" and even if we rename one of the headers directory to h
> the patch would be actually wrong as it would expose via v4l2 calls only
> formats available in the "h" directory and not in any other. (and at least
> on our adroid kernel it makes the kernel crash but haven't tested if that
> would be the case for mainline as well)
> 
> Given the limitations of the v4l2 interface I kind of don't see a way how we
> could fix this series to present proper formats to the userspace using v4l2
> calls as the list of formats can change when the speed changes and userspace
> would have no way of knowing that.
> 
> Given that I'd like to suggest that it seems to actually make sense to
> revert this unless there are some ideas how to fix it.

Sorry about this, can you submit a patch series that reverts the
offending commits?  As it was years ago, I don't exactly know what you
are referring to anymore.

thanks,

greg k-h