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 <andrzej.p@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
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: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--Attachment:
signature.asc
Description: PGP signature
