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

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

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