[PATCH] USB: Removing the use of VLAIS from the gadget driver

[PATCH] USB: Removing the use of VLAIS from the gadget driver

[behanw]

From: Behan Webster <behanw@converseincode.com>

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch removes the use of VLAIS in the gadget driver.

Signed-off-by: Mark Charlebois <charlebm@gmail.com>
Signed-off-by: Behan Webster <behanw@converseincode.com>
---
 drivers/usb/gadget/f_fs.c | 128 +++++++++++++++++++++++++++-------------------
 1 file changed, 76 insertions(+), 52 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index f394f29..4b872c4 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -30,7 +30,6 @@
 
 #define FUNCTIONFS_MAGIC	0xa647361 /* Chosen by a honest dice roll ;) */
 
-
 /* Debugging ****************************************************************/
 
 #ifdef VERBOSE_DEBUG
@@ -214,6 +213,8 @@ struct ffs_data {
 	/* ids in stringtabs are set in functionfs_bind() */
 	const void			*raw_strings;
 	struct usb_gadget_strings	**stringtabs;
+	struct usb_gadget_strings	*stringtab;
+	struct usb_string		*strings;
 
 	/*
 	 * File system's super block, write once when file system is
@@ -263,7 +264,10 @@ struct ffs_function {
 
 	struct ffs_ep			*eps;
 	u8				eps_revmap[16];
+	struct usb_descriptor_header	**fs_descs;
+	struct usb_descriptor_header	**hs_descs;
 	short				*interfaces_nums;
+	char				*raw_descs;
 
 	struct usb_function		function;
 };
@@ -1345,6 +1349,8 @@ static void ffs_data_clear(struct ffs_data *ffs)
 	kfree(ffs->raw_descs);
 	kfree(ffs->raw_strings);
 	kfree(ffs->stringtabs);
+	kfree(ffs->stringtab);
+	kfree(ffs->strings);
 }
 
 static void ffs_data_reset(struct ffs_data *ffs)
@@ -1357,6 +1363,8 @@ static void ffs_data_reset(struct ffs_data *ffs)
 	ffs->raw_descs = NULL;
 	ffs->raw_strings = NULL;
 	ffs->stringtabs = NULL;
+	ffs->stringtab = NULL;
+	ffs->strings = NULL;
 
 	ffs->raw_descs_length = 0;
 	ffs->raw_fs_descs_length = 0;
@@ -1528,12 +1536,10 @@ static void ffs_func_free(struct ffs_function *func)
 	ffs_data_put(func->ffs);
 
 	kfree(func->eps);
-	/*
-	 * eps and interfaces_nums are allocated in the same chunk so
-	 * only one free is required.  Descriptors are also allocated
-	 * in the same chunk.
-	 */
-
+	kfree(func->fs_descs);
+	kfree(func->hs_descs);
+	kfree(func->interfaces_nums);
+	kfree(func->raw_descs);
 	kfree(func);
 }
 
@@ -1907,33 +1913,35 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
 		return 0;
 	}
 
-	/* Allocate everything in one chunk so there's less maintenance. */
 	{
-		struct {
-			struct usb_gadget_strings *stringtabs[lang_count + 1];
-			struct usb_gadget_strings stringtab[lang_count];
-			struct usb_string strings[lang_count*(needed_count+1)];
-		} *d;
 		unsigned i = 0;
-
-		d = kmalloc(sizeof *d, GFP_KERNEL);
-		if (unlikely(!d)) {
+		usb_gadget_strings **stringtabs = NULL;
+		usb_gadget_strings *stringtab = NULL;
+		usb_string *strings = NULL;
+
+		stringtabs = kmalloc(sizeof(*stringtabs)*(lang_count + 1),
+			GFP_KERNEL);
+		stringtab = kmalloc(sizeof(*stringtab)*(lang_count),
+			GFP_KERNEL);
+		strings = kmalloc(sizeof(*strings)
+			* (lang_count * (needed_count + 1)), GFP_KERNEL);
+		if (unlikely(!stringtabs || !stringtab || !strings)) {
+			kfree(stringtabs);
+			kfree(stringtab);
+			kfree(strings);
 			kfree(_data);
 			return -ENOMEM;
 		}
-
-		stringtabs = d->stringtabs;
-		t = d->stringtab;
+		b = stringtabs;
+		t = stringtab;
 		i = lang_count;
 		do {
-			*stringtabs++ = t++;
+			*b++ = t++;
 		} while (--i);
-		*stringtabs = NULL;
+		*b = NULL;
 
-		stringtabs = d->stringtabs;
-		t = d->stringtab;
-		s = d->strings;
-		strings = s;
+		t = stringtab;
+		s = strings;
 	}
 
 	/* For each language */
@@ -1991,12 +1999,16 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
 
 	/* Done! */
 	ffs->stringtabs = stringtabs;
+	ffs->stringtab = stringtab;
+	ffs->strings = strings;
 	ffs->raw_strings = _data;
 
 	return 0;
 
 error_free:
 	kfree(stringtabs);
+	kfree(stringtab);
+	kfree(strings);
 error:
 	kfree(_data);
 	return -EINVAL;
@@ -2207,17 +2219,12 @@ static int ffs_func_bind(struct usb_configuration *c,
 
 	int ret;
 
-	/* Make it a single chunk, less management later on */
-	struct {
-		struct ffs_ep eps[ffs->eps_count];
-		struct usb_descriptor_header
-			*fs_descs[full ? ffs->fs_descs_count + 1 : 0];
-		struct usb_descriptor_header
-			*hs_descs[high ? ffs->hs_descs_count + 1 : 0];
-		short inums[ffs->interfaces_count];
-		char raw_descs[high ? ffs->raw_descs_length
-				    : ffs->raw_fs_descs_length];
-	} *data;
+	struct ffs_ep *eps = NULL;
+	struct usb_descriptor_header **fs_descs = NULL;
+	struct usb_descriptor_header **hs_descs = NULL;
+	short *inums = NULL;
+	char *raw_descs = NULL;
+
 
 	ENTER();
 
@@ -2225,21 +2232,40 @@ static int ffs_func_bind(struct usb_configuration *c,
 	if (unlikely(!(full | high)))
 		return -ENOTSUPP;
 
-	/* Allocate */
-	data = kmalloc(sizeof *data, GFP_KERNEL);
-	if (unlikely(!data))
+	size_t eps_sz = sizeof(*eps)*ffs->eps_count;
+	eps = kmalloc(eps_sz, GFP_KERNEL);
+	fs_descs = kmalloc(sizeof(*fs_descs)*
+			   (full ? ffs->fs_descs_count + 1 : 0), GFP_KERNEL);
+	hs_descs = kmalloc(sizeof(*hs_descs)*
+			   (high ? ffs->hs_descs_count + 1 : 0), GFP_KERNEL);
+	size_t inums_sz = sizeof(*inums)*ffs->interfaces_count;
+	inums = kmalloc(inums_sz, GFP_KERNEL);
+	size_t raw_descs_sz = sizeof(*raw_descs)*(high ? ffs->raw_descs_length :
+					ffs->raw_fs_descs_length);
+	raw_descs = kmalloc(raw_descs_sz, , GFP_KERNEL);
+
+	if (unlikely(!eps || !fs_descs || !hs_descs || !inums || !raw_descs)) {
+		kfree(eps);
+		kfree(fs_descs);
+		kfree(hs_descs);
+		kfree(inums);
+		kfree(raw_descs);
 		return -ENOMEM;
+	}
 
 	/* Zero */
-	memset(data->eps, 0, sizeof data->eps);
-	memcpy(data->raw_descs, ffs->raw_descs + 16, sizeof data->raw_descs);
-	memset(data->inums, 0xff, sizeof data->inums);
+	memset(eps, 0, eps_sz);
+	memcpy(raw_descs, ffs->raw_descs + 16, raw_descs_sz);
+	memset(inums, 0xff, inums_sz);
 	for (ret = ffs->eps_count; ret; --ret)
-		data->eps[ret].num = -1;
+		eps[ret].num = -1;
 
 	/* Save pointers */
-	func->eps             = data->eps;
-	func->interfaces_nums = data->inums;
+	func->eps             = eps;
+	func->fs_descs        = fs_descs;
+	func->hs_descs        = hs_descs;
+	func->interfaces_nums = inums;
+	func->raw_descs       = raw_descs;
 
 	/*
 	 * Go through all the endpoint descriptors and allocate
@@ -2247,10 +2273,9 @@ static int ffs_func_bind(struct usb_configuration *c,
 	 * numbers without worrying that it may be described later on.
 	 */
 	if (likely(full)) {
-		func->function.fs_descriptors = data->fs_descs;
+		func->function.fs_descriptors = fs_descs;
 		ret = ffs_do_descs(ffs->fs_descs_count,
-				   data->raw_descs,
-				   sizeof data->raw_descs,
+				   raw_descs, raw_descs_sz,
 				   __ffs_func_bind_do_descs, func);
 		if (unlikely(ret < 0))
 			goto error;
@@ -2259,10 +2284,9 @@ static int ffs_func_bind(struct usb_configuration *c,
 	}
 
 	if (likely(high)) {
-		func->function.hs_descriptors = data->hs_descs;
+		func->function.hs_descriptors = hs_descs;
 		ret = ffs_do_descs(ffs->hs_descs_count,
-				   data->raw_descs + ret,
-				   (sizeof data->raw_descs) - ret,
+				   raw_descs + ret, raw_descs_sz - ret,
 				   __ffs_func_bind_do_descs, func);
 	}
 
@@ -2273,7 +2297,7 @@ static int ffs_func_bind(struct usb_configuration *c,
 	 */
 	ret = ffs_do_descs(ffs->fs_descs_count +
 			   (high ? ffs->hs_descs_count : 0),
-			   data->raw_descs, sizeof data->raw_descs,
+			   raw_descs, raw_descs_sz,
 			   __ffs_func_bind_do_nums, func);
 	if (unlikely(ret < 0))
 		goto error;
-- 
1.8.1.2

Re: [PATCH] usb: gadget LLVMLinux: Removing the use of VLAIS from the gadget driver

[Behan Webster]

Replying to my patch email just in case it was missed before.

Thanks,

Behan

On 08/01/13 21:35, behanw@converseincode.com wrote:
> From: Behan Webster <behanw@converseincode.com>
>
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). This patch removes the use of VLAIS in the gadget driver.
>
> Signed-off-by: Mark Charlebois <charlebm@gmail.com>
> Signed-off-by: Behan Webster <behanw@converseincode.com>
> ---
>   drivers/usb/gadget/f_fs.c | 128 +++++++++++++++++++++++++++-------------------
>   1 file changed, 76 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index f394f29..4b872c4 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -30,7 +30,6 @@
>   
>   #define FUNCTIONFS_MAGIC	0xa647361 /* Chosen by a honest dice roll ;) */
>   
> -
>   /* Debugging ****************************************************************/
>   
>   #ifdef VERBOSE_DEBUG
> @@ -214,6 +213,8 @@ struct ffs_data {
>   	/* ids in stringtabs are set in functionfs_bind() */
>   	const void			*raw_strings;
>   	struct usb_gadget_strings	**stringtabs;
> +	struct usb_gadget_strings	*stringtab;
> +	struct usb_string		*strings;
>   
>   	/*
>   	 * File system's super block, write once when file system is
> @@ -263,7 +264,10 @@ struct ffs_function {
>   
>   	struct ffs_ep			*eps;
>   	u8				eps_revmap[16];
> +	struct usb_descriptor_header	**fs_descs;
> +	struct usb_descriptor_header	**hs_descs;
>   	short				*interfaces_nums;
> +	char				*raw_descs;
>   
>   	struct usb_function		function;
>   };
> @@ -1345,6 +1349,8 @@ static void ffs_data_clear(struct ffs_data *ffs)
>   	kfree(ffs->raw_descs);
>   	kfree(ffs->raw_strings);
>   	kfree(ffs->stringtabs);
> +	kfree(ffs->stringtab);
> +	kfree(ffs->strings);
>   }
>   
>   static void ffs_data_reset(struct ffs_data *ffs)
> @@ -1357,6 +1363,8 @@ static void ffs_data_reset(struct ffs_data *ffs)
>   	ffs->raw_descs = NULL;
>   	ffs->raw_strings = NULL;
>   	ffs->stringtabs = NULL;
> +	ffs->stringtab = NULL;
> +	ffs->strings = NULL;
>   
>   	ffs->raw_descs_length = 0;
>   	ffs->raw_fs_descs_length = 0;
> @@ -1528,12 +1536,10 @@ static void ffs_func_free(struct ffs_function *func)
>   	ffs_data_put(func->ffs);
>   
>   	kfree(func->eps);
> -	/*
> -	 * eps and interfaces_nums are allocated in the same chunk so
> -	 * only one free is required.  Descriptors are also allocated
> -	 * in the same chunk.
> -	 */
> -
> +	kfree(func->fs_descs);
> +	kfree(func->hs_descs);
> +	kfree(func->interfaces_nums);
> +	kfree(func->raw_descs);
>   	kfree(func);
>   }
>   
> @@ -1907,33 +1913,35 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
>   		return 0;
>   	}
>   
> -	/* Allocate everything in one chunk so there's less maintenance. */
>   	{
> -		struct {
> -			struct usb_gadget_strings *stringtabs[lang_count + 1];
> -			struct usb_gadget_strings stringtab[lang_count];
> -			struct usb_string strings[lang_count*(needed_count+1)];
> -		} *d;
>   		unsigned i = 0;
> -
> -		d = kmalloc(sizeof *d, GFP_KERNEL);
> -		if (unlikely(!d)) {
> +		usb_gadget_strings **stringtabs = NULL;
> +		usb_gadget_strings *stringtab = NULL;
> +		usb_string *strings = NULL;
> +
> +		stringtabs = kmalloc(sizeof(*stringtabs)*(lang_count + 1),
> +			GFP_KERNEL);
> +		stringtab = kmalloc(sizeof(*stringtab)*(lang_count),
> +			GFP_KERNEL);
> +		strings = kmalloc(sizeof(*strings)
> +			* (lang_count * (needed_count + 1)), GFP_KERNEL);
> +		if (unlikely(!stringtabs || !stringtab || !strings)) {
> +			kfree(stringtabs);
> +			kfree(stringtab);
> +			kfree(strings);
>   			kfree(_data);
>   			return -ENOMEM;
>   		}
> -
> -		stringtabs = d->stringtabs;
> -		t = d->stringtab;
> +		b = stringtabs;
> +		t = stringtab;
>   		i = lang_count;
>   		do {
> -			*stringtabs++ = t++;
> +			*b++ = t++;
>   		} while (--i);
> -		*stringtabs = NULL;
> +		*b = NULL;
>   
> -		stringtabs = d->stringtabs;
> -		t = d->stringtab;
> -		s = d->strings;
> -		strings = s;
> +		t = stringtab;
> +		s = strings;
>   	}
>   
>   	/* For each language */
> @@ -1991,12 +1999,16 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
>   
>   	/* Done! */
>   	ffs->stringtabs = stringtabs;
> +	ffs->stringtab = stringtab;
> +	ffs->strings = strings;
>   	ffs->raw_strings = _data;
>   
>   	return 0;
>   
>   error_free:
>   	kfree(stringtabs);
> +	kfree(stringtab);
> +	kfree(strings);
>   error:
>   	kfree(_data);
>   	return -EINVAL;
> @@ -2207,17 +2219,12 @@ static int ffs_func_bind(struct usb_configuration *c,
>   
>   	int ret;
>   
> -	/* Make it a single chunk, less management later on */
> -	struct {
> -		struct ffs_ep eps[ffs->eps_count];
> -		struct usb_descriptor_header
> -			*fs_descs[full ? ffs->fs_descs_count + 1 : 0];
> -		struct usb_descriptor_header
> -			*hs_descs[high ? ffs->hs_descs_count + 1 : 0];
> -		short inums[ffs->interfaces_count];
> -		char raw_descs[high ? ffs->raw_descs_length
> -				    : ffs->raw_fs_descs_length];
> -	} *data;
> +	struct ffs_ep *eps = NULL;
> +	struct usb_descriptor_header **fs_descs = NULL;
> +	struct usb_descriptor_header **hs_descs = NULL;
> +	short *inums = NULL;
> +	char *raw_descs = NULL;
> +
>   
>   	ENTER();
>   
> @@ -2225,21 +2232,40 @@ static int ffs_func_bind(struct usb_configuration *c,
>   	if (unlikely(!(full | high)))
>   		return -ENOTSUPP;
>   
> -	/* Allocate */
> -	data = kmalloc(sizeof *data, GFP_KERNEL);
> -	if (unlikely(!data))
> +	size_t eps_sz = sizeof(*eps)*ffs->eps_count;
> +	eps = kmalloc(eps_sz, GFP_KERNEL);
> +	fs_descs = kmalloc(sizeof(*fs_descs)*
> +			   (full ? ffs->fs_descs_count + 1 : 0), GFP_KERNEL);
> +	hs_descs = kmalloc(sizeof(*hs_descs)*
> +			   (high ? ffs->hs_descs_count + 1 : 0), GFP_KERNEL);
> +	size_t inums_sz = sizeof(*inums)*ffs->interfaces_count;
> +	inums = kmalloc(inums_sz, GFP_KERNEL);
> +	size_t raw_descs_sz = sizeof(*raw_descs)*(high ? ffs->raw_descs_length :
> +					ffs->raw_fs_descs_length);
> +	raw_descs = kmalloc(raw_descs_sz, , GFP_KERNEL);
> +
> +	if (unlikely(!eps || !fs_descs || !hs_descs || !inums || !raw_descs)) {
> +		kfree(eps);
> +		kfree(fs_descs);
> +		kfree(hs_descs);
> +		kfree(inums);
> +		kfree(raw_descs);
>   		return -ENOMEM;
> +	}
>   
>   	/* Zero */
> -	memset(data->eps, 0, sizeof data->eps);
> -	memcpy(data->raw_descs, ffs->raw_descs + 16, sizeof data->raw_descs);
> -	memset(data->inums, 0xff, sizeof data->inums);
> +	memset(eps, 0, eps_sz);
> +	memcpy(raw_descs, ffs->raw_descs + 16, raw_descs_sz);
> +	memset(inums, 0xff, inums_sz);
>   	for (ret = ffs->eps_count; ret; --ret)
> -		data->eps[ret].num = -1;
> +		eps[ret].num = -1;
>   
>   	/* Save pointers */
> -	func->eps             = data->eps;
> -	func->interfaces_nums = data->inums;
> +	func->eps             = eps;
> +	func->fs_descs        = fs_descs;
> +	func->hs_descs        = hs_descs;
> +	func->interfaces_nums = inums;
> +	func->raw_descs       = raw_descs;
>   
>   	/*
>   	 * Go through all the endpoint descriptors and allocate
> @@ -2247,10 +2273,9 @@ static int ffs_func_bind(struct usb_configuration *c,
>   	 * numbers without worrying that it may be described later on.
>   	 */
>   	if (likely(full)) {
> -		func->function.fs_descriptors = data->fs_descs;
> +		func->function.fs_descriptors = fs_descs;
>   		ret = ffs_do_descs(ffs->fs_descs_count,
> -				   data->raw_descs,
> -				   sizeof data->raw_descs,
> +				   raw_descs, raw_descs_sz,
>   				   __ffs_func_bind_do_descs, func);
>   		if (unlikely(ret < 0))
>   			goto error;
> @@ -2259,10 +2284,9 @@ static int ffs_func_bind(struct usb_configuration *c,
>   	}
>   
>   	if (likely(high)) {
> -		func->function.hs_descriptors = data->hs_descs;
> +		func->function.hs_descriptors = hs_descs;
>   		ret = ffs_do_descs(ffs->hs_descs_count,
> -				   data->raw_descs + ret,
> -				   (sizeof data->raw_descs) - ret,
> +				   raw_descs + ret, raw_descs_sz - ret,
>   				   __ffs_func_bind_do_descs, func);
>   	}
>   
> @@ -2273,7 +2297,7 @@ static int ffs_func_bind(struct usb_configuration *c,
>   	 */
>   	ret = ffs_do_descs(ffs->fs_descs_count +
>   			   (high ? ffs->hs_descs_count : 0),
> -			   data->raw_descs, sizeof data->raw_descs,
> +			   raw_descs, raw_descs_sz,
>   			   __ffs_func_bind_do_nums, func);
>   	if (unlikely(ret < 0))
>   		goto error;


-- 
Behan Webster
behanw@converseincode.com

Re: [PATCH] usb: gadget LLVMLinux: Removing the use of VLAIS from the gadget driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 250 bytes --]

Hi,

On Thu, Sep 05, 2013 at 08:07:11PM -0400, Behan Webster wrote:
> Replying to my patch email just in case it was missed before.

I remember there was a discussion of not dropping variable length array
support, wasn't there ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: gadget LLVMLinux: Removing the use of VLAIS from the gadget driver

[Behan Webster]

On 09/23/13 14:30, Felipe Balbi wrote:
> Hi,
>
> On Thu, Sep 05, 2013 at 08:07:11PM -0400, Behan Webster wrote:
>> Replying to my patch email just in case it was missed before.
> I remember there was a discussion of not dropping variable length array
> support, wasn't there ?
There was mostly an objection to the implementation of that patch. The 
new patch follows the advice given by the linux-usb list members at that 
time.

There is increasing interest from various kernel devs (including 
maintainers and above) to be able to use clang to compile the Linux 
kernel. The removal of the use of VLAIS in the kernel is a step in 
allowing that to happen.

Andrzej Pietrasiewicz attended the LLVM microconference at Plumbers last 
week and accepted the USB gadget patch.

Thanks,

Behan

-- 
Behan Webster
behanw@converseincode.com

Re: [PATCH] usb: gadget LLVMLinux: Removing the use of VLAIS from the gadget driver

[Linus Torvalds]

On Mon, Sep 23, 2013 at 12:30 PM, Felipe Balbi <balbi@ti.com> wrote:
>
> I remember there was a discussion of not dropping variable length array
> support, wasn't there ?

We should definitely drop it. The feature is an abomination. I thought
gcc only allowed them at the end of structs, in the middle of a struct
it's just f*cking insane beyond belief.

That said, for *this* particular case, that USB widget driver already
does a ton of small kmalloc's and then remembers the addresses
independently. People may not care about performance, but it *might*
be a good idea to just do one kmalloc()/kfree(), and then still have
those pointer variables, but just be offsets within that one
allocation.

That's what gcc has to basically do for that thing internally
*anyway*, just hidden behind a horrible construct that should never
have existed.

              Linus

Re: [PATCH] usb: gadget LLVMLinux: Removing the use of VLAIS from the gadget driver

[Behan Webster]

On 09/23/13 15:08, Linus Torvalds wrote:
> On Mon, Sep 23, 2013 at 12:30 PM, Felipe Balbi <balbi@ti.com> wrote:
>> I remember there was a discussion of not dropping variable length array
>> support, wasn't there ?
> We should definitely drop it. The feature is an abomination. I thought
> gcc only allowed them at the end of structs, in the middle of a struct
> it's just f*cking insane beyond belief.
>
> That said, for *this* particular case, that USB widget driver already
> does a ton of small kmalloc's and then remembers the addresses
> independently. People may not care about performance, but it *might*
> be a good idea to just do one kmalloc()/kfree(), and then still have
> those pointer variables, but just be offsets within that one
> allocation.
>
> That's what gcc has to basically do for that thing internally
> *anyway*, just hidden behind a horrible construct that should never
> have existed.
We can certainly do that instead.

I believe I already have a version of the patch which does just that 
(without using macros). I will post it for comment.

Thanks,

Behan

-- 
Behan Webster
behanw@converseincode.com

Re: [PATCH] USB: Removing the use of VLAIS from the gadget driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]

Hi,

On Thu, Aug 01, 2013 at 09:35:44PM -0400, behanw@converseincode.com wrote:
> From: Behan Webster <behanw@converseincode.com>
> 
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). This patch removes the use of VLAIS in the gadget driver.
> 
> Signed-off-by: Mark Charlebois <charlebm@gmail.com>
> Signed-off-by: Behan Webster <behanw@converseincode.com>
> ---
>  drivers/usb/gadget/f_fs.c | 128 +++++++++++++++++++++++++++-------------------
>  1 file changed, 76 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index f394f29..4b872c4 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -30,7 +30,6 @@
>  
>  #define FUNCTIONFS_MAGIC	0xa647361 /* Chosen by a honest dice roll ;) */
>  
> -
>  /* Debugging ****************************************************************/
>  
>  #ifdef VERBOSE_DEBUG
> @@ -214,6 +213,8 @@ struct ffs_data {
>  	/* ids in stringtabs are set in functionfs_bind() */
>  	const void			*raw_strings;
>  	struct usb_gadget_strings	**stringtabs;
> +	struct usb_gadget_strings	*stringtab;
> +	struct usb_string		*strings;
>  
>  	/*
>  	 * File system's super block, write once when file system is
> @@ -263,7 +264,10 @@ struct ffs_function {
>  
>  	struct ffs_ep			*eps;
>  	u8				eps_revmap[16];
> +	struct usb_descriptor_header	**fs_descs;
> +	struct usb_descriptor_header	**hs_descs;
>  	short				*interfaces_nums;
> +	char				*raw_descs;
>  
>  	struct usb_function		function;
>  };
> @@ -1345,6 +1349,8 @@ static void ffs_data_clear(struct ffs_data *ffs)
>  	kfree(ffs->raw_descs);
>  	kfree(ffs->raw_strings);
>  	kfree(ffs->stringtabs);
> +	kfree(ffs->stringtab);
> +	kfree(ffs->strings);
>  }
>  
>  static void ffs_data_reset(struct ffs_data *ffs)
> @@ -1357,6 +1363,8 @@ static void ffs_data_reset(struct ffs_data *ffs)
>  	ffs->raw_descs = NULL;
>  	ffs->raw_strings = NULL;
>  	ffs->stringtabs = NULL;
> +	ffs->stringtab = NULL;
> +	ffs->strings = NULL;
>  
>  	ffs->raw_descs_length = 0;
>  	ffs->raw_fs_descs_length = 0;
> @@ -1528,12 +1536,10 @@ static void ffs_func_free(struct ffs_function *func)
>  	ffs_data_put(func->ffs);
>  
>  	kfree(func->eps);
> -	/*
> -	 * eps and interfaces_nums are allocated in the same chunk so
> -	 * only one free is required.  Descriptors are also allocated
> -	 * in the same chunk.
> -	 */
> -
> +	kfree(func->fs_descs);
> +	kfree(func->hs_descs);
> +	kfree(func->interfaces_nums);
> +	kfree(func->raw_descs);
>  	kfree(func);
>  }
>  
> @@ -1907,33 +1913,35 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
>  		return 0;
>  	}
>  
> -	/* Allocate everything in one chunk so there's less maintenance. */
>  	{
> -		struct {
> -			struct usb_gadget_strings *stringtabs[lang_count + 1];
> -			struct usb_gadget_strings stringtab[lang_count];
> -			struct usb_string strings[lang_count*(needed_count+1)];
> -		} *d;
>  		unsigned i = 0;
> -
> -		d = kmalloc(sizeof *d, GFP_KERNEL);
> -		if (unlikely(!d)) {
> +		usb_gadget_strings **stringtabs = NULL;
> +		usb_gadget_strings *stringtab = NULL;
> +		usb_string *strings = NULL;

did you compile this patch ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] USB: Removing the use of VLAIS from the gadget driver

[Behan Webster]

On 09/23/13 16:24, Felipe Balbi wrote:
> Hi,
>
> On Thu, Aug 01, 2013 at 09:35:44PM -0400, behanw@converseincode.com wrote:
>> From: Behan Webster <behanw@converseincode.com>
>>
>> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
>> precludes the use of compilers which don't implement VLAIS (for instance the
>> Clang compiler). This patch removes the use of VLAIS in the gadget driver.
>>
>> Signed-off-by: Mark Charlebois <charlebm@gmail.com>
>> Signed-off-by: Behan Webster <behanw@converseincode.com>
>> ---
>>   drivers/usb/gadget/f_fs.c | 128 +++++++++++++++++++++++++++-------------------
>>   1 file changed, 76 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
>> index f394f29..4b872c4 100644
>> --- a/drivers/usb/gadget/f_fs.c
>> +++ b/drivers/usb/gadget/f_fs.c
>> @@ -30,7 +30,6 @@
>>   
>>   #define FUNCTIONFS_MAGIC	0xa647361 /* Chosen by a honest dice roll ;) */
>>   
>> -
>>   /* Debugging ****************************************************************/
>>   
>>   #ifdef VERBOSE_DEBUG
>> @@ -214,6 +213,8 @@ struct ffs_data {
>>   	/* ids in stringtabs are set in functionfs_bind() */
>>   	const void			*raw_strings;
>>   	struct usb_gadget_strings	**stringtabs;
>> +	struct usb_gadget_strings	*stringtab;
>> +	struct usb_string		*strings;
>>   
>>   	/*
>>   	 * File system's super block, write once when file system is
>> @@ -263,7 +264,10 @@ struct ffs_function {
>>   
>>   	struct ffs_ep			*eps;
>>   	u8				eps_revmap[16];
>> +	struct usb_descriptor_header	**fs_descs;
>> +	struct usb_descriptor_header	**hs_descs;
>>   	short				*interfaces_nums;
>> +	char				*raw_descs;
>>   
>>   	struct usb_function		function;
>>   };
>> @@ -1345,6 +1349,8 @@ static void ffs_data_clear(struct ffs_data *ffs)
>>   	kfree(ffs->raw_descs);
>>   	kfree(ffs->raw_strings);
>>   	kfree(ffs->stringtabs);
>> +	kfree(ffs->stringtab);
>> +	kfree(ffs->strings);
>>   }
>>   
>>   static void ffs_data_reset(struct ffs_data *ffs)
>> @@ -1357,6 +1363,8 @@ static void ffs_data_reset(struct ffs_data *ffs)
>>   	ffs->raw_descs = NULL;
>>   	ffs->raw_strings = NULL;
>>   	ffs->stringtabs = NULL;
>> +	ffs->stringtab = NULL;
>> +	ffs->strings = NULL;
>>   
>>   	ffs->raw_descs_length = 0;
>>   	ffs->raw_fs_descs_length = 0;
>> @@ -1528,12 +1536,10 @@ static void ffs_func_free(struct ffs_function *func)
>>   	ffs_data_put(func->ffs);
>>   
>>   	kfree(func->eps);
>> -	/*
>> -	 * eps and interfaces_nums are allocated in the same chunk so
>> -	 * only one free is required.  Descriptors are also allocated
>> -	 * in the same chunk.
>> -	 */
>> -
>> +	kfree(func->fs_descs);
>> +	kfree(func->hs_descs);
>> +	kfree(func->interfaces_nums);
>> +	kfree(func->raw_descs);
>>   	kfree(func);
>>   }
>>   
>> @@ -1907,33 +1913,35 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
>>   		return 0;
>>   	}
>>   
>> -	/* Allocate everything in one chunk so there's less maintenance. */
>>   	{
>> -		struct {
>> -			struct usb_gadget_strings *stringtabs[lang_count + 1];
>> -			struct usb_gadget_strings stringtab[lang_count];
>> -			struct usb_string strings[lang_count*(needed_count+1)];
>> -		} *d;
>>   		unsigned i = 0;
>> -
>> -		d = kmalloc(sizeof *d, GFP_KERNEL);
>> -		if (unlikely(!d)) {
>> +		usb_gadget_strings **stringtabs = NULL;
>> +		usb_gadget_strings *stringtab = NULL;
>> +		usb_string *strings = NULL;
> did you compile this patch ?
>
Appologies. The patch as posted has a bug which was fixed after sending 
it to the list.

Andrzej Pietrasiewicz <andrzej.p@samsung.com> has the fixed one. Will 
send the fixed one to the list too.

Behan

-- 
Behan Webster
behanw@converseincode.com