On Tue, Nov 03, 2015 at 01:53:40PM +0100, Robert Baldyga wrote:
> So far it was decided during the bind process whether is iso altsetting
> included to f_sourcesink function or not. This decision was based on
> availability of isochronous endpoints.
>
> Since we can assemble gadget driver using composite framework and configfs
> from many different functions, availability of given type of endpoint
> can depend on selected components or even on their order in given
> configuration.
>
> This can result with non-obvious behavior - even small, seemingly unrelated
> change in gadget configuration can decide if we have second altsetting with
> iso endpoints in given sourcesink function instance or not.
>
> Because of this it's way better to have additional parameter allowing user
> to decide if he/she wants to have iso altsetting, and if iso altsetting is
> included, and there are no iso endpoints available, function bind will fail
> instead of silently allowing to have non-complete function bound.
Hi Robert,
Why another isoc_enabled parameter is needed instead of judging if it
is supported through gadget framework? Any use cases can't be supported
by current way?
Peter
>
> Signed-off-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
> ---
> drivers/usb/gadget/function/f_sourcesink.c | 99 ++++++++++++++++++++----------
> drivers/usb/gadget/function/g_zero.h | 3 +
> drivers/usb/gadget/legacy/zero.c | 6 ++
> 3 files changed, 77 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
> index d7646d3..1d6ec88 100644
> --- a/drivers/usb/gadget/function/f_sourcesink.c
> +++ b/drivers/usb/gadget/function/f_sourcesink.c
> @@ -56,6 +56,7 @@ struct f_sourcesink {
> unsigned isoc_maxpacket;
> unsigned isoc_mult;
> unsigned isoc_maxburst;
> + unsigned isoc_enabled;
> unsigned buflen;
> };
>
> @@ -347,17 +348,28 @@ sourcesink_bind(struct usb_configuration *c, struct usb_function *f)
>
> /* allocate bulk endpoints */
> ss->in_ep = usb_ep_autoconfig(cdev->gadget, &fs_source_desc);
> - if (!ss->in_ep) {
> -autoconf_fail:
> - ERROR(cdev, "%s: can't autoconfigure on %s\n",
> - f->name, cdev->gadget->name);
> - return -ENODEV;
> - }
> + if (!ss->in_ep)
> + goto autoconf_fail;
>
> ss->out_ep = usb_ep_autoconfig(cdev->gadget, &fs_sink_desc);
> if (!ss->out_ep)
> goto autoconf_fail;
>
> + /* support high speed hardware */
> + hs_source_desc.bEndpointAddress = fs_source_desc.bEndpointAddress;
> + hs_sink_desc.bEndpointAddress = fs_sink_desc.bEndpointAddress;
> +
> + /* support super speed hardware */
> + ss_source_desc.bEndpointAddress = fs_source_desc.bEndpointAddress;
> + ss_sink_desc.bEndpointAddress = fs_sink_desc.bEndpointAddress;
> +
> + if (!ss->isoc_enabled) {
> + fs_source_sink_descs[FS_ALT_IFC_1_OFFSET] = NULL;
> + hs_source_sink_descs[HS_ALT_IFC_1_OFFSET] = NULL;
> + ss_source_sink_descs[SS_ALT_IFC_1_OFFSET] = NULL;
> + goto no_iso;
> + }
> +
> /* sanity check the isoc module parameters */
> if (ss->isoc_interval < 1)
> ss->isoc_interval = 1;
> @@ -379,30 +391,14 @@ autoconf_fail:
> /* allocate iso endpoints */
> ss->iso_in_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_source_desc);
> if (!ss->iso_in_ep)
> - goto no_iso;
> + goto autoconf_fail;
>
> ss->iso_out_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_sink_desc);
> - if (!ss->iso_out_ep) {
> - usb_ep_autoconfig_release(ss->iso_in_ep);
> - ss->iso_in_ep = NULL;
> -no_iso:
> - /*
> - * We still want to work even if the UDC doesn't have isoc
> - * endpoints, so null out the alt interface that contains
> - * them and continue.
> - */
> - fs_source_sink_descs[FS_ALT_IFC_1_OFFSET] = NULL;
> - hs_source_sink_descs[HS_ALT_IFC_1_OFFSET] = NULL;
> - ss_source_sink_descs[SS_ALT_IFC_1_OFFSET] = NULL;
> - }
> + if (!ss->iso_out_ep)
> + goto autoconf_fail;
>
> if (ss->isoc_maxpacket > 1024)
> ss->isoc_maxpacket = 1024;
> -
> - /* support high speed hardware */
> - hs_source_desc.bEndpointAddress = fs_source_desc.bEndpointAddress;
> - hs_sink_desc.bEndpointAddress = fs_sink_desc.bEndpointAddress;
> -
> /*
> * Fill in the HS isoc descriptors from the module parameters.
> * We assume that the user knows what they are doing and won't
> @@ -419,12 +415,6 @@ no_iso:
> hs_iso_sink_desc.bInterval = ss->isoc_interval;
> hs_iso_sink_desc.bEndpointAddress = fs_iso_sink_desc.bEndpointAddress;
>
> - /* support super speed hardware */
> - ss_source_desc.bEndpointAddress =
> - fs_source_desc.bEndpointAddress;
> - ss_sink_desc.bEndpointAddress =
> - fs_sink_desc.bEndpointAddress;
> -
> /*
> * Fill in the SS isoc descriptors from the module parameters.
> * We assume that the user knows what they are doing and won't
> @@ -447,6 +437,7 @@ no_iso:
> (ss->isoc_mult + 1) * (ss->isoc_maxburst + 1);
> ss_iso_sink_desc.bEndpointAddress = fs_iso_sink_desc.bEndpointAddress;
>
> +no_iso:
> ret = usb_assign_descriptors(f, fs_source_sink_descs,
> hs_source_sink_descs, ss_source_sink_descs);
> if (ret)
> @@ -459,6 +450,11 @@ no_iso:
> ss->iso_in_ep ? ss->iso_in_ep->name : "<none>",
> ss->iso_out_ep ? ss->iso_out_ep->name : "<none>");
> return 0;
> +
> +autoconf_fail:
> + ERROR(cdev, "%s: can't autoconfigure on %s\n",
> + f->name, cdev->gadget->name);
> + return -ENODEV;
> }
>
> static void
> @@ -868,6 +864,7 @@ static struct usb_function *source_sink_alloc_func(
> ss->isoc_maxpacket = ss_opts->isoc_maxpacket;
> ss->isoc_mult = ss_opts->isoc_mult;
> ss->isoc_maxburst = ss_opts->isoc_maxburst;
> + ss->isoc_enabled = ss_opts->isoc_enabled;
> ss->buflen = ss_opts->bulk_buflen;
>
> ss->function.name = "source/sink";
> @@ -1125,6 +1122,45 @@ static struct f_ss_opts_attribute f_ss_opts_isoc_maxburst =
> f_ss_opts_isoc_maxburst_show,
> f_ss_opts_isoc_maxburst_store);
>
> +static ssize_t f_ss_opts_isoc_enabled_show(struct f_ss_opts *opts, char *page)
> +{
> + int result;
> +
> + mutex_lock(&opts->lock);
> + result = sprintf(page, "%u\n", opts->isoc_enabled);
> + mutex_unlock(&opts->lock);
> +
> + return result;
> +}
> +
> +static ssize_t f_ss_opts_isoc_enabled_store(struct f_ss_opts *opts,
> + const char *page, size_t len)
> +{
> + int ret;
> + bool enabled;
> +
> + mutex_lock(&opts->lock);
> + if (opts->refcnt) {
> + ret = -EBUSY;
> + goto end;
> + }
> +
> + ret = strtobool(page, &enabled);
> + if (ret)
> + goto end;
> +
> + opts->isoc_enabled = enabled;
> + ret = len;
> +end:
> + mutex_unlock(&opts->lock);
> + return ret;
> +}
> +
> +static struct f_ss_opts_attribute f_ss_opts_isoc_enabled =
> + __CONFIGFS_ATTR(isoc_enabled, S_IRUGO | S_IWUSR,
> + f_ss_opts_isoc_enabled_show,
> + f_ss_opts_isoc_enabled_store);
> +
> static ssize_t f_ss_opts_bulk_buflen_show(struct f_ss_opts *opts, char *page)
> {
> int result;
> @@ -1170,6 +1206,7 @@ static struct configfs_attribute *ss_attrs[] = {
> &f_ss_opts_isoc_maxpacket.attr,
> &f_ss_opts_isoc_mult.attr,
> &f_ss_opts_isoc_maxburst.attr,
> + &f_ss_opts_isoc_enabled.attr,
> &f_ss_opts_bulk_buflen.attr,
> NULL,
> };
> diff --git a/drivers/usb/gadget/function/g_zero.h b/drivers/usb/gadget/function/g_zero.h
> index 15f1809..8a99071 100644
> --- a/drivers/usb/gadget/function/g_zero.h
> +++ b/drivers/usb/gadget/function/g_zero.h
> @@ -10,6 +10,7 @@
> #define GZERO_QLEN 32
> #define GZERO_ISOC_INTERVAL 4
> #define GZERO_ISOC_MAXPACKET 1024
> +#define GZERO_ISOC_ENABLED 1
>
> struct usb_zero_options {
> unsigned pattern;
> @@ -17,6 +18,7 @@ struct usb_zero_options {
> unsigned isoc_maxpacket;
> unsigned isoc_mult;
> unsigned isoc_maxburst;
> + unsigned isoc_enabled;
> unsigned bulk_buflen;
> unsigned qlen;
> };
> @@ -28,6 +30,7 @@ struct f_ss_opts {
> unsigned isoc_maxpacket;
> unsigned isoc_mult;
> unsigned isoc_maxburst;
> + unsigned isoc_enabled;
> unsigned bulk_buflen;
>
> /*
> diff --git a/drivers/usb/gadget/legacy/zero.c b/drivers/usb/gadget/legacy/zero.c
> index 37a4100..3579310 100644
> --- a/drivers/usb/gadget/legacy/zero.c
> +++ b/drivers/usb/gadget/legacy/zero.c
> @@ -66,6 +66,7 @@ module_param(loopdefault, bool, S_IRUGO|S_IWUSR);
> static struct usb_zero_options gzero_options = {
> .isoc_interval = GZERO_ISOC_INTERVAL,
> .isoc_maxpacket = GZERO_ISOC_MAXPACKET,
> + .isoc_enabled = GZERO_ISOC_ENABLED,
> .bulk_buflen = GZERO_BULK_BUFLEN,
> .qlen = GZERO_QLEN,
> };
> @@ -249,6 +250,10 @@ module_param_named(isoc_maxburst, gzero_options.isoc_maxburst, uint,
> S_IRUGO|S_IWUSR);
> MODULE_PARM_DESC(isoc_maxburst, "0 - 15 (ss only)");
>
> +module_param_named(isoc_enabled, gzero_options.isoc_enabled, uint,
> + S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(isoc_enabled, "0 - disabled, 1 - enabled");
> +
> static struct usb_function *func_lb;
> static struct usb_function_instance *func_inst_lb;
>
> @@ -284,6 +289,7 @@ static int zero_bind(struct usb_composite_dev *cdev)
> ss_opts->isoc_maxpacket = gzero_options.isoc_maxpacket;
> ss_opts->isoc_mult = gzero_options.isoc_mult;
> ss_opts->isoc_maxburst = gzero_options.isoc_maxburst;
> + ss_opts->isoc_enabled = gzero_options.isoc_enabled;
> ss_opts->bulk_buflen = gzero_options.bulk_buflen;
>
> func_ss = usb_get_function(func_inst_ss);
> --
> 1.9.1
>
> --
> 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
--
Best Regards,
Peter Chen
--
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
