On Wed, Oct 23 2013, Andrzej Pietrasiewicz wrote: > From: Mark Charlebois <charl...@gmail.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 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 <charl...@gmail.com> > Signed-off-by: Behan Webster <beh...@converseincode.com> > > [elimination of miexed declaration and code, checkpatch cleanup] > Signed-off-by: Andrzej Pietrasiewicz <andrze...@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com> > --- > 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
signature.asc
Description: PGP signature