* [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify the code
@ 2020-11-14 12:42 Lucas Tanure
2020-11-14 12:56 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Lucas Tanure @ 2020-11-14 12:42 UTC (permalink / raw)
To: Bastien Nocera, Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel, Lucas Tanure
Signed-off-by: Lucas Tanure <tanure@linux.com>
---
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);
}
static struct usb_device_driver mfi_fc_driver = {
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify the code
2020-11-14 12:42 [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify the code Lucas Tanure
@ 2020-11-14 12:56 ` Greg Kroah-Hartman
2020-11-14 14:17 ` Lucas Tanure
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-14 12:56 UTC (permalink / raw)
To: Lucas Tanure; +Cc: Bastien Nocera, linux-usb, linux-kernel
On Sat, Nov 14, 2020 at 12:42:49PM +0000, Lucas Tanure wrote:
> Signed-off-by: Lucas Tanure <tanure@linux.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?
And what's wrong with the existing code? Using the devm_*() variants
seems like a "cleanup", but it's not always the case.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify the code
2020-11-14 12:56 ` Greg Kroah-Hartman
@ 2020-11-14 14:17 ` Lucas Tanure
2020-11-14 15:02 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Lucas Tanure @ 2020-11-14 14:17 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Bastien Nocera, linux-usb, linux-kernel
On Sat, Nov 14, 2020 at 12:56 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Nov 14, 2020 at 12:42:49PM +0000, Lucas Tanure wrote:
> > Signed-off-by: Lucas Tanure <tanure@linux.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.
>
> And what's wrong with the existing code? Using the devm_*() variants
> seems like a "cleanup", but it's not always the case.
I don't know what's wrong, but I will figure out.
>
> thanks,
>
> greg k-h
Thanks
Lucas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify the code
2020-11-14 14:17 ` Lucas Tanure
@ 2020-11-14 15:02 ` Greg Kroah-Hartman
2020-11-15 8:13 ` Lucas Tanure
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-14 15:02 UTC (permalink / raw)
To: Lucas Tanure; +Cc: Bastien Nocera, linux-usb, linux-kernel
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@linuxfoundation.org> wrote:
> >
> > On Sat, Nov 14, 2020 at 12:42:49PM +0000, Lucas Tanure wrote:
> > > Signed-off-by: Lucas Tanure <tanure@linux.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?
Why are you trying to convert this file to this api anyway?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify the code
2020-11-14 15:02 ` Greg Kroah-Hartman
@ 2020-11-15 8:13 ` Lucas Tanure
2020-11-15 8:26 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Lucas Tanure @ 2020-11-15 8:13 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Bastien Nocera, linux-usb, linux-kernel
On Sat, Nov 14, 2020 at 3:03 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.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@linuxfoundation.org> wrote:
> > >
> > > On Sat, Nov 14, 2020 at 12:42:49PM +0000, Lucas Tanure wrote:
> > > > Signed-off-by: Lucas Tanure <tanure@linux.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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify the code
2020-11-15 8:13 ` Lucas Tanure
@ 2020-11-15 8:26 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-15 8:26 UTC (permalink / raw)
To: Lucas Tanure; +Cc: Bastien Nocera, linux-usb, linux-kernel
On Sun, Nov 15, 2020 at 08:13:51AM +0000, Lucas Tanure wrote:
> On Sat, Nov 14, 2020 at 3:03 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.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@linuxfoundation.org> wrote:
> > > >
> > > > On Sat, Nov 14, 2020 at 12:42:49PM +0000, Lucas Tanure wrote:
> > > > > Signed-off-by: Lucas Tanure <tanure@linux.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.
devm_* functions operate on the lifecycle of the device, not the driver.
Two totally different things :)
> > 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.
Then please fix that bug independant of any conversion to a new api, as
that is a bugfix that should be backported to older kernels.
> And using devm_* would remove the need for kfree and the end of probe.
Yes, but you can't introduce new bugs when trying to fix existing ones.
Also, you didn't say this was a bugfix anywhere, which is one reason
that writing good changelog text is essencial.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-15 8:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-14 12:42 [PATCH] USB: apple-mfi-fastcharge: Use devm_kzalloc and simplify the code Lucas Tanure
2020-11-14 12:56 ` Greg Kroah-Hartman
2020-11-14 14:17 ` Lucas Tanure
2020-11-14 15:02 ` Greg Kroah-Hartman
2020-11-15 8:13 ` Lucas Tanure
2020-11-15 8:26 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).