Date: Fri, 6 Nov 2015 11:05:07 +0800
From: Peter Chen <peter.chen@freescale.com>
To: Robert Baldyga <r.baldyga@samsung.com>
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>
Subject: Re: [PATCH 01/23] usb: gadget: f_sourcesink: make ISO altset
 user-selectable
Message-ID: <20151106030506.GB12560@shlinux2>
References: <1446555242-3733-1-git-send-email-r.baldyga@samsung.com>
 <1446555242-3733-2-git-send-email-r.baldyga@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1446555242-3733-2-git-send-email-r.baldyga@samsung.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
SpamDiagnosticOutput: 1:23
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Nov 2015 03:07:10.8612
 (UTC)
X-MS-Exchange-CrossTenant-Id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=710a03f5-10f6-4d38-9ff4-a80b81da590d;Ip=[192.88.168.50];Helo=[tx30smr01.am.freescale.net]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR0301MB2003
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 9806
Lines: 287

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@samsung.com>
> ---
>  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@vger.kernel.org
> 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-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/