linux-kernel - Re: [PATCH] USB: apple-mfi-fastcharge: Use devm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJX_Q+0-9=7q=VcM6uP+8mCX2mVAjS4sT52mvpj-hS6rm63DGg@mail.gmail.com>
Date:   Sun, 15 Nov 2020 08:13:51 +0000
From:   Lucas Tanure <tanure@...ux.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Bastien Nocera <hadess@...ess.net>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify
 the code

On Sat, Nov 14, 2020 at 3:03 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Sat, Nov 14, 2020 at 02:17:48PM +0000, Lucas Tanure wrote:
> > On Sat, Nov 14, 2020 at 12:56 PM Greg Kroah-Hartman
> > <gregkh@...uxfoundation.org> wrote:
> > >
> > > On Sat, Nov 14, 2020 at 12:42:49PM +0000, Lucas Tanure wrote:
> > > > Signed-off-by: Lucas Tanure <tanure@...ux.com>
> > >
> > > I can't take patches without any changelog text, sorry.
> > >
> > > > ---
> > > >  drivers/usb/misc/apple-mfi-fastcharge.c | 17 +++++------------
> > > >  1 file changed, 5 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c b/drivers/usb/misc/apple-mfi-fastcharge.c
> > > > index 9de0171b5177..de86e389a008 100644
> > > > --- a/drivers/usb/misc/apple-mfi-fastcharge.c
> > > > +++ b/drivers/usb/misc/apple-mfi-fastcharge.c
> > > > @@ -178,16 +178,13 @@ static int mfi_fc_probe(struct usb_device *udev)
> > > >  {
> > > >       struct power_supply_config battery_cfg = {};
> > > >       struct mfi_device *mfi = NULL;
> > > > -     int err;
> > > >
> > > >       if (!mfi_fc_match(udev))
> > > >               return -ENODEV;
> > > >
> > > > -     mfi = kzalloc(sizeof(struct mfi_device), GFP_KERNEL);
> > > > -     if (!mfi) {
> > > > -             err = -ENOMEM;
> > > > -             goto error;
> > > > -     }
> > > > +     mfi = devm_kzalloc(&udev->dev, sizeof(*mfi), GFP_KERNEL);
> > > > +     if (!mfi)
> > > > +             return -ENOMEM;
> > > >
> > > >       battery_cfg.drv_data = mfi;
> > > >
> > > > @@ -197,8 +194,7 @@ static int mfi_fc_probe(struct usb_device *udev)
> > > >                                               &battery_cfg);
> > > >       if (IS_ERR(mfi->battery)) {
> > > >               dev_err(&udev->dev, "Can't register battery\n");
> > > > -             err = PTR_ERR(mfi->battery);
> > > > -             goto error;
> > > > +             return PTR_ERR(mfi->battery);
> > > >       }
> > > >
> > > >       mfi->udev = usb_get_dev(udev);
> > > > @@ -206,9 +202,6 @@ static int mfi_fc_probe(struct usb_device *udev)
> > > >
> > > >       return 0;
> > > >
> > > > -error:
> > > > -     kfree(mfi);
> > > > -     return err;
> > > >  }
> > > >
> > > >  static void mfi_fc_disconnect(struct usb_device *udev)
> > > > @@ -220,7 +213,7 @@ static void mfi_fc_disconnect(struct usb_device *udev)
> > > >               power_supply_unregister(mfi->battery);
> > > >       dev_set_drvdata(&udev->dev, NULL);
> > > >       usb_put_dev(mfi->udev);
> > > > -     kfree(mfi);
> > > > +     devm_kfree(&udev->dev, mfi);
> > >
> > > Are you sure about this?
> > I think so, as the probe will allocate again that struct, the
> > disconnect should free the previous one.
>
> Why do you need to manually free it here like this?
My understanding is that memory will only be freed when the driver
gets unloaded and the next connection of the device will allocate a
new one.
So every new disconnection and re-connection there will be a small
memory leak until the driver gets unloaded.

>
> Why are you trying to convert this file to this api anyway?
I was just trying to improve the code as the original source calls
kfree even when kzalloc fails.
And using devm_* would remove the need for kfree and the end of probe.

>
> thanks,
>
> greg k-h

Thanks
Lucas