linux-kernel - Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c

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: <CAMdYzYq=2G1bvu9M_DR0vdQDtcvVBhQFRPTNKo1ySFrMZOk=jg@mail.gmail.com>
Date:   Fri, 17 Dec 2021 13:16:05 -0500
From:   Peter Geis <pgwipeout@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Lee Jones <lee.jones@...aro.org>,
        Robin Murphy <robin.murphy@....com>,
        Heiko Stuebner <heiko@...ech.de>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Nicolas Frattaroli <frattaroli.nicolas@...il.com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c

On Fri, Dec 17, 2021 at 10:29 AM Dmitry Osipenko <digetx@...il.com> wrote:
>
> 17.12.2021 17:55, Peter Geis пишет:
> > This adds reboot support to the rk808o pmic driver and enables it for
> > the rk809 and rk817 devices.
> > This only enables if the rockchip,system-power-controller flag is set.
> >
> > Signed-off-by: Peter Geis <pgwipeout@...il.com>
> > Signed-off-by: Frank Wunderlich <frank-w@...lic-files.de>
> > ---
> > This patch was created to address issues with psci-reset on rk356x
> > chips. Until the rk356x series ATF open source code is released so we
> > can fix the issue at the source, this is the only way to ensure reliable
> > resetting on devices using these chips.
> >
> > After testing the rk808 (thanks Robin!), it was found DEV_OFF_RST does
> > not reset the PMIC to a power on state. Since the rk805 and rk818 match
> > this register layout, I'm removing support for all three in the v2.
> > It may be possible to add support to them using an RTC wakeup, but that
> > has not been explored and is outside the scope of this patch.
> >
> > Changelog:
> > V2:
> > - Squash the patch from Frank Wunderlich for rk809 support.
> > - Remove support for the rk805, rk808, and rk818 devices.
> > - Only register the reset handler for supported devices.
> > - Remove unnecessary dev_err and dev_warn statements.
> > - Register the reset handler directly
> >
> >  drivers/mfd/rk808.c       | 43 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/rk808.h |  1 +
> >  2 files changed, 44 insertions(+)
> ...
> > +static struct notifier_block rk808_restart_handler = {
> > +     .notifier_call = rk808_restart_notify,
> > +     .priority = 255,
> > +};
>
> Hello!
>
> Please use the default 128 priority if there are no other conflicting
> handlers on this RK.

Unfortunately the psci-reboot handler is set to 129, I'll adjust this
to 192 which is in line with other PMIC reboot drivers.

>
> >  static void rk8xx_shutdown(struct i2c_client *client)
> >  {
> >       struct rk808 *rk808 = i2c_get_clientdata(client);
> > @@ -727,6 +757,19 @@ static int rk808_probe(struct i2c_client *client,
> >       if (of_property_read_bool(np, "rockchip,system-power-controller")) {
> >               rk808_i2c_client = client;
> >               pm_power_off = rk808_pm_power_off;
> > +
> > +             switch (rk808->variant) {
> > +             case RK809_ID:
> > +             case RK817_ID:
> > +                     ret = register_restart_handler(&rk808_restart_handler);
>
> There is no corresponding unregister_restart_handler(), which you should
> add to rk808_remove(). Otherwise kernel will crash on reboot if you'll
> unload this driver module.

Thanks, added!

>
> > +                     break;
> > +             default:
> > +                     dev_info(&client->dev, "pmic controlled board reset not supported\n");
>
> I'd set ret=0 explicitly here. Later on somebody may change the code and
> ret won't be zero anymore, this is not an uncommon trouble in kernel.

It took me a moment to see the logic here, but I understand it now.

>
> > +                     break;
> > +             }
> > +
> > +             if (ret)
> > +                     dev_err(&client->dev, "failed to register restart handler, %d\n", ret);
>
>
>

Thank you for the review.