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: <CAJZ5v0jbHR03UyJBMmBBYnSsZmGG0OXqLJvMfXxXKVn4Np4Egw@mail.gmail.com>
Date:   Thu, 26 Jan 2023 17:47:34 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     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 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>
> ---
>  drivers/thermal/intel/intel_soc_dts_iosf.c | 58 ++++++++--------------
>  drivers/thermal/intel/intel_soc_dts_iosf.h |  2 +-
>  2 files changed, 23 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
> index 342b0bb5a56d..130c416ec601 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.c
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
> @@ -71,20 +71,13 @@ static int get_tj_max(u32 *tj_max)
>         return err;
>  }
>
> -static int sys_get_trip_temp(struct thermal_zone_device *tzd, int trip,
> -                            int *temp)
> +static int get_trip_temp(struct intel_soc_dts_sensors *sensors, int trip, int *temp)
>  {
>         int status;
>         u32 out;
> -       struct intel_soc_dts_sensor_entry *dts;
> -       struct intel_soc_dts_sensors *sensors;
>
> -       dts = tzd->devdata;
> -       sensors = dts->sensors;
> -       mutex_lock(&sensors->dts_update_lock);
>         status = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
>                                SOC_DTS_OFFSET_PTPS, &out);
> -       mutex_unlock(&sensors->dts_update_lock);
>         if (status)
>                 return status;
>
> @@ -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?

> +
>         return 0;
>  err_restore_te_out:
>         iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> @@ -202,24 +200,12 @@ static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip,
>
>         mutex_lock(&sensors->dts_update_lock);
>         status = update_trip_temp(tzd->devdata, trip, temp,
> -                                 dts->trip_types[trip]);
> +                                 dts->trips[trip].type);
>         mutex_unlock(&sensors->dts_update_lock);
>
>         return status;
>  }
>
> -static int sys_get_trip_type(struct thermal_zone_device *tzd,
> -                            int trip, enum thermal_trip_type *type)
> -{
> -       struct intel_soc_dts_sensor_entry *dts;
> -
> -       dts = tzd->devdata;
> -
> -       *type = dts->trip_types[trip];
> -
> -       return 0;
> -}
> -
>  static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>                              int *temp)
>  {
> @@ -245,8 +231,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>
>  static struct thermal_zone_device_ops tzone_ops = {
>         .get_temp = sys_get_curr_temp,
> -       .get_trip_temp = sys_get_trip_temp,
> -       .get_trip_type = sys_get_trip_type,
>         .set_trip_temp = sys_set_trip_temp,
>  };
>
> @@ -320,7 +304,8 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>         dts->trip_mask = trip_mask;
>         dts->trip_count = trip_count;
>         snprintf(name, sizeof(name), "soc_dts%d", id);
> -       dts->tzone = thermal_zone_device_register(name,
> +       dts->tzone = thermal_zone_device_register_with_trips(name,
> +                                                 dts->trips,
>                                                   trip_count,
>                                                   trip_mask,
>                                                   dts, &tzone_ops,
> @@ -430,27 +415,28 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
>                 notification = false;
>         else
>                 notification = true;
> -       for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> -               sensors->soc_dts[i].sensors = sensors;
> -               ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> -                                          notification, trip_count,
> -                                          read_only_trip_count);
> -               if (ret)
> -                       goto err_free;
> -       }
>
>         for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
>                 ret = update_trip_temp(&sensors->soc_dts[i], 0, 0,
>                                        THERMAL_TRIP_PASSIVE);
>                 if (ret)
> -                       goto err_remove_zone;
> +                       goto err_free;
>
>                 ret = update_trip_temp(&sensors->soc_dts[i], 1, 0,
>                                        THERMAL_TRIP_PASSIVE);
>                 if (ret)
> -                       goto err_remove_zone;
> +                       goto err_free;
>         }
>
> +       for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> +               sensors->soc_dts[i].sensors = sensors;
> +               ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> +                                          notification, trip_count,
> +                                          read_only_trip_count);
> +               if (ret)
> +                       goto err_remove_zone;
> +       }
> +
>         return sensors;
>  err_remove_zone:
>         for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i)
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
> index c54945748200..ee0a39e3edd3 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.h
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
> @@ -27,7 +27,7 @@ struct intel_soc_dts_sensor_entry {
>         u32 store_status;
>         u32 trip_mask;
>         u32 trip_count;
> -       enum thermal_trip_type trip_types[2];
> +       struct thermal_trip trips[2];
>         struct thermal_zone_device *tzone;
>         struct intel_soc_dts_sensors *sensors;
>  };
> --
> 2.34.1
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ