Re: [PATCH 5/5] usb: gadget: mass_storage: Send correct number of LUNs to host

[Michal Nazarewicz <mina86@mina86.com>]

On Mon, Jun 22 2015, Krzysztof Opasiak wrote:
> GET_MAX_LUN request should return number of realy created LUNs
> not the size of preallocated array.
>
> This patch changes fsg_common_set_nluns() to fsg_common_set_max_luns()
> which now only allocates an empty array for luns and set nluns
> to 0. While LUNS are create we simply count them and store result
> in nluns which is send later to host.
>
> Reported-by: David Fisher <david.fisher1@synopsys.com>
> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>

At this point I would just change common->luns to be an array rather
than a pointer.  This would remove need for max_luns all together.

> ---
>  drivers/usb/gadget/function/f_mass_storage.c |   69 ++++++++++++++------------
>  drivers/usb/gadget/function/f_mass_storage.h |    2 +-
>  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(+), 41 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 4257238..7e37070 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -280,6 +280,7 @@ struct fsg_common {
>  	u8			cmnd[MAX_COMMAND_SIZE];
>  
>  	unsigned int		nluns;
> +	unsigned int            max_luns;
>  	unsigned int		lun;
>  	struct fsg_lun		**luns;
>  	struct fsg_lun		*curlun;
> @@ -2131,7 +2132,7 @@ static int received_cbw(struct fsg_dev *fsg, struct fsg_buffhd *bh)
>  	}
>  
>  	/* Is the CBW meaningful? */
> -	if (cbw->Lun >= FSG_MAX_LUNS || cbw->Flags & ~US_BULK_FLAG_IN ||
> +	if (cbw->Lun >= common->nluns || cbw->Flags & ~US_BULK_FLAG_IN ||
>  			cbw->Length <= 0 || cbw->Length > MAX_COMMAND_SIZE) {
>  		DBG(fsg, "non-meaningful CBW: lun = %u, flags = 0x%x, "
>  				"cmdlen %u\n",
> @@ -2411,8 +2412,7 @@ static void handle_exception(struct fsg_common *common)
>  	else {
>  		for (i = 0; i < common->nluns; ++i) {
>  			curlun = common->luns[i];
> -			if (!curlun)
> -				continue;
> +			WARN_ON(!curlun);
>  			curlun->prevent_medium_removal = 0;
>  			curlun->sense_data = SS_NO_SENSE;
>  			curlun->unit_attention_data = SS_NO_SENSE;
> @@ -2558,7 +2558,9 @@ static int fsg_main_thread(void *common_)
>  		down_write(&common->filesem);
>  		for (; i--; ++curlun_it) {
>  			struct fsg_lun *curlun = *curlun_it;
> -			if (!curlun || !fsg_lun_is_open(curlun))
> +
> +			WARN_ON(!curlun);
> +			if (!fsg_lun_is_open(curlun))
>  				continue;
>  
>  			fsg_lun_close(curlun);
> @@ -2759,6 +2761,8 @@ static void _fsg_common_remove_luns(struct fsg_common *common, int n)
>  		if (common->luns[i]) {
>  			fsg_common_remove_lun(common->luns[i], common->sysfs);
>  			common->luns[i] = NULL;
> +			WARN_ON(common->nluns == 0);
> +			common->nluns--;
>  		}
>  }
>  
> @@ -2773,20 +2777,22 @@ void fsg_common_free_luns(struct fsg_common *common)
>  	fsg_common_remove_luns(common);
>  	kfree(common->luns);
>  	common->luns = NULL;
> +	common->max_luns = 0;
> +	common->nluns = 0;
>  }
>  EXPORT_SYMBOL_GPL(fsg_common_free_luns);
>  
> -int fsg_common_set_nluns(struct fsg_common *common, int nluns)
> +int fsg_common_set_max_luns(struct fsg_common *common, int max_luns)
>  {
>  	struct fsg_lun **curlun;
>  
>  	/* Find out how many LUNs there should be */
> -	if (nluns < 1 || nluns > FSG_MAX_LUNS) {
> -		pr_err("invalid number of LUNs: %u\n", nluns);
> +	if (max_luns < 1 || max_luns > FSG_MAX_LUNS) {
> +		pr_err("invalid number of LUNs: %u\n", max_luns);
>  		return -EINVAL;
>  	}
>  
> -	curlun = kcalloc(nluns, sizeof(*curlun), GFP_KERNEL);
> +	curlun = kcalloc(max_luns, sizeof(*curlun), GFP_KERNEL);
>  	if (unlikely(!curlun))
>  		return -ENOMEM;
>  
> @@ -2794,13 +2800,15 @@ int fsg_common_set_nluns(struct fsg_common *common, int nluns)
>  		fsg_common_free_luns(common);
>  
>  	common->luns = curlun;
> -	common->nluns = nluns;
> +	common->max_luns = max_luns;
> +	common->nluns = 0;
>  
> -	pr_info("Number of LUNs=%d\n", common->nluns);
> +	pr_info("Number of LUNs=%d, max number of LUNs=%d\n",
> +		common->nluns, common->max_luns);
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(fsg_common_set_nluns);
> +EXPORT_SYMBOL_GPL(fsg_common_set_max_luns);
>  
>  void fsg_common_set_ops(struct fsg_common *common,
>  			const struct fsg_operations *ops)
> @@ -2882,7 +2890,7 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg,
>  	char *pathbuf, *p;
>  	int rc = -ENOMEM;
>  
> -	if (!common->nluns || !common->luns)
> +	if (!common->max_luns || !common->luns)
>  		return -ENODEV;
>  
>  	if (common->luns[id])
> @@ -2924,6 +2932,7 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg,
>  	}
>  
>  	common->luns[id] = lun;
> +	common->nluns++;
>  
>  	if (cfg->filename) {
>  		rc = fsg_lun_open(lun, cfg->filename);
> @@ -2955,6 +2964,7 @@ error_lun:
>  		device_unregister(&lun->dev);
>  	fsg_lun_close(lun);
>  	common->luns[id] = NULL;
> +	common->nluns--;
>  error_sysfs:
>  	kfree(lun);
>  	return rc;
> @@ -2966,7 +2976,10 @@ int fsg_common_create_luns(struct fsg_common *common, struct fsg_config *cfg)
>  	char buf[8]; /* enough for 100000000 different numbers, decimal */
>  	int i, rc;
>  
> -	for (i = 0; i < common->nluns; ++i) {
> +	if (cfg->nluns > common->max_luns)
> +		return -EINVAL;
> +
> +	for (i = 0; i < cfg->nluns; ++i) {
>  		snprintf(buf, sizeof(buf), "lun%d", i);
>  		rc = fsg_common_create_lun(common, &cfg->luns[i], i, buf, NULL);
>  		if (rc)
> @@ -3031,7 +3044,7 @@ static void fsg_common_release(struct kref *ref)
>  
>  	if (likely(common->luns)) {
>  		struct fsg_lun **lun_it = common->luns;
> -		unsigned i = common->nluns;
> +		unsigned i = common->max_luns;
>  
>  		/* In error recovery common->nluns may be zero. */
>  		for (; i; --i, ++lun_it) {
> @@ -3058,6 +3071,7 @@ static void fsg_common_release(struct kref *ref)
>  static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
>  {
>  	struct fsg_dev		*fsg = fsg_from_func(f);
> +	struct fsg_common	*common = fsg->common;
>  	struct usb_gadget	*gadget = c->cdev->gadget;
>  	int			i;
>  	struct usb_ep		*ep;
> @@ -3070,25 +3084,16 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
>  	 * or LUNs ids are not contiguous.
>  	 */
>  	if (likely(common->luns)) {
> -		bool found_null = false;
> -
> -		for (i = 0; i < common->nluns; ++i) {
> -			if (!common->luns[i]) {
> -				found_null = true;
> -				continue;
> -			}
> -
> -			if (!found_null) {
> -				continue;
> -			} else {
> -				pr_err("LUN ids should be contiguous.\n");
> -				return -EINVAL;
> -			}
> -		}
> +		for (i = 0; i < common->max_luns; ++i)
> +			if (!common->luns[i])
> +				break;
>  
> -		if (i == 0 || !common->luns[i]) {
> +		if (i == 0) {
>  			pr_err("There should be at least one LUN.\n");
>  			return -EINVAL;
> +		} else if (i < common->nluns) {
> +			pr_err("LUN ids should be contiguous.\n");
> +			return -EINVAL;
>  		}
>  	} else {
>  		return -EINVAL;
> @@ -3388,6 +3393,8 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
>  
>  	fsg_common_remove_lun(lun_opts->lun, fsg_opts->common->sysfs);
>  	fsg_opts->common->luns[lun_opts->lun_id] = NULL;
> +	WARN_ON(fsg_opts->common->nluns == 0);
> +	fsg_opts->common->nluns--;
>  	lun_opts->lun_id = 0;
>  	mutex_unlock(&fsg_opts->lock);
>  
> @@ -3540,7 +3547,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
>  		rc = PTR_ERR(opts->common);
>  		goto release_opts;
>  	}
> -	rc = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> +	rc = fsg_common_set_max_luns(opts->common, FSG_MAX_LUNS);
>  	if (rc)
>  		goto release_opts;
>  
> diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
> index b4866fc..47d366a 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.h
> +++ b/drivers/usb/gadget/function/f_mass_storage.h
> @@ -143,7 +143,7 @@ void fsg_common_remove_luns(struct fsg_common *common);
>  
>  void fsg_common_free_luns(struct fsg_common *common);
>  
> -int fsg_common_set_nluns(struct fsg_common *common, int nluns);
> +int fsg_common_set_max_luns(struct fsg_common *common, int max_luns);
>  
>  void fsg_common_set_ops(struct fsg_common *common,
>  			const struct fsg_operations *ops);
> diff --git a/drivers/usb/gadget/legacy/acm_ms.c b/drivers/usb/gadget/legacy/acm_ms.c
> index 1194b09..1c56196 100644
> --- a/drivers/usb/gadget/legacy/acm_ms.c
> +++ b/drivers/usb/gadget/legacy/acm_ms.c
> @@ -200,9 +200,9 @@ static int acm_ms_bind(struct usb_composite_dev *cdev)
>  	if (status)
>  		goto fail;
>  
> -	status = fsg_common_set_nluns(opts->common, config.nluns);
> +	status = fsg_common_set_max_luns(opts->common, config.nluns);
>  	if (status)
> -		goto fail_set_nluns;
> +		goto fail_set_max_luns;
>  
>  	status = fsg_common_set_cdev(opts->common, cdev, config.can_stall);
>  	if (status)
> @@ -240,7 +240,7 @@ fail_string_ids:
>  	fsg_common_remove_luns(opts->common);
>  fail_set_cdev:
>  	fsg_common_free_luns(opts->common);
> -fail_set_nluns:
> +fail_set_max_luns:
>  	fsg_common_free_buffers(opts->common);
>  fail:
>  	usb_put_function_instance(fi_msg);
> diff --git a/drivers/usb/gadget/legacy/mass_storage.c b/drivers/usb/gadget/legacy/mass_storage.c
> index e7bfb08..7c2074c 100644
> --- a/drivers/usb/gadget/legacy/mass_storage.c
> +++ b/drivers/usb/gadget/legacy/mass_storage.c
> @@ -191,9 +191,9 @@ static int msg_bind(struct usb_composite_dev *cdev)
>  	if (status)
>  		goto fail;
>  
> -	status = fsg_common_set_nluns(opts->common, config.nluns);
> +	status = fsg_common_set_max_luns(opts->common, config.nluns);
>  	if (status)
> -		goto fail_set_nluns;
> +		goto fail_set_max_luns;
>  
>  	fsg_common_set_ops(opts->common, &ops);
>  
> @@ -228,7 +228,7 @@ fail_string_ids:
>  	fsg_common_remove_luns(opts->common);
>  fail_set_cdev:
>  	fsg_common_free_luns(opts->common);
> -fail_set_nluns:
> +fail_set_max_luns:
>  	fsg_common_free_buffers(opts->common);
>  fail:
>  	usb_put_function_instance(fi_msg);
> diff --git a/drivers/usb/gadget/legacy/multi.c b/drivers/usb/gadget/legacy/multi.c
> index b21b51f..df441b2 100644
> --- a/drivers/usb/gadget/legacy/multi.c
> +++ b/drivers/usb/gadget/legacy/multi.c
> @@ -407,9 +407,9 @@ static int __ref multi_bind(struct usb_composite_dev *cdev)
>  	if (status)
>  		goto fail2;
>  
> -	status = fsg_common_set_nluns(fsg_opts->common, config.nluns);
> +	status = fsg_common_set_max_luns(fsg_opts->common, config.nluns);
>  	if (status)
> -		goto fail_set_nluns;
> +		goto fail_set_max_luns;
>  
>  	status = fsg_common_set_cdev(fsg_opts->common, cdev, config.can_stall);
>  	if (status)
> @@ -449,7 +449,7 @@ fail_string_ids:
>  	fsg_common_remove_luns(fsg_opts->common);
>  fail_set_cdev:
>  	fsg_common_free_luns(fsg_opts->common);
> -fail_set_nluns:
> +fail_set_max_luns:
>  	fsg_common_free_buffers(fsg_opts->common);
>  fail2:
>  	usb_put_function_instance(fi_msg);
> -- 
> 1.7.9.5
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe stable" in