Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <4e85a49272e734fc0dc1b7a9da66d6977516a47a.camel@fi.rohmeurope.com>
Subject: Re: [PATCH v3 6/8] power: supply: Clean-up few drivers by using
 managed work init
From:   Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Reply-To: matti.vaittinen@fi.rohmeurope.com
To:     wens@csie.org
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Sebastian Reichel <sre@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        "open list:THERMAL" <linux-pm@vger.kernel.org>
In-Reply-To: <CAGb2v67Jd6qFS-zmD+Hm4BJHA+-kx0nAxvDovUwW=WwZTEGYeg@mail.gmail.com>
References: <cover.1616506559.git.matti.vaittinen@fi.rohmeurope.com>
         <e5b1b0380cdd1aa066c9ac6d7a8b1a86ba1ddbbe.1616506559.git.matti.vaittinen@fi.rohmeurope.com>
         <CAGb2v67Jd6qFS-zmD+Hm4BJHA+-kx0nAxvDovUwW=WwZTEGYeg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Date:   Wed, 24 Mar 2021 09:52:14 +0200
User-Agent: Evolution 3.34.4 (3.34.4-1.fc31) 
Content-Transfer-Encoding: 7bit
Precedence: bulk


On Tue, 2021-03-23 at 22:36 +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Tue, Mar 23, 2021 at 9:58 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> > Few drivers implement remove call-back only for ensuring a delayed
> > work gets cancelled prior driver removal. Clean-up these by
> > switching
> > to use devm_delayed_work_autocancel() instead.
> > 
> > This change is compile-tested only. All testing is appreciated.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> > Changelog from RFCv2:
> >  - RFC dropped. No functional changes.
> > 
> >  drivers/power/supply/axp20x_usb_power.c      | 15 +++++----------
> >  drivers/power/supply/bq24735-charger.c       | 18 ++++++--------
> > ----
> >  drivers/power/supply/ltc2941-battery-gauge.c | 20 +++++++---------
> > ----
> >  drivers/power/supply/sbs-battery.c           | 16 +++++-----------
> >  4 files changed, 23 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/power/supply/axp20x_usb_power.c
> > b/drivers/power/supply/axp20x_usb_power.c
> > index 8933ae26c3d6..4259709e3491 100644
> > --- a/drivers/power/supply/axp20x_usb_power.c
> > +++ b/drivers/power/supply/axp20x_usb_power.c
> > @@ -8,6 +8,7 @@
> > 
> >  #include <linux/bitops.h>
> >  #include <linux/device.h>
> > +#include <linux/devm-helpers.h>
> >  #include <linux/init.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/kernel.h>
> > @@ -646,21 +647,16 @@ static int axp20x_usb_power_probe(struct
> > platform_device *pdev)
> >                 }
> >         }
> > 
> > +       ret = devm_delayed_work_autocancel(&pdev->dev, &power-
> > >vbus_detect,
> > +                                          axp20x_usb_power_poll_vb
> > us);
> > +       if (ret)
> > +               return ret;
> 
> This doesn't look right. The IRQ is requested before this, and the
> delayed_work
> struct is initialized even earlier, so you'd be re-initializing the
> struct,
> with the work item potentially running or queued up already.

I checked this and you are 100% correct.

b5e8642ed95ff6ecc20cc6038fe831affa9d098c
"power: supply: axp20x_usb_power: Init work before enabling IRQs" had
fixed the order between RFCv1 and the patch v3. This is what one gets
when not being careful with rebase. Thanks again for the heads-up! I'll
send follow-up fix still today.

Br,
	Matti Vaittinen