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

[PATCH v3 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 number of luns allocated by user - 1.

Best regards,

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

---
Changes since v2:
- drop nluns from fsg_common as discussed with Michal
- rebased onto balbi/fixes

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 v3 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>
CC: stable@vger.kernel.org # 3.13+
---
 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 f936268..f72102a 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3524,6 +3524,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);
@@ -3534,6 +3537,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 v3 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 f72102a..c3b62c7 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 v3 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 c3b62c7..9e88e2b 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

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

[Sergei Shtylyov]

Hello.

On 07/07/2015 05:57 PM, Krzysztof Opasiak wrote:

> Mass storage spec allows up to 16 LUNs, so let's don't

    s/don't/not/.

> add some more restrictive limits.

> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>

[...]

WBR, Sergei

[PATCH v3 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. According to MS spec we should return the max index of valid
LUN, not the number of luns - 1.

Reported-by: David Fisher <david.fisher1@synopsys.com>
Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
CC: stable@vger.kernel.org # 3.13+
---
 drivers/usb/gadget/function/f_mass_storage.c |  127 +++++++++-----------------
 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, 45 insertions(+), 104 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 9e88e2b..5fcd9a0 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -279,9 +279,8 @@ struct fsg_common {
 	int			cmnd_size;
 	u8			cmnd[MAX_COMMAND_SIZE];
 
-	unsigned int		nluns;
 	unsigned int		lun;
-	struct fsg_lun		**luns;
+	struct fsg_lun		*luns[FSG_MAX_LUNS];
 	struct fsg_lun		*curlun;
 
 	unsigned int		bulk_out_maxpacket;
@@ -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)
+{
+	int i = ARRAY_SIZE(common->luns) - 1;
+
+	while (i >= 0 && !common->luns[i])
+		--i;
+
+	return i;
+}
+
 static int fsg_setup(struct usb_function *f,
 		     const struct usb_ctrlrequest *ctrl)
 {
@@ -533,7 +542,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_get_max_lun(fsg->common);
 
 		/* Respond with data/status */
 		req->length = min((u16)1, w_length);
@@ -2131,8 +2140,9 @@ 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 ||
-			cbw->Length <= 0 || cbw->Length > MAX_COMMAND_SIZE) {
+	if (cbw->Lun >= ARRAY_SIZE(common->luns) ||
+	    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",
 				cbw->Lun, cbw->Flags, cbw->Length);
@@ -2159,7 +2169,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 +2317,7 @@ reset:
 	}
 
 	common->running = 1;
-	for (i = 0; i < common->nluns; ++i)
+	for (i = 0; i < ARRAY_SIZE(common->luns); ++i)
 		if (common->luns[i])
 			common->luns[i]->unit_attention_data =
 				SS_RESET_OCCURRED;
@@ -2409,7 +2419,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 < ARRAY_SIZE(common->luns); ++i) {
 			curlun = common->luns[i];
 			if (!curlun)
 				continue;
@@ -2453,7 +2463,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->ARRAY_SIZE(common->luns); ++i) */
 		/*	if (common->luns[i]) */
 		/*		common->luns[i]->unit_attention_data = */
 		/*			SS_RESET_OCCURRED;  */
@@ -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) {
+			struct fsg_lun *curlun = common->luns[i];
 			if (!curlun || !fsg_lun_is_open(curlun))
 				continue;
 
@@ -2676,6 +2685,7 @@ 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));
 
 	return common;
 }
@@ -2764,42 +2774,10 @@ static void _fsg_common_remove_luns(struct fsg_common *common, int n)
 
 void fsg_common_remove_luns(struct fsg_common *common)
 {
-	_fsg_common_remove_luns(common, common->nluns);
+	_fsg_common_remove_luns(common, ARRAY_SIZE(common->luns));
 }
 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(FSG_MAX_LUNS, sizeof(*curlun), GFP_KERNEL);
-	if (unlikely(!curlun))
-		return -ENOMEM;
-
-	if (common->luns)
-		fsg_common_free_luns(common);
-
-	common->luns = curlun;
-	common->nluns = nluns;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(fsg_common_set_nluns);
-
 void fsg_common_set_ops(struct fsg_common *common,
 			const struct fsg_operations *ops)
 {
@@ -2880,7 +2858,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 (id >= ARRAY_SIZE(common->luns))
 		return -ENODEV;
 
 	if (common->luns[id])
@@ -2964,14 +2942,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;
 
@@ -3020,6 +2998,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);
+	int i;
 
 	/* If the thread isn't already dead, tell it to exit now */
 	if (common->state != FSG_STATE_TERMINATED) {
@@ -3027,22 +3006,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);
@@ -3056,6 +3027,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;
@@ -3063,6 +3035,13 @@ 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 */
+	ret = _fsg_common_get_max_lun(common);
+	if (ret < 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,
@@ -3509,14 +3488,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");
 
@@ -3539,8 +3515,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);
@@ -3566,23 +3540,12 @@ static struct usb_function *fsg_alloc(struct usb_function_instance *fi)
 	struct fsg_opts *opts = fsg_opts_from_func_inst(fi);
 	struct fsg_common *common = opts->common;
 	struct fsg_dev *fsg;
-	unsigned nluns, i;
 
 	fsg = kzalloc(sizeof(*fsg), GFP_KERNEL);
 	if (unlikely(!fsg))
 		return ERR_PTR(-ENOMEM);
 
 	mutex_lock(&opts->lock);
-	if (!opts->refcnt) {
-		for (nluns = i = 0; i < FSG_MAX_LUNS; ++i)
-			if (common->luns[i])
-				nluns = i + 1;
-		if (!nluns)
-			pr_warn("No LUNS defined, continuing anyway\n");
-		else
-			common->nluns = nluns;
-		pr_info("Number of LUNs=%u\n", common->nluns);
-	}
 	opts->refcnt++;
 	mutex_unlock(&opts->lock);
 
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

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

[Michal Nazarewicz]

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?

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

> +{
> +	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;
>  

-- 
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 v3 4/5] usb: gadget: mass_storage: Use static array for luns

[Krzysztof Opasiak]

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

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

[Michal Nazarewicz]

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

On Mon, Jul 20 2015, Krzysztof Opasiak wrote:
> 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).

I don’t believe that’s accurate.  See loop in my patch:

+       if (!opts->refcnt) {
+               for (nluns = i = 0; i < FSG_MAX_LUNS; ++i)
+                       if (common->luns[i])
+                               nluns = i + 1;
+               if (!nluns)
+                       pr_warn("No LUNS defined, continuing anyway\n");
+               else
+                       common->nluns = nluns;
+               pr_info("Number of LUNs=%u\n", common->nluns);
+       }

It iterates over all luns and if lun is non-NULL it sets nluns to index
of that lun plus one.  So if luns[0], lun[1], lun[2], lun[3] and lun[5]
are set, common->nluns = 6.

-- 
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 v3 4/5] usb: gadget: mass_storage: Use static array for luns

[Krzysztof Opasiak]

On 07/20/2015 03:17 PM, Michal Nazarewicz wrote:
>> 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?
>
> On Mon, Jul 20 2015, Krzysztof Opasiak wrote:
>> 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).
>
> I don’t believe that’s accurate.  See loop in my patch:
>
> +       if (!opts->refcnt) {
> +               for (nluns = i = 0; i < FSG_MAX_LUNS; ++i)
> +                       if (common->luns[i])
> +                               nluns = i + 1;
> +               if (!nluns)
> +                       pr_warn("No LUNS defined, continuing anyway\n");
> +               else
> +                       common->nluns = nluns;
> +               pr_info("Number of LUNs=%u\n", common->nluns);
> +       }
>
> It iterates over all luns and if lun is non-NULL it sets nluns to index
> of that lun plus one.  So if luns[0], lun[1], lun[2], lun[3] and lun[5]
> are set, common->nluns = 6.
>

Yeah I see it now. What I wrote was a problem before your patch but now 
it seams to be ok, just nluns name is very misleading. Summing up, just 
like you wrote this patch should not go to stable.

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

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

[Michal Nazarewicz]

On Mon, Jul 20 2015, Krzysztof Opasiak wrote:
> Yeah I see it now. What I wrote was a problem before your patch but now 
> it seams to be ok, just nluns name is very misleading. Summing up, just 
> like you wrote this patch should not go to stable.

For me nluns has always been *size* of the luns array.  And with changes
from my patch, it’s size in the std::vector sense, i.e. it’s not the
same as capacity.

Your patchset gets away from nluns so this confusion will be cleared
anyway.

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

[PATCH v3 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 |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 5fcd9a0..69167ad 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3042,6 +3042,13 @@ 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 (ret != 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 v3 5/5] usb: gadget: mass_storage: Warn if LUNs ids are not contiguous

[Michal Nazarewicz]

On Tue, Jul 07 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 |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 5fcd9a0..69167ad 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3042,6 +3042,13 @@ 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 (ret != 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--