linux-kernel - Re: [PATCH 3/3] thermal/drivers/intel: Use generic trip points for intel_soc_dts

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: <CAJZ5v0hFG1VjzKubWkLYPXeGHN9sMt4SDdC_f-dLseaXpsAOKg@mail.gmail.com>
Date:   Tue, 31 Jan 2023 20:17:54 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        srinivas.pandruvada@...ux.intel.com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, rui.zhang@...el.com,
        Daniel Lezcano <daniel.lezcano@...nel.org>,
        Amit Kucheria <amitk@...nel.org>
Subject: Re: [PATCH 3/3] thermal/drivers/intel: Use generic trip points for intel_soc_dts_iosf

On Tue, Jan 31, 2023 at 6:03 PM Daniel Lezcano
<daniel.lezcano@...aro.org> wrote:
>
>
> Hi Rafael,
>
>
> On 26/01/2023 17:47, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> > <daniel.lezcano@...aro.org> wrote:
> >>
> >> From: Daniel Lezcano <daniel.lezcano@...nel.org>
> >>
> >> The thermal framework gives the possibility to register the trip
> >> points with the thermal zone. When that is done, no get_trip_* ops are
> >> needed and they can be removed.
> >>
> >> Convert ops content logic into generic trip points and register them with the
> >> thermal zone.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> >> ---
>
> [ ... ]
>
> >> @@ -173,8 +166,13 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> >>          if (status)
> >>                  goto err_restore_te_out;
> >>
> >> -       dts->trip_types[thres_index] = trip_type;
> >> -
> >> +       status = get_trip_temp(sensors, thres_index, &temp);
> >> +       if (status)
> >> +               goto err_restore_te_out;
> >> +
> >> +       dts->trips[thres_index].type = trip_type;
> >> +       dts->trips[thres_index].temperature = temp;
> >
> > This change doesn't look correct to me, because this function takes
> > temp as an argument and it is used to populate the trip with it at
> > least in some cases.
> >
> > Why should temp be overwritten here?
>
> You are correct. This is wrong.
>
> I think we should call get_trip_temp() before calling update_trip_temp()
> instead of passing a zero temperature parameter

update_trip_temp() is sort of a misnomer, because it is used for
initializing a trip point for example in
intel_soc_dts_iosf_add_read_only_critical_trip() and in this
particular case get_trip_temp() need not be called before it.

This driver seems to be in need of a cleanup.