On Wed, Oct 23 2013, Andrzej Pietrasiewicz wrote:
> From: Mark Charlebois <charlebm@xxxxxxxxx>
>
> 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 alternate patch calculates offsets into the kmalloc-ed
> memory buffer using macros. The previous patch required multiple kmalloc and
> kfree calls. This version uses "group" vs "struct" since it really is not a
> struct and is essentially a group of VLA in a common allocated block. This
> version also fixes the issues pointed out by Andrzej Pietrasiewicz.
>
> Signed-off-by: Mark Charlebois <charlebm@xxxxxxxxx>
> Signed-off-by: Behan Webster <behanw@xxxxxxxxxxxxxxxxxx>
>
> [elimination of miexed declaration and code, checkpatch cleanup]
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> ---
> drivers/usb/gadget/f_fs.c | 110 +++++++++++++++++++++++++++++----------------
> 1 files changed, 71 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 0658908..9ccda73 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -30,6 +30,21 @@
>
> #define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest dice roll ;) */
I think something more explicit with fewer automatically declared
variables cloud be more readable:
> +/* Variable Length Array Macros **********************************************/
> +#define vla_group(groupname) size_t groupname##__##next = 0
> +#define vla_group_size(groupname) groupname##__##next
#define vla_group(groupname) size_t groupname##__next = 0
#define vla_group_size(groupname) groupname##__next
> +
> +#define vla_item(groupname, type, name, n) \
> + size_t groupname##_##name##__##offset = \
> + (groupname##__##next + __alignof__(type) - 1) & \
> + ~(__alignof__(type) - 1); \
> + size_t groupname##_##name##__##sz = (n) * sizeof(type); \
> + type *groupname##_##name = ({ \
> + groupname##__##next = groupname##_##name##__##offset + \
> + groupname##_##name##__##sz; NULL; })
#define vla_item(groupname, type, name, n) \
size_t groupname##_##name##__offset = ({ \
size_t align_mask = __alignof__(type) - 1; \
size_t offset = (groupname##__next + align_mask) & align_mask; \
size_t size = (n) * sizeof(type); \
groupname##__next = offset + size; \
offset;
})
> +
> +#define vla_ptr(ptr, groupname, name) (groupname##_##name = \
> + (__typeof__(groupname##_##name))&ptr[groupname##_##name##__##offset])
>
#define vla_ptr(ptr, groupname, name)
((void *) ((char *)ptr + groupname##_##name##__offset))
> /* Debugging ****************************************************************/
>
> @@ -1901,30 +1916,38 @@ static int __ffs_data_got_strings(struct ffs_data *ffs,
>
> /* 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;
> + vla_group(d);
> + vla_item(d, struct usb_gadget_strings *, stringtabs,
> + lang_count + 1);
> + vla_item(d, struct usb_gadget_strings, stringtab, lang_count);
> + vla_item(d, struct usb_string, strings,
> + lang_count*(needed_count+1));
>
> - d = kmalloc(sizeof *d, GFP_KERNEL);
> - if (unlikely(!d)) {
> + char *vlabuf = kmalloc(vla_group_size(d), GFP_KERNEL);
> +
> + if (unlikely(!vlabuf)) {
> kfree(_data);
> return -ENOMEM;
> }
>
> - stringtabs = d->stringtabs;
> - t = d->stringtab;
> + /* Initialize the VLA pointers */
> + vla_ptr(vlabuf, d, stringtabs);
> + vla_ptr(vlabuf, d, stringtab);
> + vla_ptr(vlabuf, d, strings);
> +
> + stringtabs = d_stringtabs;
> + t = d_stringtab;
stringtabs = vla_ptr(vlabuf, d, stringtabs);
t = vla_ptr(vlabuf, d, stringtab);
> i = lang_count;
> do {
> *stringtabs++ = t++;
> } while (--i);
> *stringtabs = NULL;
>
> - stringtabs = d->stringtabs;
> - t = d->stringtab;
> - s = d->strings;
> + /* stringtabs = vlabuf = d_stringtabs for later kfree */
> + stringtabs = d_stringtabs;
> + t = d_stringtab;
> + s = d_strings;
stringtabs = vla_ptr(vlabuf, d, stringtabs);
t = vla_ptr(vlabuf, d, stringtab);
s = vla_ptr(vlabuf, d, strings);
> strings = s;
> }
And similarly below:
> @@ -2200,16 +2223,16 @@ 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;
> + vla_group(d);
> + vla_item(d, struct ffs_ep, eps, ffs->eps_count);
> + vla_item(d, struct usb_descriptor_header *, fs_descs,
> + full ? ffs->fs_descs_count + 1 : 0);
> + vla_item(d, struct usb_descriptor_header *, hs_descs,
> + high ? ffs->hs_descs_count + 1 : 0);
> + vla_item(d, short, inums, ffs->interfaces_count);
> + vla_item(d, char, raw_descs,
> + high ? ffs->raw_descs_length : ffs->raw_fs_descs_length);
> + char *vlabuf;
>
> ENTER();
>
> @@ -2217,21 +2240,30 @@ 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))
> + /* Allocate a single chunk, less management later on */
> + vlabuf = kmalloc(vla_group_size(d), GFP_KERNEL);
> + if (unlikely(!vlabuf))
> return -ENOMEM;
>
> + /* Initialize each struct member pointer in the allocated memory */
> + vla_ptr(vlabuf, d, eps);
> + vla_ptr(vlabuf, d, fs_descs);
> + vla_ptr(vlabuf, d, hs_descs);
> + vla_ptr(vlabuf, d, inums);
> + vla_ptr(vlabuf, d, raw_descs);
> +
> /* 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(d_eps, 0, d_eps__sz);
> + memcpy(d_raw_descs, ffs->raw_descs + 16, d_raw_descs__sz);
> + memset(d_inums, 0xff, d_inums__sz);
> for (ret = ffs->eps_count; ret; --ret)
> - data->eps[ret].num = -1;
> + d_eps[ret].num = -1;
>
> - /* Save pointers */
> - func->eps = data->eps;
> - func->interfaces_nums = data->inums;
> + /* Save pointers
> + * d_eps == vlabuf, func->eps used to kfree vlabuf later
> + */
> + func->eps = d_eps;
> + func->interfaces_nums = d_inums;
>
> /*
> * Go through all the endpoint descriptors and allocate
> @@ -2239,10 +2271,10 @@ 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 = d_fs_descs;
> ret = ffs_do_descs(ffs->fs_descs_count,
> - data->raw_descs,
> - sizeof data->raw_descs,
> + d_raw_descs,
> + d_raw_descs__sz,
> __ffs_func_bind_do_descs, func);
> if (unlikely(ret < 0))
> goto error;
> @@ -2251,10 +2283,10 @@ static int ffs_func_bind(struct usb_configuration *c,
> }
>
> if (likely(high)) {
> - func->function.hs_descriptors = data->hs_descs;
> + func->function.hs_descriptors = d_hs_descs;
> ret = ffs_do_descs(ffs->hs_descs_count,
> - data->raw_descs + ret,
> - (sizeof data->raw_descs) - ret,
> + d_raw_descs + ret,
> + d_raw_descs__sz - ret,
> __ffs_func_bind_do_descs, func);
> }
>
> @@ -2265,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,
> + d_raw_descs, d_raw_descs__sz,
> __ffs_func_bind_do_nums, func);
> if (unlikely(ret < 0))
> goto error;
> --
> 1.7.0.4
Attachment:
signature.asc
Description: PGP signature
