Re: [PATCH v2 1/9] usb: gadget: uvc: Make bSourceID read/write

[Dan Scally <dan.scally@xxxxxxxxxxxxxxxx>]

Hi Laurent

On 29/12/2022 00:30, Laurent Pinchart wrote:
> On Thu, Dec 29, 2022 at 02:30:04AM +0200, Laurent Pinchart wrote:
> > Hi Dan,
> >
> > Thank you for the patch.
> >
> > On Mon, Nov 21, 2022 at 09:25:09AM +0000, Daniel Scally wrote:
> > > At the moment, the UVC function graph is hardcoded IT -> PU -> OT.
> > > To add XU support we need the ability to insert the XU descriptors
> > > into the chain. To facilitate that, make the output terminal's
> > > bSourceID attribute writeable so that we can configure its source.
> > >
> > > Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> > > ---
> > > Changes in v2:
> > >
> > > 	- Updated the ABI Documentation to reflect the change.
> > >
> > >   .../ABI/testing/configfs-usb-gadget-uvc       |  2 +-
> > >   drivers/usb/gadget/function/uvc_configfs.c    | 57 ++++++++++++++++++-
> > >   2 files changed, 57 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > > index 611b23e6488d..feb3f2cc0c16 100644
> > > --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > > +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> > > @@ -52,7 +52,7 @@ Date:		Dec 2014
> > >   KernelVersion:	4.0
> > >   Description:	Default output terminal descriptors
> > >   
> > > -		All attributes read only:
> > > +		All attributes read only except bSourceID:
> > >   
> > >   		==============	=============================================
> > >   		iTerminal	index of string descriptor
> > > diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> > > index 4303a3283ba0..af4258120f9a 100644
> > > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > > +++ b/drivers/usb/gadget/function/uvc_configfs.c
> > > @@ -483,11 +483,66 @@ UVC_ATTR_RO(uvcg_default_output_, cname, aname)
> > >   UVCG_DEFAULT_OUTPUT_ATTR(b_terminal_id, bTerminalID, 8);
> > >   UVCG_DEFAULT_OUTPUT_ATTR(w_terminal_type, wTerminalType, 16);
> > >   UVCG_DEFAULT_OUTPUT_ATTR(b_assoc_terminal, bAssocTerminal, 8);
> > > -UVCG_DEFAULT_OUTPUT_ATTR(b_source_id, bSourceID, 8);
> > >   UVCG_DEFAULT_OUTPUT_ATTR(i_terminal, iTerminal, 8);
> > >   
> > >   #undef UVCG_DEFAULT_OUTPUT_ATTR
> > >   
> > > +static ssize_t uvcg_default_output_b_source_id_show(struct config_item *item,
> > > +						    char *page)
> > > +{
> > > +	struct config_group *group = to_config_group(item);
> > > +	struct f_uvc_opts *opts;
> > > +	struct config_item *opts_item;
> > > +	struct mutex *su_mutex = &group->cg_subsys->su_mutex;
> > > +	struct uvc_output_terminal_descriptor *cd;
> > > +	int result;
> > > +
> > > +	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> > > +
> > > +	opts_item = group->cg_item.ci_parent->ci_parent->ci_parent->ci_parent;
> > > +	opts = to_f_uvc_opts(opts_item);
> > > +	cd = &opts->uvc_output_terminal;
> > > +
> > > +	mutex_lock(&opts->lock);
> > > +	result = sprintf(page, "%u\n", le8_to_cpu(cd->bSourceID));
> > > +	mutex_unlock(&opts->lock);
> > > +
> > > +	mutex_unlock(su_mutex);
> > > +
> > > +	return result;
> > > +}
> > > +
> > > +static ssize_t uvcg_default_output_b_source_id_store(struct config_item *item,
> > > +						     const char *page, size_t len)
> > > +{
> > > +	struct config_group *group = to_config_group(item);
> > > +	struct f_uvc_opts *opts;
> > > +	struct config_item *opts_item;
> > > +	struct mutex *su_mutex = &group->cg_subsys->su_mutex;
> > > +	struct uvc_output_terminal_descriptor *cd;
> > > +	int result;
> > > +	u8 num;
> > > +
> > > +	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> > > +
> > > +	opts_item = group->cg_item.ci_parent->ci_parent->ci_parent->ci_parent;
> > > +	opts = to_f_uvc_opts(opts_item);
> > > +	cd = &opts->uvc_output_terminal;
> > > +
> > > +	result = kstrtou8(page, 0, &num);
> > > +	if (result)
> > > +		return result;
> > > +
> > > +	mutex_lock(&opts->lock);
> > > +	cd->bSourceID = num;
> > > +	mutex_unlock(&opts->lock);
> > > +
> > > +	mutex_unlock(su_mutex);
> > > +
> > > +	return len;
> > > +}
> > > +UVC_ATTR(uvcg_default_output_, b_source_id, bSourceID);
> > Feel free to shoot this idea down if it's a bad one: given that the
> > bSourceID attributes serve to create a pipeline by linking entities,
> > would it make sense to model these links with symlinks ?
> I forgot to mention that this would handle the bSourceID attribute
> automatically, avoiding mistakes. But maybe we're over-engineering all
> this...


Hmmmm, maybe. I lean towards over-engineered, but not strongly so. 
Assuming the string descriptors stand as is, the .allow_link() for XUs 
would have to account for linking to both a string and another unit. The 
position of the Source ID field in the Unit Descriptors differs, and for 
the XUs is nested behind another struct...and properly supporting XUs as 
specified means we'd need to allow multiple links in to an XU, all of 
which might make it a bit more complicated than it is helpful. Happy to 
be convinced otherwise though; I'm on the fence about it.

> > > +
> > >   static struct configfs_attribute *uvcg_default_output_attrs[] = {
> > >   	&uvcg_default_output_attr_b_terminal_id,
> > >   	&uvcg_default_output_attr_w_terminal_type,