linux-kernel - Re: [PATCH v3 6/8] power: supply: Clean-up few drivers by using managed work init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e85a49272e734fc0dc1b7a9da66d6977516a47a.camel@fi.rohmeurope.com>
Date:   Wed, 24 Mar 2021 09:52:14 +0200
From:   Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
To:     wens@...e.org
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Sebastian Reichel <sre@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "open list:THERMAL" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v3 6/8] power: supply: Clean-up few drivers by using
 managed work init


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@...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@...rohmeurope.com>
> > Acked-by: Sebastian Reichel <sebastian.reichel@...labora.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