[3/8] usb: gadget: uvc: configfs: Allocate groups dynamically

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 <laurent.pinchart@ideasonboard.com>
> 
> 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]