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

2015-06-22 Thread 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.fish...@synopsys.com
Signed-off-by: Krzysztof Opasiak k.opas...@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 RD Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line unsubscribe linux-usb in


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

2015-06-22 Thread 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.fish...@synopsys.com
 Signed-off-by: Krzysztof Opasiak k.opas...@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 intnluns;
 + unsigned intmax_luns;
   unsigned intlun;
   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