linux-kernel - Re: [PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2 rev 2 PMIC peripherals

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] [day] [month] [year] [list]

Message-ID: <CAA8EJprBZfO8mMoXAm0iw0u=VZhcSkT1iQ7m-AupdKFnk26Qyw@mail.gmail.com>
Date: Wed, 31 Jul 2024 02:37:21 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Anjelique Melendez <quic_amelende@...cinc.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, amitk@...nel.org, 
	thara.gopinath@...il.com, andersson@...nel.org, quic_collinsd@...cinc.com, 
	rafael@...nel.org, daniel.lezcano@...aro.org, rui.zhang@...el.com, 
	lukasz.luba@....com, linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2
 rev 2 PMIC peripherals

On Wed, 31 Jul 2024 at 01:44, Anjelique Melendez
<quic_amelende@...cinc.com> wrote:
>
> >>
> >> +/* Configure TEMP_DAC registers based on DT thermal_zone trips */
> >> +static int qpnp_tm_gen2_rev2_update_trip_temps(struct qpnp_tm_chip *chip)
> >> +{
> >> +    struct thermal_trip trip = {0};
> >> +    int ret, ntrips, i;
> >> +
> >> +    ntrips = thermal_zone_get_num_trips(chip->tz_dev);
> >> +    /* Keep hardware defaults if no DT trips are defined. */
> >> +    if (ntrips <= 0)
> >> +            return 0;
> >> +
> >> +    for (i = 0; i < ntrips; i++) {
> >> +            ret = thermal_zone_get_trip(chip->tz_dev, i, &trip);
> >> +            if (ret < 0)
> >> +                    return ret;
> >> +
> >> +            ret = qpnp_tm_gen2_rev2_set_temp_thresh(chip, i, trip.temperature);
> >> +            if (ret < 0)
> >> +                    return ret;
> >> +    }
> >> +
> >> +    /* Verify that trips are strictly increasing. */
> >
> > There is no such requirement in the DT bindings. Please don't invent
> > artificial restrictions, especially if they are undocumented.
> >
>
> This is not an entirely new restirction. Currently the temp alarm driver
> has hardcoded temperature thresholds options which are "strictly increasing"
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/qcom/qcom-spmi-temp-alarm.c?h=v6.11-rc1#n44).
> The threshold values are initially configured based on the stage 2 critical trip
> temperature.
> For newer PMICs, we have individual temperature registers for stage 1, 2, and 3,
> so we instead configure each threshold temperature as defined in DT. In general
> since stage 1 = warning, stage 2 = system should shut down, stage 3 = emergency shutdown,
> we would expect for temperature thresholds to increase for each stage
> (https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/thermal?h=v5.4.281&id=f1599f9e4cd6f1dd0cad202853fb830854f4e944).
>
> I agree that we are missing some documentation but since the trips are defined in the
> thermal_zone node what is the best way to mention this requirement? Will adding a
> few sentences to qcom,spmi-temp-alarm.yaml description be enough? Do we need
> to make changes to thermal_zone.yaml so that dt_binding_check catches this requirement?

In my opinion the driver needs to be fixed to compile the state after
looking at all trip points, but Daniel / Amit / Thara can have a more
qualified opinion.

>
> >> +    for (i = 1; i < STAGE_COUNT; i++) {
> >> +            if (chip->temp_dac_map[i] <= chip->temp_dac_map[i - 1]) {
> >> +                    dev_err(chip->dev, "Threshold %d=%ld <= threshold %d=%ld\n",
> >> +                            i, chip->temp_dac_map[i], i - 1,
> >> +                            chip->temp_dac_map[i - 1]);
> >> +                    return -EINVAL;
> >> +            }
> >> +    }
> >> +
> >> +    return 0;
> Thanks,
> Anjelique



-- 
With best wishes
Dmitry