Hi Kieran,
On Wednesday, 1 August 2018 12:58:40 EEST Kieran Bingham wrote:
> On 01/08/18 01:29, Laurent Pinchart wrote:
> > The UVC configfs implementation creates all groups as global static
> > variables. This prevents creationg of multiple UVC function instances,
>
> /creationg/creation/
>
> > as they would all require their own configfs group instances.
> >
> > Fix this by allocating all groups dynamically. To avoid duplicating code
> > around, extend the config_item_type structure with group name and
> > children, and implement helper functions to create children
> > automatically for most groups.
> >
> > Signed-off-by: Laurent Pinchart <[email protected]>
>
> I'm struggling to see what paths free all of the dynamically created
> children in this patch.
>
> Is this already supported by the config_group framework?
>
> I see a reference to config_group_put(&opts->func_inst.group); in one
> error path - but that's about it.
>
> Am I missing something nice and obvious? (or is it already handled by
> framework code not in this patch)
>
>
> In fact, I can't see how it could be handled by core - as the children
> are added to a new structure you have created.
>
> I'll let you look into this :)
I did, for a whole day :-S It wasn't easy as the configfs documentation is
quite terse, and doesn't clearly explain when and how references should be
acquired and released. I'll post a v2 shortly.
> > ---
> >
> > drivers/usb/gadget/function/f_uvc.c | 8 +-
> > drivers/usb/gadget/function/uvc_configfs.c | 480 ++++++++++++++----------
> > 2 files changed, 282 insertions(+), 206 deletions(-)
[snip]
> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> > b/drivers/usb/gadget/function/uvc_configfs.c index
> > dbc95c9558de..e019ea317c7a 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > +++ b/drivers/usb/gadget/function/uvc_configfs.c
[snip]
> > -static struct config_group uvcg_terminal_grp;
> > -
> > -static const struct config_item_type uvcg_terminal_grp_type = {
> > - .ct_owner = THIS_MODULE,
> > +static const struct uvcg_config_group_type uvcg_terminal_grp_type = {
> > + .type = {
> > + .ct_owner = THIS_MODULE,
> > + },
> > + .name = "terminal",
> > + .children = (const struct uvcg_config_group_type*[]) {
>
> Is this cast really needed? Or is it just to constify the array ?
It's needed to make the expression within the curly braces an array that is
then turned into a pointer to initialize the children field, which is defined
as
const struct uvcg_config_group_type **children;
Without that you would get the following compilation errors.
drivers/usb/gadget/function/uvc_configfs.c:557:2: error: braces around scalar
initializer [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:557:2: error: (near initialization
for ‘uvcg_terminal_grp_type.children’) [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:558:3: error: initialization from
incompatible pointer type [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:558:3: error: (near initialization
for ‘uvcg_terminal_grp_type.children’) [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:559:3: error: excess elements in
scalar initializer [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:559:3: error: (near initialization
for ‘uvcg_terminal_grp_type.children’) [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:560:3: error: excess elements in
scalar initializer [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:560:3: error: (near initialization
for ‘uvcg_terminal_grp_type.children’) [-Werror]
Such a syntax removes the need to declare the array externally to
uvcg_terminal_grp_type as
static const struct uvcg_config_group_type *uvcg_terminal_grp_children[] = {
&uvcg_camera_grp_type,
&uvcg_output_grp_type,
NULL,
};
static const struct uvcg_config_group_type uvcg_terminal_grp_type = {
.type = {
.ct_owner = THIS_MODULE,
},
.name = "terminal",
.children = uvcg_terminal_grp_children,
};
> > + &uvcg_camera_grp_type,
> > + &uvcg_output_grp_type,
> > + NULL,
> > + },
> > };
[snip]
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
