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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 13, 2014 at 6:55 PM, Robert Baldyga <r.baldyga@xxxxxxxxxxx> 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 <pavitra1729@xxxxxxxxx>
>> ---
>>  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 +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 100644
>> --- a/drivers/usb/gadget/function/f_uac2.c
>> +++ b/drivers/usb/gadget/function/f_uac2.c
>> @@ -994,7 +994,7 @@ afunc_bind(struct usb_configuration *cfg, struct
>> usb_function *fn)
>>      prm->rbuf = kzalloc(prm->max_psize * USB_XFERS, GFP_KERNEL);
>>      if (!prm->rbuf) {
>>          prm->max_psize = 0;
>> -        goto err;
>> +        goto err_free_descs;
>>      }
>>
>>      prm = &agdev->uac2.p_prm;
>> @@ -1002,17 +1002,19 @@ afunc_bind(struct usb_configuration *cfg,
>> struct usb_function *fn)
>>      prm->rbuf = kzalloc(prm->max_psize * USB_XFERS, GFP_KERNEL);
>>      if (!prm->rbuf) {
>>          prm->max_psize = 0;
>> -        goto err;
>> +        goto err_free_descs;
>>      }
>>
>>      ret = alsa_uac2_init(agdev);
>>      if (ret)
>> -        goto err;
>> +        goto err_free_descs;
>>      return 0;
>> +
>> +err_free_descs:
>> +    usb_free_all_descriptors(fn);
>>  err:
>>      kfree(agdev->uac2.p_prm.rbuf);
>>      kfree(agdev->uac2.c_prm.rbuf);
>> -    usb_free_all_descriptors(fn);
>>      if (agdev->in_ep)
>>          agdev->in_ep->driver_data = NULL;
>>      if (agdev->out_ep)
>>
>
> Thanks,
> Robert Baldyga
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux