linux-kernel - Re: [PATCH v5 1/3] thermal: qcom-spmi: Use PMIC thermal stage 2 for critical trip points

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAD=FV=VODGOE=qvJVeLGBs8xXQvbHZu7S9rqSpMY4AetUjd3fQ@mail.gmail.com>
Date:   Wed, 25 Jul 2018 16:19:56 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Zhang Rui <rui.zhang@...el.com>,
        Eduardo Valentin <edubezval@...il.com>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>, devicetree@...r.kernel.org,
        linux-pm@...r.kernel.org, David Collins <collinsd@...eaurora.org>,
        Stephen Boyd <sboyd@...nel.org>
Subject: Re: [PATCH v5 1/3] thermal: qcom-spmi: Use PMIC thermal stage 2 for
 critical trip points

Hi,

On Tue, Jul 24, 2018 at 4:46 PM, Matthias Kaehlcke <mka@...omium.org> wrote:
> +static int qpnp_tm_update_critical_trip_temp(struct qpnp_tm_chip *chip,
> +                                            int temp)
> +{
> +       u8 reg;
> +       bool disable_s2_shutdown = false;
> +       int ret;
> +
> +       WARN_ON(!mutex_is_locked(&chip->lock));
> +
> +       /*
> +        * Default: S2 and S3 shutdown enabled, thresholds at
> +        * 105C/125C/145C, monitoring at 25Hz
> +        */
> +       reg = SHUTDOWN_CTRL1_RATE_25HZ;
> +
> +       if ((temp == THERMAL_TEMP_INVALID) ||
> +           (temp < STAGE2_THRESHOLD_MIN)) {
> +               chip->thresh = THRESH_MIN;
> +               goto skip;
> +       }
> +
> +       if (temp <= STAGE2_THRESHOLD_MAX) {
> +               chip->thresh = THRESH_MAX -
> +                       ((STAGE2_THRESHOLD_MAX - temp) /
> +                        TEMP_THRESH_STEP);
> +               disable_s2_shutdown = true;
> +       } else {
> +               chip->thresh = THRESH_MAX;
> +
> +               if (!IS_ERR(chip->adc))
> +                       disable_s2_shutdown = true;
> +               else
> +                       dev_warn(chip->dev,
> +                                "No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140°C! Configuring stage 2 shutdown at 140°C.\n");

Putting a non-ASCII character (the degree symbol) in your commit
message is one thing, but are you sure it's wise to put it in the
kernel logs?


> +       }
> +
> +skip:
> +       reg |= chip->thresh;
> +       if (disable_s2_shutdown)
> +               reg |= SHUTDOWN_CTRL1_OVERRIDE_S2;
> +
> +       ret = qpnp_tm_write(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, reg);
> +       if (ret < 0)
> +               return ret;
> +
> +       return ret;

Simplify the above lines to:

return qpnp_tm_write(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, reg);


> @@ -313,12 +441,7 @@ static int qpnp_tm_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 return ret;
>
> -       chip->tz_dev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, chip,
> -                                                       &qpnp_tm_sensor_ops);
> -       if (IS_ERR(chip->tz_dev)) {
> -               dev_err(&pdev->dev, "failed to register sensor\n");
> -               return PTR_ERR(chip->tz_dev);
> -       }
> +       chip->initialized = true;

Should we add "thermal_zone_device_update(chip->tz_dev,
THERMAL_EVENT_UNSPECIFIED);" here

...also: do we care about any type of locking for chip->initialized?
Technically we can be running on weakly ordered memory so if
qpnp_tm_update_temp_no_adc() is running on a different processor then
possibly it could still keep returning the default temperature for a
little while.  We could try to analyze whether there's some sort of
implicit barrier or we could add manual memory barriers, but generally
I try to avoid that and just do the simple locking...  What about just
setting chip-Initialized = true at the end of qpnp_tm_init() while the
mutex is still held?


I'd also love to hear from someone with more thermal framework
experience to make sure it's legit to return a default value if
someone calls us while we're initting.  It seems sane to me but nice
to confirm it's OK.


Overall I like the idea of this patch so hopefully others do too.
Thanks for sending it out!


-Doug