Re: [PATCH] USB: Gadget: Composite: Added error handling codes toprevent a memory leak case when the configuration's bind function failed

[Yongsul Oh]

Dear Michal Nazarewicz

Thank you very much for your kindness.
And i also agree with your comments.
I wil try to change my commit-msg & re-send it as soon as possible.

And about your last commets,

> Also, like written previously, I think there might be memory leak in 
other error
> recovery paths as well.  If you could take a look whether I'm right, 
that would be
> awesome.

I will try to find out if the memory leaks in other error recovery paths 
or not,
 but i think that might be another case and need different patch-set.
(If i find out that, i will try to make another patch-set and send it 
all again)

Best regards.
Yongsul Oh

On 2012ë 03ì 16ì 20:14, Michal Nazarewicz wrote:
> On Fri, 16 Mar 2012 07:27:15 +0100, Yongsul Oh 
> <yongsul96.oh@xxxxxxxxxxx> wrote:
>
> >  In some usb gadget driver, (for example usb gadget serial 
> > driver(serial.c),
>
> s/usb/USB/g
> s/gadget driver/composite gadget drivers/
>
> I also don't think the list of examples is necessary here.  Maybe just 
> a list file
> names? My personal opinion, others may disagree though.
>
> Also, comma should go after the closing paren.
>
> > multifunction composite driver(multi,c), nokia composite gadget 
> > driver(nokia.c),
> > HID composite driver(hid.c), CDC composite driver(cdc2.c)..) the 
> > configuration's
> > bind function by called the 'usb_add_config()' has multiple bind 
> > config functions
>
> s/by called the/called by the/
> s/has/calls/
>
> > for each functionality. (for example cdc2 configuration bind function 
> > -'cdc_do_config()'
>
> s/functions for each functionality/functions, one for each USB 
> functionality/
>
> > has two functionality bind config functions -'ecm_bind_config()' & 
> > 'acm_bind_config()'
> > in CDC composite driver.)
> >
> >  In each functionality bind config function, new instance for each 
> > functionality is
>
> s/for each functionality//
>
> > allocated & initialized by 'kzalloc()' and finally the new instance 
> > is added by
>
> s/& initialized by 'kzalloc()' and finally the new instance/and/
>
> > 'usb_add_function()'. After 'usb_add_function' state, already created 
> > the instance
> > is only handled by it's configuration & freed from functionality 
> > unbind function.
>
> I'm not sure what you meant by this last sentence.
>
> >  So, If an error occurred during the second functionality bind config 
> > state,
>
> s/If/if/
> s/state//
>
> > (for example an error occurred at 'acm_bind_config()' after 
> > succeeding of
> > 'ecm_bind_function()') the created instance by 'acm_bind_config()'
>
> s/the created instance by 'acm_bind_config()'/the instance created by 
> acm_bind_config()/
>
> > cannot be freed. And it makes memory leak situation.
>
> s/freed. And it makes memory leak situation./freed creating a memory 
> leak./
>
> > This patch fixes this issue
>
> s/issue/issue./
>
> Also, drop apostrophes around function names.  It looks weird.
>
> > Signed-off-by: Yongsul Oh <yongsul96.oh@xxxxxxxxxxx>
>
> Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
>
> Also, like written previously, I think there might be memory leak in 
> other error
> recovery paths as well.  If you could take a look whether I'm right, 
> that would be
> awesome.
>
> > ---
> >  drivers/usb/gadget/composite.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/composite.c 
> > b/drivers/usb/gadget/composite.c
> > index baaebf2..4cb1801 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -737,6 +737,19 @@ int usb_add_config(struct usb_composite_dev *cdev,
> >     status = bind(config);
> >      if (status < 0) {
> > +        while (!list_empty(&config->functions)) {
> > +            struct usb_function        *f;
> > +
> > +            f = list_first_entry(&config->functions,
> > +                    struct usb_function, list);
> > +            list_del(&f->list);
> > +            if (f->unbind) {
> > +                DBG(cdev, "unbind function '%s'/%p\n",
> > +                    f->name, f);
> > +                f->unbind(config, f);
> > +                /* may free memory for "f" */
> > +            }
> > +        }
> >          list_del(&config->list);
> >          config->cdev = NULL;
> >      } else {

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/