Re: [PATCH v3 4/5] usb: gadget: mass_storage: Use static array for luns

[Krzysztof Opasiak <k.opasiak@samsung.com>]

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 <david.fisher1@synopsys.com>
>> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>
>> 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