From: Yang Yingliang <[email protected]>
Fix missing mutex_unlock() in some error path.
Fixes: 12cc585f36b8 ("power: supply: rt5033_charger: Add cable detection and USB OTG supply")
Signed-off-by: Yang Yingliang <[email protected]>
Signed-off-by: Jakob Hauser <[email protected]>
---
drivers/power/supply/rt5033_charger.c | 28 ++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/power/supply/rt5033_charger.c b/drivers/power/supply/rt5033_charger.c
index 2c2073b8979d..091ca4a21f29 100644
--- a/drivers/power/supply/rt5033_charger.c
+++ b/drivers/power/supply/rt5033_charger.c
@@ -361,7 +361,8 @@ static int rt5033_charger_set_otg(struct rt5033_charger *charger)
0x37 << RT5033_CHGCTRL2_CV_SHIFT);
if (ret) {
dev_err(charger->dev, "Failed set OTG boost v_out\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_unlock;
}
/* Set operation mode to OTG */
@@ -369,7 +370,8 @@ static int rt5033_charger_set_otg(struct rt5033_charger *charger)
RT5033_CHGCTRL1_MODE_MASK, RT5033_BOOST_MODE);
if (ret) {
dev_err(charger->dev, "Failed to update OTG mode.\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_unlock;
}
/* In case someone switched from charging to OTG directly */
@@ -378,9 +380,10 @@ static int rt5033_charger_set_otg(struct rt5033_charger *charger)
charger->otg = true;
+out_unlock:
mutex_unlock(&charger->lock);
- return 0;
+ return ret;
}
static int rt5033_charger_unset_otg(struct rt5033_charger *charger)
@@ -420,8 +423,10 @@ static int rt5033_charger_set_charging(struct rt5033_charger *charger)
/* In case someone switched from OTG to charging directly */
if (charger->otg) {
ret = rt5033_charger_unset_otg(charger);
- if (ret)
+ if (ret) {
+ mutex_unlock(&charger->lock);
return -EINVAL;
+ }
}
charger->online = true;
@@ -448,6 +453,7 @@ static int rt5033_charger_set_mivr(struct rt5033_charger *charger)
RT5033_CHGCTRL4_MIVR_MASK, RT5033_CHARGER_MIVR_4600MV);
if (ret) {
dev_err(charger->dev, "Failed to set MIVR level.\n");
+ mutex_unlock(&charger->lock);
return -EINVAL;
}
@@ -463,7 +469,7 @@ static int rt5033_charger_set_mivr(struct rt5033_charger *charger)
static int rt5033_charger_set_disconnect(struct rt5033_charger *charger)
{
- int ret;
+ int ret = 0;
mutex_lock(&charger->lock);
@@ -475,7 +481,8 @@ static int rt5033_charger_set_disconnect(struct rt5033_charger *charger)
RT5033_CHARGER_MIVR_DISABLE);
if (ret) {
dev_err(charger->dev, "Failed to disable MIVR.\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_unlock;
}
charger->mivr_enabled = false;
@@ -483,16 +490,19 @@ static int rt5033_charger_set_disconnect(struct rt5033_charger *charger)
if (charger->otg) {
ret = rt5033_charger_unset_otg(charger);
- if (ret)
- return -EINVAL;
+ if (ret) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
}
if (charger->online)
charger->online = false;
+out_unlock:
mutex_unlock(&charger->lock);
- return 0;
+ return ret;
}
static enum power_supply_property rt5033_charger_props[] = {
--
2.39.2
Ok, but why not already in patch #1?
CJ
Le 27/09/2023 à 22:26, Jakob Hauser a écrit :
> From: Yang Yingliang <[email protected]>
>
> Fix missing mutex_unlock() in some error path.
>
> Fixes: 12cc585f36b8 ("power: supply: rt5033_charger: Add cable detection and USB OTG supply")
> Signed-off-by: Yang Yingliang <[email protected]>
> Signed-off-by: Jakob Hauser <[email protected]>
> ---
> drivers/power/supply/rt5033_charger.c | 28 ++++++++++++++++++---------
> 1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/power/supply/rt5033_charger.c b/drivers/power/supply/rt5033_charger.c
> index 2c2073b8979d..091ca4a21f29 100644
> --- a/drivers/power/supply/rt5033_charger.c
> +++ b/drivers/power/supply/rt5033_charger.c
> @@ -361,7 +361,8 @@ static int rt5033_charger_set_otg(struct rt5033_charger *charger)
> 0x37 << RT5033_CHGCTRL2_CV_SHIFT);
> if (ret) {
> dev_err(charger->dev, "Failed set OTG boost v_out\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;
> }
>
> /* Set operation mode to OTG */
> @@ -369,7 +370,8 @@ static int rt5033_charger_set_otg(struct rt5033_charger *charger)
> RT5033_CHGCTRL1_MODE_MASK, RT5033_BOOST_MODE);
> if (ret) {
> dev_err(charger->dev, "Failed to update OTG mode.\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;
> }
>
> /* In case someone switched from charging to OTG directly */
> @@ -378,9 +380,10 @@ static int rt5033_charger_set_otg(struct rt5033_charger *charger)
>
> charger->otg = true;
>
> +out_unlock:
> mutex_unlock(&charger->lock);
>
> - return 0;
> + return ret;
> }
>
> static int rt5033_charger_unset_otg(struct rt5033_charger *charger)
> @@ -420,8 +423,10 @@ static int rt5033_charger_set_charging(struct rt5033_charger *charger)
> /* In case someone switched from OTG to charging directly */
> if (charger->otg) {
> ret = rt5033_charger_unset_otg(charger);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&charger->lock);
> return -EINVAL;
> + }
> }
>
> charger->online = true;
> @@ -448,6 +453,7 @@ static int rt5033_charger_set_mivr(struct rt5033_charger *charger)
> RT5033_CHGCTRL4_MIVR_MASK, RT5033_CHARGER_MIVR_4600MV);
> if (ret) {
> dev_err(charger->dev, "Failed to set MIVR level.\n");
> + mutex_unlock(&charger->lock);
> return -EINVAL;
> }
>
> @@ -463,7 +469,7 @@ static int rt5033_charger_set_mivr(struct rt5033_charger *charger)
>
> static int rt5033_charger_set_disconnect(struct rt5033_charger *charger)
> {
> - int ret;
> + int ret = 0;
>
> mutex_lock(&charger->lock);
>
> @@ -475,7 +481,8 @@ static int rt5033_charger_set_disconnect(struct rt5033_charger *charger)
> RT5033_CHARGER_MIVR_DISABLE);
> if (ret) {
> dev_err(charger->dev, "Failed to disable MIVR.\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;
> }
>
> charger->mivr_enabled = false;
> @@ -483,16 +490,19 @@ static int rt5033_charger_set_disconnect(struct rt5033_charger *charger)
>
> if (charger->otg) {
> ret = rt5033_charger_unset_otg(charger);
> - if (ret)
> - return -EINVAL;
> + if (ret) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> }
>
> if (charger->online)
> charger->online = false;
>
> +out_unlock:
> mutex_unlock(&charger->lock);
>
> - return 0;
> + return ret;
> }
>
> static enum power_supply_property rt5033_charger_props[] = {
Hi Christophe,
On 28.09.23 07:23, Marion & Christophe JAILLET wrote:
> Ok, but why not already in patch #1?
Thanks for your hints about the missing "unlock"s. And sorry for causing
you extra work by having the fix in a separate patch.
The patch you refer to ("power: supply: rt5033_charger: Add cable
detection and USB OTG supply") has its own history. It was already
applied once, showed up in linux-next, caused some issues, was therefore
removed again. In the meantime, some fixes were provided by different
contributors.
This series actually tries to apply that patch again, accompanied by two
fixes – and two additional clean-up patches. I added the fixes patches
as-is, also to credit the contributors.
Possibly the cover sheet of the series was a bit too thin about that
history.
Kind regards,
Jakob
Hi,
On Thu, Sep 28, 2023 at 09:46:33PM +0200, Jakob Hauser wrote:
> On 28.09.23 07:23, Marion & Christophe JAILLET wrote:
> > Ok, but why not already in patch #1?
>
> Thanks for your hints about the missing "unlock"s. And sorry for causing you
> extra work by having the fix in a separate patch.
>
> The patch you refer to ("power: supply: rt5033_charger: Add cable detection
> and USB OTG supply") has its own history. It was already applied once,
> showed up in linux-next, caused some issues, was therefore removed again. In
> the meantime, some fixes were provided by different contributors.
Since the commit has been dropped, please merge the fixes into the
patch. E.g. patch #1 does not make any sense on its own.
> This series actually tries to apply that patch again, accompanied by two
> fixes – and two additional clean-up patches. I added the fixes patches
> as-is, also to credit the contributors.
You can use Co-developed-by tag for that.
> Possibly the cover sheet of the series was a bit too thin about that
> history.
>
> Kind regards,
> Jakob
Patches 4-5 look fine to me, but do not apply without 1-3. For 1-3 I
did not check them in detail. Please merge them first, since it's
quite hard to read in the current state.
Thanks,
-- Sebastian