[PATCH v2 0/5] Mass storage fixes and improvements

[PATCH v2 0/5] Mass storage fixes and improvements

[Krzysztof Opasiak]

Hello,

This series fix a few bugs in mass storage function, adds a warning
message when binding function with not contiguous LUN ids and replace
dynamically allocated luns array with static one what simplifies the
code.

This series also fix GET_MAX_LUNS request to return max id of valid
lun, not some number of slots in luns array.

Best regards,

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics 

Krzysztof Opasiak (5):
  usb: gadget: mass_storage: Free buffers if create lun fails
  usb: gadget: mass_storage: Place EXPORT_SYMBOL_GPL() after func
    definition
  usb: gadget: storage-common: Set FSG_MAX_LUNS to 16
  usb: gadget: mass_storage: Use static array for luns
  usb: gadget: mass_storage: Warn if LUNs ids are not contiguous

 drivers/usb/gadget/function/f_mass_storage.c |  140 +++++++++++---------------
 drivers/usb/gadget/function/f_mass_storage.h |    4 -
 drivers/usb/gadget/function/storage_common.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 --
 6 files changed, 62 insertions(+), 102 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/5] usb: gadget: mass_storage: Free buffers if create lun fails

[Krzysztof Opasiak]

Creation of LUN 0 may fail (for example due to ENOMEM).
As fsg_common_set_num_buffers() does some memory allocation
we should free it before it becomes unavailable.

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_mass_storage.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 3cc109f..2e8f37e 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3526,6 +3526,9 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 	config.removable = true;
 	rc = fsg_common_create_lun(opts->common, &config, 0, "lun.0",
 			(const char **)&opts->func_inst.group.cg_item.ci_name);
+	if (rc)
+		goto release_buffers;
+
 	opts->lun0.lun = opts->common->luns[0];
 	opts->lun0.lun_id = 0;
 	config_group_init_type_name(&opts->lun0.group, "lun.0", &fsg_lun_type);
@@ -3536,6 +3539,8 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 
 	return &opts->func_inst;
 
+release_buffers:
+	fsg_common_free_buffers(opts->common);
 release_luns:
 	kfree(opts->common->luns);
 release_opts:
-- 
1.7.9.5

[PATCH v2 2/5] usb: gadget: mass_storage: Place EXPORT_SYMBOL_GPL() after func definition

[Krzysztof Opasiak]

EXPORT_SYMBOL_GPL() is usually placed after function definition
not before.

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_mass_storage.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 2e8f37e..48af0a6 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2761,12 +2761,12 @@ static void _fsg_common_remove_luns(struct fsg_common *common, int n)
 			common->luns[i] = NULL;
 		}
 }
-EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
 
 void fsg_common_remove_luns(struct fsg_common *common)
 {
 	_fsg_common_remove_luns(common, common->nluns);
 }
+EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
 
 void fsg_common_free_luns(struct fsg_common *common)
 {
-- 
1.7.9.5

[PATCH v2 3/5] usb: gadget: storage-common: Set FSG_MAX_LUNS to 16

[Krzysztof Opasiak]

Mass storage spec allows up to 16 LUNs, so let's don't
add some more restrictive limits.

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_mass_storage.c |    2 +-
 drivers/usb/gadget/function/storage_common.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 48af0a6..ad0f69b 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -54,7 +54,7 @@
  * following fields:
  *
  *	nluns		Number of LUNs function have (anywhere from 1
- *				to FSG_MAX_LUNS which is 8).
+ *				to FSG_MAX_LUNS).
  *	luns		An array of LUN configuration values.  This
  *				should be filled for each LUN that
  *				function will include (ie. for "nluns"
diff --git a/drivers/usb/gadget/function/storage_common.h b/drivers/usb/gadget/function/storage_common.h
index 70c8914..c3544e6 100644
--- a/drivers/usb/gadget/function/storage_common.h
+++ b/drivers/usb/gadget/function/storage_common.h
@@ -123,7 +123,7 @@ static inline bool fsg_lun_is_open(struct fsg_lun *curlun)
 #define FSG_BUFLEN	((u32)16384)
 
 /* Maximal number of LUNs supported in mass storage function */
-#define FSG_MAX_LUNS	8
+#define FSG_MAX_LUNS	16
 
 enum fsg_buffer_state {
 	BUF_STATE_EMPTY = 0,
-- 
1.7.9.5

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

[Krzysztof Opasiak]

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;
 	unsigned int		lun;
-	struct fsg_lun		**luns;
+	struct fsg_lun		*luns[FSG_MAX_LUNS];
 	struct fsg_lun		*curlun;
 
 	unsigned int		bulk_out_maxpacket;
@@ -533,7 +533,7 @@ static int fsg_setup(struct usb_function *f,
 				w_length != 1)
 			return -EDOM;
 		VDBG(fsg, "get max LUN\n");
-		*(u8 *)req->buf = fsg->common->nluns - 1;
+		*(u8 *)req->buf = fsg->common->max_lun;
 
 		/* Respond with data/status */
 		req->length = min((u16)1, w_length);
@@ -2131,7 +2131,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->max_lun || 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",
@@ -2159,7 +2159,7 @@ static int received_cbw(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 	if (common->data_size == 0)
 		common->data_dir = DATA_DIR_NONE;
 	common->lun = cbw->Lun;
-	if (common->lun < common->nluns)
+	if (common->lun < ARRAY_SIZE(common->luns))
 		common->curlun = common->luns[common->lun];
 	else
 		common->curlun = NULL;
@@ -2307,7 +2307,7 @@ reset:
 	}
 
 	common->running = 1;
-	for (i = 0; i < common->nluns; ++i)
+	for (i = 0; i <= common->max_lun; ++i)
 		if (common->luns[i])
 			common->luns[i]->unit_attention_data =
 				SS_RESET_OCCURRED;
@@ -2409,7 +2409,7 @@ static void handle_exception(struct fsg_common *common)
 	if (old_state == FSG_STATE_ABORT_BULK_OUT)
 		common->state = FSG_STATE_STATUS_PHASE;
 	else {
-		for (i = 0; i < common->nluns; ++i) {
+		for (i = 0; i <= common->max_lun; ++i) {
 			curlun = common->luns[i];
 			if (!curlun)
 				continue;
@@ -2453,7 +2453,7 @@ static void handle_exception(struct fsg_common *common)
 		 * a waste of time.  Ditto for the INTERFACE_CHANGE and
 		 * CONFIG_CHANGE cases.
 		 */
-		/* for (i = 0; i < common->nluns; ++i) */
+		/* for (i = 0; i <= common->max_lun; ++i) */
 		/*	if (common->luns[i]) */
 		/*		common->luns[i]->unit_attention_data = */
 		/*			SS_RESET_OCCURRED;  */
@@ -2552,12 +2552,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 = 0;
 
 		down_write(&common->filesem);
-		for (; i--; ++curlun_it) {
-			struct fsg_lun *curlun = *curlun_it;
+		for (; i <= common->max_lun; ++i) {
+			struct fsg_lun *curlun = common->luns[i];
 			if (!curlun || !fsg_lun_is_open(curlun))
 				continue;
 
@@ -2676,6 +2675,8 @@ static struct fsg_common *fsg_common_setup(struct fsg_common *common)
 	init_completion(&common->thread_notifier);
 	init_waitqueue_head(&common->fsg_wait);
 	common->state = FSG_STATE_TERMINATED;
+	memset(common->luns, 0, sizeof(common->luns));
+	common->max_lun = -1;
 
 	return common;
 }
@@ -2751,6 +2752,12 @@ void fsg_common_remove_lun(struct fsg_lun *lun, bool sysfs)
 }
 EXPORT_SYMBOL_GPL(fsg_common_remove_lun);
 
+static void _fsg_common_reduce_max_lun(struct fsg_common *common)
+{
+	while (common->max_lun >= 0 && common->luns[common->max_lun] == NULL)
+		common->max_lun--;
+}
+
 static void _fsg_common_remove_luns(struct fsg_common *common, int n)
 {
 	int i;
@@ -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);
 }
 EXPORT_SYMBOL_GPL(fsg_common_remove_luns);
 
-void fsg_common_free_luns(struct fsg_common *common)
-{
-	fsg_common_remove_luns(common);
-	kfree(common->luns);
-	common->luns = NULL;
-}
-EXPORT_SYMBOL_GPL(fsg_common_free_luns);
-
-int fsg_common_set_nluns(struct fsg_common *common, int nluns)
-{
-	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);
-		return -EINVAL;
-	}
-
-	curlun = kcalloc(nluns, sizeof(*curlun), GFP_KERNEL);
-	if (unlikely(!curlun))
-		return -ENOMEM;
-
-	if (common->luns)
-		fsg_common_free_luns(common);
-
-	common->luns = curlun;
-	common->nluns = nluns;
-
-	pr_info("Number of LUNs=%d\n", common->nluns);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(fsg_common_set_nluns);
-
 void fsg_common_set_ops(struct fsg_common *common,
 			const struct fsg_operations *ops)
 {
@@ -2882,8 +2857,8 @@ 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)
-		return -ENODEV;
+	if (id >= ARRAY_SIZE(common->luns))
+		return -EINVAL;
 
 	if (common->luns[id])
 		return -EBUSY;
@@ -2924,6 +2899,8 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg,
 	}
 
 	common->luns[id] = lun;
+	if (common->max_lun < (int)id)
+		common->max_lun = id;
 
 	if (cfg->filename) {
 		rc = fsg_lun_open(lun, cfg->filename);
@@ -2954,7 +2931,7 @@ error_lun:
 	if (common->sysfs)
 		device_unregister(&lun->dev);
 	fsg_lun_close(lun);
-	common->luns[id] = NULL;
+	_fsg_common_reduce_max_lun(common);
 error_sysfs:
 	kfree(lun);
 	return rc;
@@ -2966,14 +2943,14 @@ 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) {
+	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)
 			goto fail;
 	}
 
-	pr_info("Number of LUNs=%d\n", common->nluns);
+	pr_info("Number of LUNs=%d\n", cfg->nluns);
 
 	return 0;
 
@@ -3022,6 +2999,7 @@ EXPORT_SYMBOL_GPL(fsg_common_run_thread);
 static void fsg_common_release(struct kref *ref)
 {
 	struct fsg_common *common = container_of(ref, struct fsg_common, ref);
+	unsigned int i;
 
 	/* If the thread isn't already dead, tell it to exit now */
 	if (common->state != FSG_STATE_TERMINATED) {
@@ -3029,22 +3007,14 @@ static void fsg_common_release(struct kref *ref)
 		wait_for_completion(&common->thread_notifier);
 	}
 
-	if (likely(common->luns)) {
-		struct fsg_lun **lun_it = common->luns;
-		unsigned i = common->nluns;
-
-		/* In error recovery common->nluns may be zero. */
-		for (; i; --i, ++lun_it) {
-			struct fsg_lun *lun = *lun_it;
-			if (!lun)
-				continue;
-			fsg_lun_close(lun);
-			if (common->sysfs)
-				device_unregister(&lun->dev);
-			kfree(lun);
-		}
-
-		kfree(common->luns);
+	for (i = 0; i < ARRAY_SIZE(common->luns); ++i) {
+		struct fsg_lun *lun = common->luns[i];
+		if (!lun)
+			continue;
+		fsg_lun_close(lun);
+		if (common->sysfs)
+			device_unregister(&lun->dev);
+		kfree(lun);
 	}
 
 	_fsg_common_free_buffers(common->buffhds, common->fsg_num_buffers);
@@ -3058,6 +3028,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;
@@ -3065,6 +3036,12 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
 	int			ret;
 	struct fsg_opts		*opts;
 
+	/* Don't allow to bind if we don't have at least one LUN */
+	if (common->max_lun < 0) {
+		pr_err("There should be at least one LUN.\n");
+		return -EINVAL;
+	}
+
 	opts = fsg_opts_from_func_inst(f->fi);
 	if (!opts->no_configfs) {
 		ret = fsg_common_set_cdev(fsg->common, c->cdev,
@@ -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);
 
 	mutex_lock(&fsg_opts->lock);
-	if (fsg_opts->refcnt || fsg_opts->common->luns[num]) {
+	if (fsg_opts->refcnt) {
 		ret = -EBUSY;
 		goto out;
 	}
@@ -3359,6 +3334,7 @@ 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;
+	_fsg_common_reduce_max_lun(fsg_opts->common);
 	lun_opts->lun_id = 0;
 	mutex_unlock(&fsg_opts->lock);
 
@@ -3511,14 +3487,11 @@ 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);
-	if (rc)
-		goto release_opts;
 
 	rc = fsg_common_set_num_buffers(opts->common,
 					CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS);
 	if (rc)
-		goto release_luns;
+		goto release_opts;
 
 	pr_info(FSG_DRIVER_DESC ", version: " FSG_DRIVER_VERSION "\n");
 
@@ -3541,8 +3514,6 @@ static struct usb_function_instance *fsg_alloc_inst(void)
 
 release_buffers:
 	fsg_common_free_buffers(opts->common);
-release_luns:
-	kfree(opts->common->luns);
 release_opts:
 	kfree(opts);
 	return ERR_PTR(rc);
diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
index b4866fc..764807c 100644
--- a/drivers/usb/gadget/function/f_mass_storage.h
+++ b/drivers/usb/gadget/function/f_mass_storage.h
@@ -141,10 +141,6 @@ void fsg_common_remove_lun(struct fsg_lun *lun, bool sysfs);
 
 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);
-
 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..3c8e309 100644
--- a/drivers/usb/gadget/legacy/acm_ms.c
+++ b/drivers/usb/gadget/legacy/acm_ms.c
@@ -200,10 +200,6 @@ static int acm_ms_bind(struct usb_composite_dev *cdev)
 	if (status)
 		goto fail;
 
-	status = fsg_common_set_nluns(opts->common, config.nluns);
-	if (status)
-		goto fail_set_nluns;
-
 	status = fsg_common_set_cdev(opts->common, cdev, config.can_stall);
 	if (status)
 		goto fail_set_cdev;
@@ -239,8 +235,6 @@ static int acm_ms_bind(struct usb_composite_dev *cdev)
 fail_string_ids:
 	fsg_common_remove_luns(opts->common);
 fail_set_cdev:
-	fsg_common_free_luns(opts->common);
-fail_set_nluns:
 	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..8e2be7f 100644
--- a/drivers/usb/gadget/legacy/mass_storage.c
+++ b/drivers/usb/gadget/legacy/mass_storage.c
@@ -191,10 +191,6 @@ static int msg_bind(struct usb_composite_dev *cdev)
 	if (status)
 		goto fail;
 
-	status = fsg_common_set_nluns(opts->common, config.nluns);
-	if (status)
-		goto fail_set_nluns;
-
 	fsg_common_set_ops(opts->common, &ops);
 
 	status = fsg_common_set_cdev(opts->common, cdev, config.can_stall);
@@ -227,8 +223,6 @@ static int msg_bind(struct usb_composite_dev *cdev)
 fail_string_ids:
 	fsg_common_remove_luns(opts->common);
 fail_set_cdev:
-	fsg_common_free_luns(opts->common);
-fail_set_nluns:
 	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..fb683e3 100644
--- a/drivers/usb/gadget/legacy/multi.c
+++ b/drivers/usb/gadget/legacy/multi.c
@@ -407,10 +407,6 @@ static int __ref multi_bind(struct usb_composite_dev *cdev)
 	if (status)
 		goto fail2;
 
-	status = fsg_common_set_nluns(fsg_opts->common, config.nluns);
-	if (status)
-		goto fail_set_nluns;
-
 	status = fsg_common_set_cdev(fsg_opts->common, cdev, config.can_stall);
 	if (status)
 		goto fail_set_cdev;
@@ -448,8 +444,6 @@ static int __ref multi_bind(struct usb_composite_dev *cdev)
 fail_string_ids:
 	fsg_common_remove_luns(fsg_opts->common);
 fail_set_cdev:
-	fsg_common_free_luns(fsg_opts->common);
-fail_set_nluns:
 	fsg_common_free_buffers(fsg_opts->common);
 fail2:
 	usb_put_function_instance(fi_msg);
-- 
1.7.9.5

[PATCH v2 5/5] usb: gadget: mass_storage: Warn if LUNs ids are not contiguous

[Krzysztof Opasiak]

According to mass storage specification:

"Logical Unit Numbers on the device shall be numbered contiguously
 starting from LUN 0 to a maximum LUN of 15 (Fh)"

So let's at least print a warning message that LUNs ids should be
contiguous.

Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
 drivers/usb/gadget/function/f_mass_storage.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index befe251..08a4fdb 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3042,6 +3042,12 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
 		return -EINVAL;
 	}
 
+	for (i = 0; i < ARRAY_SIZE(common->luns); ++i)
+		if (!common->luns[i])
+			break;
+	if (common->max_lun != i - 1)
+		pr_err("LUN ids should be contiguous.\n");
+
 	opts = fsg_opts_from_func_inst(f->fi);
 	if (!opts->no_configfs) {
 		ret = fsg_common_set_cdev(fsg->common, c->cdev,
-- 
1.7.9.5

Re: [PATCH v2 1/5] usb: gadget: mass_storage: Free buffers if create lun fails

[Krzysztof Opasiak]

Felippe,

Could you please take this patch as a fix now and leave the rest of this 
series to merge window or should I resend it separately?

On 07/02/2015 08:56 PM, Krzysztof Opasiak wrote:
> Creation of LUN 0 may fail (for example due to ENOMEM).
> As fsg_common_set_num_buffers() does some memory allocation
> we should free it before it becomes unavailable.
>
> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> ---
>   drivers/usb/gadget/function/f_mass_storage.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 3cc109f..2e8f37e 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3526,6 +3526,9 @@ static struct usb_function_instance *fsg_alloc_inst(void)
>   	config.removable = true;
>   	rc = fsg_common_create_lun(opts->common, &config, 0, "lun.0",
>   			(const char **)&opts->func_inst.group.cg_item.ci_name);
> +	if (rc)
> +		goto release_buffers;
> +
>   	opts->lun0.lun = opts->common->luns[0];
>   	opts->lun0.lun_id = 0;
>   	config_group_init_type_name(&opts->lun0.group, "lun.0", &fsg_lun_type);
> @@ -3536,6 +3539,8 @@ static struct usb_function_instance *fsg_alloc_inst(void)
>
>   	return &opts->func_inst;
>
> +release_buffers:
> +	fsg_common_free_buffers(opts->common);
>   release_luns:
>   	kfree(opts->common->luns);
>   release_opts:
>

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

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

[Michal Nazarewicz]

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.

>  	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);

>  }
>  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.

> +	_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.

>  
>  	mutex_lock(&fsg_opts->lock);
> -	if (fsg_opts->refcnt || fsg_opts->common->luns[num]) {
> +	if (fsg_opts->refcnt) {
>  		ret = -EBUSY;
>  		goto out;
>  	}

-- 
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--

Re: [PATCH v2 5/5] usb: gadget: mass_storage: Warn if LUNs ids are not contiguous

[Michal Nazarewicz]

On Thu, Jul 02 2015, Krzysztof Opasiak wrote:
> According to mass storage specification:
>
> "Logical Unit Numbers on the device shall be numbered contiguously
>  starting from LUN 0 to a maximum LUN of 15 (Fh)"
>
> So let's at least print a warning message that LUNs ids should be
> contiguous.
>
> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> ---
>  drivers/usb/gadget/function/f_mass_storage.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index befe251..08a4fdb 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3042,6 +3042,12 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
>  		return -EINVAL;
>  	}
>  
> +	for (i = 0; i < ARRAY_SIZE(common->luns); ++i)
> +		if (!common->luns[i])
> +			break;
> +	if (common->max_lun != i - 1)
> +		pr_err("LUN ids should be contiguous.\n");
> +
>  	opts = fsg_opts_from_func_inst(f->fi);
>  	if (!opts->no_configfs) {
>  		ret = fsg_common_set_cdev(fsg->common, c->cdev,
> -- 
> 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--

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

[Krzysztof Opasiak]

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