On Fri, Jul 19 2013, Andrzej Pietrasiewicz wrote: > This is needed to prepare for configfs integration. > > So far the luns have been allocated during gadget's initialization, based > on the nluns module parameter's value; the exact number is known when the > gadget is initialized and that number of luns is allocated in one go; they > all will be used. > > When configfs is in place, the luns will be created one-by-one by the user. > Once the user is satisfied with the number of luns, they activate the > gadget. The number of luns must be <= FSG_MAX_LUN (currently 8), but other > than that it is not known up front and the user need not use contiguous > numbering (apart from the default lun #0). On the other hand, the function > code uses lun numbers to identify them and the number needs to be used > as an index into an array. > > Given the above, an array needs to be allocated, but it might happen that > 7 out of its 8 elements will not be used. On my machine > sizeof(struct fsg_lun) == 462, so > 3k of memory is allocated but not used > in the worst case. > > By adding another level of indirection (allocating an array of pointers > to struct fsg_lun and then allocating individual luns instead of an array > of struct fsg_luns) at most 7 pointers are wasted, which is much less. > > This patch also changes some for/while loops to cope with the fact > that in the luns array some entries are potentially empty. > > Signed-off-by: Andrzej Pietrasiewicz <andrze...@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
Acked-by: Michal Nazarewicz <min...@mina86.com>
Still some minor comments inline:
> ---
> drivers/usb/gadget/f_mass_storage.c | 56 +++++++++++++++++++++++-----------
> 1 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c
> b/drivers/usb/gadget/f_mass_storage.c
> index c36e208..d01d1dd 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2439,7 +2443,7 @@ static void handle_exception(struct fsg_common *common)
> * CONFIG_CHANGE cases.
> */
> /* for (i = 0; i < common->nluns; ++i) */
> - /* common->luns[i].unit_attention_data = */
> + /* common->luns[i]->unit_attention_data = */
> /* SS_RESET_OCCURRED; */
> break;
It's a comment so it's not a big deal, but nonetheless, this should read:
- /* common->luns[i].unit_attention_data = */
- /* SS_RESET_OCCURRED; */
+ /* if (common->luns[i]) */
+ /* common->luns[i]->unit_attention_data = */
+ /* SS_RESET_OCCURRED; */
> @@ -2659,16 +2664,25 @@ struct fsg_common *fsg_common_init(struct fsg_common
> *common,
> * Create the LUNs, open their backing files, and register the
> * LUN devices in sysfs.
> */
> - curlun = kcalloc(nluns, sizeof(*curlun), GFP_KERNEL);
> - if (unlikely(!curlun)) {
> + curlun_it = kcalloc(nluns, sizeof(*curlun_it), GFP_KERNEL);
> + if (unlikely(!curlun_it)) {
> rc = -ENOMEM;
> goto error_release;
> }
> - common->luns = curlun;
> + common->luns = curlun_it;
>
> init_rwsem(&common->filesem);
>
> - for (i = 0, lcfg = cfg->luns; i < nluns; ++i, ++curlun, ++lcfg) {
> + for (i = 0, lcfg = cfg->luns; i < nluns; ++i, ++curlun_it, ++lcfg) {
> + struct fsg_lun *curlun;
> +
> + *curlun_it = kzalloc(sizeof(**curlun_it), GFP_KERNEL);
> + curlun = *curlun_it;
I don't like the order of those statements. How about instead of the
two, first:
+ curlun = kzalloc(sizeof(*curlun), GFP_KERNEL);
here
> + if (!curlun) {
> + rc = -ENOMEM;
> + common->nluns = i;
> + goto error_release;
> + }
and assignment to memory here:
+ *curlun_it = curlun;
> curlun->cdrom = !!lcfg->cdrom;
> curlun->ro = lcfg->cdrom || lcfg->ro;
> curlun->initially_ro = curlun->ro;
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: m...@google.com>--------------ooO--(_)--Ooo--
signature.asc
Description: PGP signature
