Subject: [PATCH] usb: gadget: function: Remove redundant usb_free_all_descriptors

2014-10-13 Thread pavi1729 Sid 
Removed the usb_free_all_descriptors call in *_bind functions
as this call is already present in usb_assign_descriptors.
usb_assign_descriptors is the only call where usb descriptor
allocation happens, also in case of error freeing of the
allocated memory takes place in the same call. Hence the
call in the *_bind functions are redundant.
Also the presence of usb_free_all_descriptors in the *_bind
functions might result in double-free corruption of the
allocated mmeory.

Signed-off-by: Pavitrakumar Managutte pavitra1...@gmail.com
---
 drivers/usb/gadget/function/f_eem.c|  1 -
 drivers/usb/gadget/function/f_hid.c|  7 +++
 drivers/usb/gadget/function/f_ncm.c|  1 -
 drivers/usb/gadget/function/f_phonet.c |  1 -
 drivers/usb/gadget/function/f_rndis.c  |  5 +++--
 drivers/usb/gadget/function/f_subset.c |  1 -
 drivers/usb/gadget/function/f_uac2.c   | 10 ++
 7 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/function/f_eem.c
b/drivers/usb/gadget/function/f_eem.c
index 4d8b236..c9e90de 100644
--- a/drivers/usb/gadget/function/f_eem.c
+++ b/drivers/usb/gadget/function/f_eem.c
@@ -325,7 +325,6 @@ static int eem_bind(struct usb_configuration *c,
struct usb_function *f)
 return 0;

 fail:
-usb_free_all_descriptors(f);
 if (eem-port.out_ep)
 eem-port.out_ep-driver_data = NULL;
 if (eem-port.in_ep)
diff --git a/drivers/usb/gadget/function/f_hid.c
b/drivers/usb/gadget/function/f_hid.c
index a95290a..c36de0c 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -621,12 +621,14 @@ static int __init hidg_bind(struct
usb_configuration *c, struct usb_function *f)
 dev = MKDEV(major, hidg-minor);
 status = cdev_add(hidg-cdev, dev, 1);
 if (status)
-goto fail;
+goto fail_free_descs;

 device_create(hidg_class, NULL, dev, NULL, %s%d, hidg, hidg-minor);

 return 0;

+fail_free_descs:
+usb_free_all_descriptors(f);
 fail:
 ERROR(f-config-cdev, hidg_bind FAILED\n);
 if (hidg-req != NULL) {
@@ -635,7 +637,6 @@ fail:
 usb_ep_free_request(hidg-in_ep, hidg-req);
 }

-usb_free_all_descriptors(f);
 return status;
 }

@@ -652,8 +653,6 @@ static void hidg_unbind(struct usb_configuration
*c, struct usb_function *f)
 kfree(hidg-req-buf);
 usb_ep_free_request(hidg-in_ep, hidg-req);

-usb_free_all_descriptors(f);
-
 kfree(hidg-report_desc);
 kfree(hidg);
 }
diff --git a/drivers/usb/gadget/function/f_ncm.c
b/drivers/usb/gadget/function/f_ncm.c
index bcdc882..38a9279 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1453,7 +1453,6 @@ static int ncm_bind(struct usb_configuration *c,
struct usb_function *f)
 return 0;

 fail:
-usb_free_all_descriptors(f);
 if (ncm-notify_req) {
 kfree(ncm-notify_req-buf);
 usb_ep_free_request(ncm-notify, ncm-notify_req);
diff --git a/drivers/usb/gadget/function/f_phonet.c
b/drivers/usb/gadget/function/f_phonet.c
index b9cfc15..74ddcac 100644
--- a/drivers/usb/gadget/function/f_phonet.c
+++ b/drivers/usb/gadget/function/f_phonet.c
@@ -571,7 +571,6 @@ err_req:
 for (i = 0; i  phonet_rxq_size  fp-out_reqv[i]; i++)
 usb_ep_free_request(fp-out_ep, fp-out_reqv[i]);
 err:
-usb_free_all_descriptors(f);
 if (fp-out_ep)
 fp-out_ep-driver_data = NULL;
 if (fp-in_ep)
diff --git a/drivers/usb/gadget/function/f_rndis.c
b/drivers/usb/gadget/function/f_rndis.c
index ddb09dc..2f0517f 100644
--- a/drivers/usb/gadget/function/f_rndis.c
+++ b/drivers/usb/gadget/function/f_rndis.c
@@ -803,7 +803,7 @@ rndis_bind(struct usb_configuration *c, struct
usb_function *f)
 if (rndis-manufacturer  rndis-vendorID 
 rndis_set_param_vendor(rndis-config, rndis-vendorID,
rndis-manufacturer))
-goto fail;
+goto fail_free_descs;

 /* NOTE:  all that is done without knowing or caring about
  * the network link ... which is unavailable to this code
@@ -817,10 +817,11 @@ rndis_bind(struct usb_configuration *c, struct
usb_function *f)
 rndis-notify-name);
 return 0;

+fail_free_descs:
+usb_free_all_descriptors(f);
 fail:
 kfree(f-os_desc_table);
 f-os_desc_n = 0;
-usb_free_all_descriptors(f);

 if (rndis-notify_req) {
 kfree(rndis-notify_req-buf);
diff --git a/drivers/usb/gadget/function/f_subset.c
b/drivers/usb/gadget/function/f_subset.c
index 1ea8baf..e3dfa67 100644
--- a/drivers/usb/gadget/function/f_subset.c
+++ b/drivers/usb/gadget/function/f_subset.c
@@ -380,7 +380,6 @@ geth_bind(struct usb_configuration *c, struct
usb_function *f)
 return 0;

 fail:
-usb_free_all_descriptors(f);
 /* we might as well release our claims on endpoints */
 if (geth-port.out_ep)
 geth-port.out_ep-driver_data = NULL;
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 3ed89ec..b45c39c

Re: Subject: [PATCH] usb: gadget: function: Remove redundant usb_free_all_descriptors

2014-10-13 Thread Robert Baldyga 
Hi,

Subject:  at the beginning of your email subject looks unnecessary.

When you're sending next version of your patch, you should add subject
prefix [PATCH vN], when N is number of version (eg. [PATCH v2]). See
Documentation/SubmittingPatches.
('git format-patch --subject-prefix' is helpful).

On 10/13/2014 11:35 AM, pavi1729 Sid wrote:
 Removed the usb_free_all_descriptors call in *_bind functions
 as this call is already present in usb_assign_descriptors.
 usb_assign_descriptors is the only call where usb descriptor
 allocation happens, also in case of error freeing of the
 allocated memory takes place in the same call. Hence the
 call in the *_bind functions are redundant.
 Also the presence of usb_free_all_descriptors in the *_bind
 functions might result in double-free corruption of the
 allocated mmeory.

s/mmeory/memory

 
 Signed-off-by: Pavitrakumar Managutte pavitra1...@gmail.com
 ---
  drivers/usb/gadget/function/f_eem.c|  1 -
  drivers/usb/gadget/function/f_hid.c|  7 +++
  drivers/usb/gadget/function/f_ncm.c|  1 -
  drivers/usb/gadget/function/f_phonet.c |  1 -
  drivers/usb/gadget/function/f_rndis.c  |  5 +++--
  drivers/usb/gadget/function/f_subset.c |  1 -
  drivers/usb/gadget/function/f_uac2.c   | 10 ++
  7 files changed, 12 insertions(+), 14 deletions(-)

There is another one usb function which needs to be fixed this way:
drivers/usb/gadget/function/f_obex.c

 
 diff --git a/drivers/usb/gadget/function/f_eem.c
 b/drivers/usb/gadget/function/f_eem.c
 index 4d8b236..c9e90de 100644
 --- a/drivers/usb/gadget/function/f_eem.c
 +++ b/drivers/usb/gadget/function/f_eem.c
 @@ -325,7 +325,6 @@ static int eem_bind(struct usb_configuration *c,
 struct usb_function *f)
  return 0;
 
  fail:
 -usb_free_all_descriptors(f);
  if (eem-port.out_ep)
  eem-port.out_ep-driver_data = NULL;
  if (eem-port.in_ep)
 diff --git a/drivers/usb/gadget/function/f_hid.c
 b/drivers/usb/gadget/function/f_hid.c
 index a95290a..c36de0c 100644
 --- a/drivers/usb/gadget/function/f_hid.c
 +++ b/drivers/usb/gadget/function/f_hid.c
 @@ -621,12 +621,14 @@ static int __init hidg_bind(struct
 usb_configuration *c, struct usb_function *f)
  dev = MKDEV(major, hidg-minor);
  status = cdev_add(hidg-cdev, dev, 1);
  if (status)
 -goto fail;
 +goto fail_free_descs;
 
  device_create(hidg_class, NULL, dev, NULL, %s%d, hidg, hidg-minor);
 
  return 0;
 
 +fail_free_descs:
 +usb_free_all_descriptors(f);
  fail:
  ERROR(f-config-cdev, hidg_bind FAILED\n);
  if (hidg-req != NULL) {
 @@ -635,7 +637,6 @@ fail:
  usb_ep_free_request(hidg-in_ep, hidg-req);
  }
 
 -usb_free_all_descriptors(f);
  return status;
  }
 
 @@ -652,8 +653,6 @@ static void hidg_unbind(struct usb_configuration
 *c, struct usb_function *f)
  kfree(hidg-req-buf);
  usb_ep_free_request(hidg-in_ep, hidg-req);
 
 -usb_free_all_descriptors(f);
 -

Why usb_free_all_descriptors() removed from here?

  kfree(hidg-report_desc);
  kfree(hidg);
  }
 diff --git a/drivers/usb/gadget/function/f_ncm.c
 b/drivers/usb/gadget/function/f_ncm.c
 index bcdc882..38a9279 100644
 --- a/drivers/usb/gadget/function/f_ncm.c
 +++ b/drivers/usb/gadget/function/f_ncm.c
 @@ -1453,7 +1453,6 @@ static int ncm_bind(struct usb_configuration *c,
 struct usb_function *f)
  return 0;
 
  fail:
 -usb_free_all_descriptors(f);
  if (ncm-notify_req) {
  kfree(ncm-notify_req-buf);
  usb_ep_free_request(ncm-notify, ncm-notify_req);
 diff --git a/drivers/usb/gadget/function/f_phonet.c
 b/drivers/usb/gadget/function/f_phonet.c
 index b9cfc15..74ddcac 100644
 --- a/drivers/usb/gadget/function/f_phonet.c
 +++ b/drivers/usb/gadget/function/f_phonet.c
 @@ -571,7 +571,6 @@ err_req:
  for (i = 0; i  phonet_rxq_size  fp-out_reqv[i]; i++)
  usb_ep_free_request(fp-out_ep, fp-out_reqv[i]);
  err:
 -usb_free_all_descriptors(f);

What if usb_ep_alloc_request() fails?
Consider calling usb_free_all_descriptors() under label err_req.

  if (fp-out_ep)
  fp-out_ep-driver_data = NULL;
  if (fp-in_ep)
 diff --git a/drivers/usb/gadget/function/f_rndis.c
 b/drivers/usb/gadget/function/f_rndis.c
 index ddb09dc..2f0517f 100644
 --- a/drivers/usb/gadget/function/f_rndis.c
 +++ b/drivers/usb/gadget/function/f_rndis.c
 @@ -803,7 +803,7 @@ rndis_bind(struct usb_configuration *c, struct
 usb_function *f)
  if (rndis-manufacturer  rndis-vendorID 
  rndis_set_param_vendor(rndis-config, rndis-vendorID,
 rndis-manufacturer))
 -goto fail;
 +goto fail_free_descs;
 
  /* NOTE:  all that is done without knowing or caring about
   * the network link ... which is unavailable to this code
 @@ -817,10 +817,11 @@ rndis_bind(struct usb_configuration *c, struct
 usb_function *f)
  rndis-notify-name);
  return 0;
 
 +fail_free_descs:
 +usb_free_all_descriptors(f);
  fail:

Re: Subject: [PATCH] usb: gadget: function: Remove redundant usb_free_all_descriptors

2014-10-13 Thread pavi1729 Sid 
On Mon, Oct 13, 2014 at 6:55 PM, Robert Baldyga r.bald...@samsung.com wrote:
 Hi,

 Subject:  at the beginning of your email subject looks unnecessary.

 When you're sending next version of your patch, you should add subject
 prefix [PATCH vN], when N is number of version (eg. [PATCH v2]). See
 Documentation/SubmittingPatches.
 ('git format-patch --subject-prefix' is helpful).

 On 10/13/2014 11:35 AM, pavi1729 Sid wrote:
 Removed the usb_free_all_descriptors call in *_bind functions
 as this call is already present in usb_assign_descriptors.
 usb_assign_descriptors is the only call where usb descriptor
 allocation happens, also in case of error freeing of the
 allocated memory takes place in the same call. Hence the
 call in the *_bind functions are redundant.
 Also the presence of usb_free_all_descriptors in the *_bind
 functions might result in double-free corruption of the
 allocated mmeory.

 s/mmeory/memory
Will fix it.


 Signed-off-by: Pavitrakumar Managutte pavitra1...@gmail.com
 ---
  drivers/usb/gadget/function/f_eem.c|  1 -
  drivers/usb/gadget/function/f_hid.c|  7 +++
  drivers/usb/gadget/function/f_ncm.c|  1 -
  drivers/usb/gadget/function/f_phonet.c |  1 -
  drivers/usb/gadget/function/f_rndis.c  |  5 +++--
  drivers/usb/gadget/function/f_subset.c |  1 -
  drivers/usb/gadget/function/f_uac2.c   | 10 ++
  7 files changed, 12 insertions(+), 14 deletions(-)

 There is another one usb function which needs to be fixed this way:
 drivers/usb/gadget/function/f_obex.c
Will fix it.


 diff --git a/drivers/usb/gadget/function/f_eem.c
 b/drivers/usb/gadget/function/f_eem.c
 index 4d8b236..c9e90de 100644
 --- a/drivers/usb/gadget/function/f_eem.c
 +++ b/drivers/usb/gadget/function/f_eem.c
 @@ -325,7 +325,6 @@ static int eem_bind(struct usb_configuration *c,
 struct usb_function *f)
  return 0;

  fail:
 -usb_free_all_descriptors(f);
  if (eem-port.out_ep)
  eem-port.out_ep-driver_data = NULL;
  if (eem-port.in_ep)
 diff --git a/drivers/usb/gadget/function/f_hid.c
 b/drivers/usb/gadget/function/f_hid.c
 index a95290a..c36de0c 100644
 --- a/drivers/usb/gadget/function/f_hid.c
 +++ b/drivers/usb/gadget/function/f_hid.c
 @@ -621,12 +621,14 @@ static int __init hidg_bind(struct
 usb_configuration *c, struct usb_function *f)
  dev = MKDEV(major, hidg-minor);
  status = cdev_add(hidg-cdev, dev, 1);
  if (status)
 -goto fail;
 +goto fail_free_descs;

  device_create(hidg_class, NULL, dev, NULL, %s%d, hidg, hidg-minor);

  return 0;

 +fail_free_descs:
 +usb_free_all_descriptors(f);
  fail:
  ERROR(f-config-cdev, hidg_bind FAILED\n);
  if (hidg-req != NULL) {
 @@ -635,7 +637,6 @@ fail:
  usb_ep_free_request(hidg-in_ep, hidg-req);
  }

 -usb_free_all_descriptors(f);
  return status;
  }

 @@ -652,8 +653,6 @@ static void hidg_unbind(struct usb_configuration
 *c, struct usb_function *f)
  kfree(hidg-req-buf);
  usb_ep_free_request(hidg-in_ep, hidg-req);

 -usb_free_all_descriptors(f);
 -

 Why usb_free_all_descriptors() removed from here?
My bad .. its not supposed to be in unbind function. Will fix it.

  kfree(hidg-report_desc);
  kfree(hidg);
  }
 diff --git a/drivers/usb/gadget/function/f_ncm.c
 b/drivers/usb/gadget/function/f_ncm.c
 index bcdc882..38a9279 100644
 --- a/drivers/usb/gadget/function/f_ncm.c
 +++ b/drivers/usb/gadget/function/f_ncm.c
 @@ -1453,7 +1453,6 @@ static int ncm_bind(struct usb_configuration *c,
 struct usb_function *f)
  return 0;

  fail:
 -usb_free_all_descriptors(f);
  if (ncm-notify_req) {
  kfree(ncm-notify_req-buf);
  usb_ep_free_request(ncm-notify, ncm-notify_req);
 diff --git a/drivers/usb/gadget/function/f_phonet.c
 b/drivers/usb/gadget/function/f_phonet.c
 index b9cfc15..74ddcac 100644
 --- a/drivers/usb/gadget/function/f_phonet.c
 +++ b/drivers/usb/gadget/function/f_phonet.c
 @@ -571,7 +571,6 @@ err_req:
  for (i = 0; i  phonet_rxq_size  fp-out_reqv[i]; i++)
  usb_ep_free_request(fp-out_ep, fp-out_reqv[i]);
  err:
 -usb_free_all_descriptors(f);

 What if usb_ep_alloc_request() fails?
 Consider calling usb_free_all_descriptors() under label err_req.
Agreed. Will fix it.

  if (fp-out_ep)
  fp-out_ep-driver_data = NULL;
  if (fp-in_ep)
 diff --git a/drivers/usb/gadget/function/f_rndis.c
 b/drivers/usb/gadget/function/f_rndis.c
 index ddb09dc..2f0517f 100644
 --- a/drivers/usb/gadget/function/f_rndis.c
 +++ b/drivers/usb/gadget/function/f_rndis.c
 @@ -803,7 +803,7 @@ rndis_bind(struct usb_configuration *c, struct
 usb_function *f)
  if (rndis-manufacturer  rndis-vendorID 
  rndis_set_param_vendor(rndis-config, rndis-vendorID,
 rndis-manufacturer))
 -goto fail;
 +goto fail_free_descs;

  /* NOTE:  all that is done without knowing or caring about
   * the network link ... which is unavailable to this code
 @@ -817,10