From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout4.w1.samsung.com ([210.118.77.14]:29739 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753478AbbGTKe5 (ORCPT ); Mon, 20 Jul 2015 06:34:57 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 8BIT Message-id: <55ACCECC.5000609@samsung.com> Date: Mon, 20 Jul 2015 12:34:52 +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 v3 4/5] usb: gadget: mass_storage: Use static array for luns References: <1436281065-32407-1-git-send-email-k.opasiak@samsung.com> <1436281065-32407-5-git-send-email-k.opasiak@samsung.com> In-reply-to: Sender: stable-owner@vger.kernel.org List-ID: On 07/08/2015 04:00 PM, Michal Nazarewicz wrote: > On Tue, Jul 07 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. According to MS spec we should return the max index of valid >> LUN, not the number of luns - 1. > > This is no longer true, is it? Why my fix this bug has been solved, no? > As such, this should not go to stable. Or am I missing something? First of all please excuse me my late response but I were out of office. Unfortunately it's still true. Your fix solved this bug partially. Now we report nluns - 1 instead of FSG_LUN_MAX but in case of not contiguous luns it is not enough. Let's consider mass storage function with luns 0 1 3 5. nluns == 4 so in GET_MAX_LUN we will return 3 (nluns - 1). Some hosts (in particular Windows) iterate over all luns up to value returned from this request. This means that host will ask about LUNs 0 1 2 3. LUN 2 is invalid and will be skipped but he will never ask about luns 4 and 5 what makes LUN 5 unavailable. Standard says that GET_MAX_LUNS should return index of last valid LUN which in this case is 5 not 3 and this is what this patch does. > >> Reported-by: David Fisher >> Signed-off-by: Krzysztof Opasiak > > Acked-by: Michal Nazarewicz > >> CC: stable@vger.kernel.org # 3.13+ > > >> @@ -490,6 +489,16 @@ static void bulk_out_complete(struct usb_ep *ep, struct usb_request *req) >> spin_unlock(&common->lock); >> } >> >> +static int _fsg_common_get_max_lun(struct fsg_common *common) > > Perhaps even ‘unsigned’. As 0 is a valid LUN index this function returns -1 if there is no LUNs at all. > >> +{ >> + int i = ARRAY_SIZE(common->luns) - 1; > > Ditto. This applies in other places of the patch so I won’t keep > repeating it. > >> + >> + while (i >= 0 && !common->luns[i]) >> + --i; >> + >> + return i; >> +} >> + >> static int fsg_setup(struct usb_function *f, >> const struct usb_ctrlrequest *ctrl) >> { > >> @@ -2552,12 +2562,11 @@ static int fsg_main_thread(void *common_) >> >> if (!common->ops || !common->ops->thread_exits >> || common->ops->thread_exits(common) < 0) { >> - struct fsg_lun **curlun_it = common->luns; >> - unsigned i = common->nluns; >> + int i; >> >> down_write(&common->filesem); >> - for (; i--; ++curlun_it) { >> - struct fsg_lun *curlun = *curlun_it; >> + for (i = 0; i < ARRAY_SIZE(common->luns); --i) { > > ++i > >> + struct fsg_lun *curlun = common->luns[i]; >> if (!curlun || !fsg_lun_is_open(curlun)) >> continue; >> > -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics