| Message ID | e437e728317b6a2a860f7812f64a98146a27965e.1695844349.git.jahau@rocketmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Remaining patches for RT5033 charger | expand |
Ok, but why not already in patch #1? CJ Le 27/09/2023 à 22:26, Jakob Hauser a écrit : > From: Yang Yingliang <yangyingliang@huawei.com> > > 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 <yangyingliang@huawei.com> > Signed-off-by: Jakob Hauser <jahau@rocketmail.com> > --- > 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
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[] = {