[PATCH 0/5] Mass storage fixes and improvements

[PATCH 0/5] Mass storage fixes and improvements

[Krzysztof Opasiak]

Hello,

This series fix a few bugs in mass storage function and disallows to
bind function with not contiguous LUN ids.

Last patch in this series fix GET_MAX_LUNS request to return actual
number of luns, not the number of prealocated space.

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: Enforce contiguous LUN ids
  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: Send correct number of LUNs to host

 drivers/usb/gadget/function/f_mass_storage.c |   72 ++++++++++++++++++++------
 drivers/usb/gadget/function/f_mass_storage.h |    2 +-
 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, 67 insertions(+), 27 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe stable" in

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

--
To unsubscribe from this list: send the line "unsubscribe stable" in

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

[Michal Nazarewicz]

On Mon, Jun 22 2015, 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>
> Cc: <stable@vger.kernel.org> # 3.13+

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
>

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

[PATCH 2/5] usb: gadget: mass_storage: Enforce contiguous LUN ids

[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 don't allow to bind ms function unless we have at least LUN0
and LUNs ids are contiguous.

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

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 2e8f37e..19b31d7 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3065,6 +3065,35 @@ 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
+	 * 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;
+			}
+		}
+
+		if (i == 0 || !common->luns[i]) {
+			pr_err("There should be at least one LUN.\n");
+			return -EINVAL;
+		}
+	} else {
+		return -EINVAL;
+	}
+
 	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

--
To unsubscribe from this list: send the line "unsubscribe stable" in

Re: [PATCH 2/5] usb: gadget: mass_storage: Enforce contiguous LUN ids

[Michal Nazarewicz]

On Mon, Jun 22 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 don't allow to bind ms function unless we have at least LUN0
> and LUNs ids are contiguous.
>
> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>

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

but then again I think that we should let user space shoot themselves in
the foot if they want to, especially as letting them to that is less
code. ;)

> ---
>  drivers/usb/gadget/function/f_mass_storage.c |   29 ++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 2e8f37e..19b31d7 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3065,6 +3065,35 @@ 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
> +	 * 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;
> +			}
> +		}
> +
> +		if (i == 0 || !common->luns[i]) {
> +			pr_err("There should be at least one LUN.\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
>  	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--
--
To unsubscribe from this list: send the line "unsubscribe stable" in

Re: [PATCH 2/5] usb: gadget: mass_storage: Enforce contiguous LUN ids

[Alan Stern]

On Mon, 22 Jun 2015, Michal Nazarewicz wrote:

> On Mon, Jun 22 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 don't allow to bind ms function unless we have at least LUN0
> > and LUNs ids are contiguous.
> >
> > Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
> 
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> 
> but then again I think that we should let user space shoot themselves in
> the foot if they want to, especially as letting them to that is less
> code. ;)

How about logging a warning message if the LUNs aren't contiguous but 
then continuing on?  Like we do if the serial number string isn't 
provided.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe stable" in

Re: [PATCH 2/5] usb: gadget: mass_storage: Enforce contiguous LUN ids

[Krzysztof Opasiak]

On 06/22/2015 04:45 PM, Alan Stern wrote:
> On Mon, 22 Jun 2015, Michal Nazarewicz wrote:
>
>> On Mon, Jun 22 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 don't allow to bind ms function unless we have at least LUN0
>>> and LUNs ids are contiguous.
>>>
>>> Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
>>
>> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>>
>> but then again I think that we should let user space shoot themselves in
>> the foot if they want to, especially as letting them to that is less
>> code. ;)
>

It's a little bit philosophical problem about "Should we validate user 
configuration and data?". A good example may be validating MAC provided 
by user. Not doing this will also reduce the amount of code but we 
return error and notify user instead of making up how to interpret some 
invalid data.

Problem with not contiguous LUNs is that some hosts (in particular 
Linux) stop discovering them when they find first invalid LUN. So if we 
allocate LUNs 0, 1, 2, 7, 8, Linux host (at least the one tested by me) 
will see only first three of them and of course it comply with spec 
which says that LUNs should be contiguous. (Windows is not so 
restrictive and continues until value returned from Get Max LUN)


> How about logging a warning message if the LUNs aren't contiguous but
> then continuing on?  Like we do if the serial number string isn't
> provided.

I think that a good log message is much better than nothing;)

If there is no agreement on enforcing contiguous LUNs I will update the 
patches and print some warn instead of returning error.

BR's
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 3/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>
---
 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 19b31d7..4257238 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

--
To unsubscribe from this list: send the line "unsubscribe stable" in

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

[Michal Nazarewicz]

On Mon, Jun 22 2015, Krzysztof Opasiak wrote:
> 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 19b31d7..4257238 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
>

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

[PATCH 4/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>
---
 drivers/usb/gadget/function/storage_common.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

--
To unsubscribe from this list: send the line "unsubscribe stable" in

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

[Michal Nazarewicz]

On Mon, Jun 22 2015, Krzysztof Opasiak wrote:
> 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/storage_common.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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
>

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

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

[Krzysztof Opasiak]

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

--
To unsubscribe from this list: send the line "unsubscribe stable" in

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

[Michal Nazarewicz]

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

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

[Krzysztof Opasiak]

On 06/22/2015 04:34 PM, Michal Nazarewicz wrote:
> 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.

Sounds like a good idea and I also though about it but I gave up due to 
memory usage. Most legacy gadgets reduce size of allocated luns array to 
number of received luns in module params. Adding array with static size 
will make this impossible. If we may don't care about this ~80 bytes I 
will be happy make common->luns array as it simplifies the code.

BR's
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe stable" in