[PATCH] USB: apple-mfi-fastcharge: fix reference leak in apple_mfi_fc_set_property

[PATCH] USB: apple-mfi-fastcharge: fix reference leak in apple_mfi_fc_set_property

[Zhang Qilong]

pm_runtime_get_sync() will increment pm usage counter even
it failed. Forgetting to call pm_runtime_put_noidle will
result in reference leak in apple_mfi_fc_set_property, so
we should fix it.

Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
 drivers/usb/misc/apple-mfi-fastcharge.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c b/drivers/usb/misc/apple-mfi-fastcharge.c
index b403094a6b3a..9e1ad4536e36 100644
--- a/drivers/usb/misc/apple-mfi-fastcharge.c
+++ b/drivers/usb/misc/apple-mfi-fastcharge.c
@@ -120,8 +120,10 @@ static int apple_mfi_fc_set_property(struct power_supply *psy,
 	dev_dbg(&mfi->udev->dev, "prop: %d\n", psp);
 
 	ret = pm_runtime_get_sync(&mfi->udev->dev);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put_noidle(&mfi->udev->dev);
 		return ret;
+	}
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-- 
2.17.1

Re: [PATCH] USB: apple-mfi-fastcharge: fix reference leak in apple_mfi_fc_set_property

[Bastien Nocera]

On Fri, 2020-10-30 at 23:45 +0800, Zhang Qilong wrote:
> pm_runtime_get_sync() will increment pm usage counter even
> it failed. Forgetting to call pm_runtime_put_noidle will
> result in reference leak in apple_mfi_fc_set_property, so
> we should fix it.
> 
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>

It looks correct, but I don't know whether it's necessary.
There's a boatload of users of that API that don't even check for the
get_sync() retval, and loads more where it's checked but never acted
upon.

Do you intend to fix all those as well?

> ---
>  drivers/usb/misc/apple-mfi-fastcharge.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c
> b/drivers/usb/misc/apple-mfi-fastcharge.c
> index b403094a6b3a..9e1ad4536e36 100644
> --- a/drivers/usb/misc/apple-mfi-fastcharge.c
> +++ b/drivers/usb/misc/apple-mfi-fastcharge.c
> @@ -120,8 +120,10 @@ static int apple_mfi_fc_set_property(struct
> power_supply *psy,
>         dev_dbg(&mfi->udev->dev, "prop: %d\n", psp);
>  
>         ret = pm_runtime_get_sync(&mfi->udev->dev);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               pm_runtime_put_noidle(&mfi->udev->dev);
>                 return ret;
> +       }
>  
>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:

Re: [PATCH] USB: apple-mfi-fastcharge: fix reference leak in apple_mfi_fc_set_property

[Sergei Shtylyov]

On 10/30/20 6:45 PM, Zhang Qilong wrote:

> pm_runtime_get_sync() will increment pm usage counter even

   You missed when/if in this (and the following) patch.

> it failed. Forgetting to call pm_runtime_put_noidle will
> result in reference leak in apple_mfi_fc_set_property, so
> we should fix it.
> 
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
[...]

MBR, Sergei

答复: [PATCH] USB: apple-mfi-fastcharge: fix reference leak in apple_mfi_fc_set_property

[zhangqilong]

> 
> On Fri, 2020-10-30 at 23:45 +0800, Zhang Qilong wrote:
> > pm_runtime_get_sync() will increment pm usage counter even it failed.
> > Forgetting to call pm_runtime_put_noidle will result in reference leak
> > in apple_mfi_fc_set_property, so we should fix it.
> >
> > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> 
> It looks correct, but I don't know whether it's necessary.
> There's a boatload of users of that API that don't even check for the
> get_sync() retval, and loads more where it's checked but never acted upon.
> 
> Do you intend to fix all those as well?
> 

pm_runtime_get_sync will increase the reference count at first, and it will resume the device
later. If runtime of the device is active or has error(something else....), resume will failed. If we
do not call put operation to decrease the reference, the result is that this device cannot enter
the idle state and always stay busy or other non-idle state, maybe it consumes more power or
raise additional errors... And I'll try to fix them : )

Thanks, best wish!

Zhang Qilong

> > ---
> >  drivers/usb/misc/apple-mfi-fastcharge.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c
> > b/drivers/usb/misc/apple-mfi-fastcharge.c
> > index b403094a6b3a..9e1ad4536e36 100644
> > --- a/drivers/usb/misc/apple-mfi-fastcharge.c
> > +++ b/drivers/usb/misc/apple-mfi-fastcharge.c
> > @@ -120,8 +120,10 @@ static int apple_mfi_fc_set_property(struct
> > power_supply *psy,
> >         dev_dbg(&mfi->udev->dev, "prop: %d\n", psp);
> >
> >         ret = pm_runtime_get_sync(&mfi->udev->dev);
> > -       if (ret < 0)
> > +       if (ret < 0) {
> > +               pm_runtime_put_noidle(&mfi->udev->dev);
> >                 return ret;
> > +       }
> >
> >         switch (psp) {
> >         case POWER_SUPPLY_PROP_CHARGE_TYPE:
>