From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout4.w1.samsung.com ([210.118.77.14]:41526 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754115AbbGBTkY (ORCPT ); Thu, 2 Jul 2015 15:40:24 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 8BIT Message-id: <559593A3.4080607@samsung.com> Date: Thu, 02 Jul 2015 21:40:19 +0200 From: Krzysztof Opasiak To: Michal Nazarewicz , balbi@ti.com Cc: stable@vger.kernel.org, david.fisher1@synopsys.com, gregkh@linuxfoundation.org, andrzej.p@samsung.com, m.szyprowski@samsung.com, linux-usb@vger.kernel.org Subject: Re: [PATCH v2 4/5] usb: gadget: mass_storage: Use static array for luns References: <1435863398-24646-1-git-send-email-k.opasiak@samsung.com> <1435863398-24646-5-git-send-email-k.opasiak@samsung.com> In-reply-to: Sender: stable-owner@vger.kernel.org List-ID: On 07/02/2015 09:29 PM, Michal Nazarewicz wrote: > 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 >> Signed-off-by: Krzysztof Opasiak >> --- >> 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. > The reason why I did this was allowing not contiguous LUNs ids. As you suggested in earlier discussion [1] we should allow user space shoot themselves in the foot and tis is one of the consequence. What we should really return in GET_MAX_LUN is last valid LUN id in our array, not number of LUNs. Windows is paying attention to this value and for examnple for luns: 0 5 6 it makes a different if you reply with nluns -1 or max_lun. In first option windows will see only one LUN while in second all three of them. As we increment max_lun only when adding a LUN currently it is not possible to get out of bounds. >> 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); agree, should be fixed. > >> } >> 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. This one is also true. Sorry, my mistake... > >> + _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. Ok I will leave it as it was before . > >> >> mutex_lock(&fsg_opts->lock); >> - if (fsg_opts->refcnt || fsg_opts->common->luns[num]) { >> + if (fsg_opts->refcnt) { >> ret = -EBUSY; >> goto out; >> } > 1 - http://marc.info/?l=linux-usb&m=143498348620786&w=2 -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics