Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1032341AbbKFH0Z (ORCPT ); Fri, 6 Nov 2015 02:26:25 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:32265 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1032082AbbKFH0X (ORCPT ); Fri, 6 Nov 2015 02:26:23 -0500 X-AuditID: cbfec7f4-f79c56d0000012ee-5d-563c561d70e6 Subject: Re: [PATCH 01/23] usb: gadget: f_sourcesink: make ISO altset user-selectable To: Peter Chen References: <1446555242-3733-1-git-send-email-r.baldyga@samsung.com> <1446555242-3733-2-git-send-email-r.baldyga@samsung.com> <20151106030506.GB12560@shlinux2> Cc: balbi@ti.com, gregkh@linuxfoundation.org, andrzej.p@samsung.com, m.szyprowski@samsung.com, b.zolnierkie@samsung.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org From: Robert Baldyga Message-id: <563C561B.6060709@samsung.com> Date: Fri, 06 Nov 2015 08:26:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-version: 1.0 In-reply-to: <20151106030506.GB12560@shlinux2> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrLLMWRmVeSWpSXmKPExsVy+t/xy7qyYTZhBuuPMlvMetnOYrFxxnpW i4P36y2aF69ns7i8aw6bxaJlrcwWa4/cZbc4NvsvkwOHx7/D/Uwe++euYffo27KK0eP4je1M Hp83yQWwRnHZpKTmZJalFunbJXBlTPnHX3AzpOL08QmsDYy7XLsYOTkkBEwk7jXsYoewxSQu 3FvP1sXIxSEksJRR4tLdQ2AJIYFnjBK7XpWA2MIC4RJH+nuZQGwRAU2JdeePMUE0LGaU6Fr3 jB3EYRbYwiix/v41FpAqNgEdiS3fJzCC2LwCWhIz3v8DWsHBwSKgKvHqmjFIWFQgQmLihAZW iBJBiR+T74G1cgroSRz/t5EJpJwZyL5/UQskzCwgL7F5zVvmCYwCs5B0zEKomoWkagEj8ypG 0dTS5ILipPRcQ73ixNzi0rx0veT83E2MkED/soNx8TGrQ4wCHIxKPLw3lliHCbEmlhVX5h5i lOBgVhLhlWO2CRPiTUmsrEotyo8vKs1JLT7EKM3BoiTOO3fX+xAhgfTEktTs1NSC1CKYLBMH p1QDYwPTk5k+F3STmp+tmy2k0qZZlaMZdOz2qg/T3LTVl/bNnMxUd+WLr34yf0Nn4NU/tyPY 4r41nTr3x++RdNVd44dz/JY4xc3JPJ71/Oh1zg8zlG8+k3wlFs/3NXtm0a8Lbuf7HFSlDjAZ PZ/1OHBW1o2krIXzef/Mui4psao+jPGu4rzT/T8mqymxFGckGmoxFxUnAgDiogOdcAIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10790 Lines: 301 On 11/06/2015 04:05 AM, Peter Chen wrote: > 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? > It's because guessing during bind process leads to non-obvious behavior - we can have iso altsetting included into function or not depending on many seemingly unrelated factors. Moreover SourceSink, which is in fact testing Function, is the only Function which implements conditional altsetting enabling, and we definitely don't want testing function to behave that much differently from another USB Functions. The third reason is that modifying descriptors set depending on availability of given endpoint type during bind process complicates bind process automatization, which I implement in this patch set. After all, I don't know any real USB device doing such strange thing. Thanks, Robert >> >> Signed-off-by: Robert Baldyga >> --- >> 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 : "", >> ss->iso_out_ep ? ss->iso_out_ep->name : ""); >> 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/