On Thu, Jul 02 2015, Krzysztof Opasiak wrote:
> This patch replace dynamicly allocated luns array with static one.
> This simplifies the code of mass storage function and modules.
> It also fix issue with reporting wrong number of LUNs in GET_MAX_LUN
> request.
>
> Also change the nluns to max_luns which is better name for value
> stored in that place as we no longer need to store size of luns
> array.
>
> Reported-by: David Fisher <david.fisher1@xxxxxxxxxxxx>
> Signed-off-by: Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx>
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 125 ++++++++++----------------
> drivers/usb/gadget/function/f_mass_storage.h | 4 -
> drivers/usb/gadget/legacy/acm_ms.c | 6 --
> drivers/usb/gadget/legacy/mass_storage.c | 6 --
> drivers/usb/gadget/legacy/multi.c | 6 --
> 5 files changed, 48 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index ad0f69b..befe251 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -279,9 +279,9 @@ struct fsg_common {
> int cmnd_size;
> u8 cmnd[MAX_COMMAND_SIZE];
>
> - unsigned int nluns;
> + int max_lun;
To be honest, I don’t like this change. It is more idiomatic to store
number of elements in an array rather than index of the last element.
Sooner or later someone will do for (i = 0; i < common->max_lun; ++i)
out of habit.
> unsigned int lun;
> - struct fsg_lun **luns;
> + struct fsg_lun *luns[FSG_MAX_LUNS];
> struct fsg_lun *curlun;
>
> unsigned int bulk_out_maxpacket;
> @@ -2760,48 +2767,16 @@ static void _fsg_common_remove_luns(struct fsg_common *common, int n)
> fsg_common_remove_lun(common->luns[i], common->sysfs);
> common->luns[i] = NULL;
> }
> +
> + _fsg_common_reduce_max_lun(common);
> }
>
> void fsg_common_remove_luns(struct fsg_common *common)
> {
> - _fsg_common_remove_luns(common, common->nluns);
> + _fsg_common_remove_luns(common, common->max_lun);
Shouldn’t this be:
+ _fsg_common_remove_luns(common, common->max_lun + 1);
> }
> EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
>
> @@ -2954,7 +2931,7 @@ error_lun:
> if (common->sysfs)
> device_unregister(&lun->dev);
> fsg_lun_close(lun);
> - common->luns[id] = NULL;
You need to keep this line.
> + _fsg_common_reduce_max_lun(common);
> error_sysfs:
> kfree(lun);
> return rc;
> @@ -3305,11 +3282,9 @@ static struct config_group *fsg_lun_make(struct config_group *group,
> return ERR_PTR(ret);
>
> fsg_opts = to_fsg_opts(&group->cg_item);
> - if (num >= FSG_MAX_LUNS)
> - return ERR_PTR(-ERANGE);
Going through all the memory allocation and mutex locking just to find
out that num is to big seems wasteful. I’d keep this check here.
>
> mutex_lock(&fsg_opts->lock);
> - if (fsg_opts->refcnt || fsg_opts->common->luns[num]) {
> + if (fsg_opts->refcnt) {
> ret = -EBUSY;
> goto out;
> }
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
