| 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
| ||
|
Message-ID: <ZqoxXdQRxhfr5cHY@shredder.mtl.com>
Date: Wed, 31 Jul 2024 15:43:09 +0300
From: Ido Schimmel <idosch@...dia.com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Linux PM <linux-pm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Linux ACPI <linux-acpi@...r.kernel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Lukasz Luba <lukasz.luba@....com>, Zhang Rui <rui.zhang@...el.com>,
Petr Machata <petrm@...dia.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v1 13/17] mlxsw: core_thermal: Use the .should_bind()
thermal zone callback
On Tue, Jul 30, 2024 at 08:34:45PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> Make the mlxsw core_thermal driver use the .should_bind() thermal zone
> callback to provide the thermal core with the information on whether or
> not to bind the given cooling device to the given trip point in the
> given thermal zone. If it returns 'true', the thermal core will bind
> the cooling device to the trip and the corresponding unbinding will be
> taken care of automatically by the core on the removal of the involved
> thermal zone or cooling device.
>
> It replaces the .bind() and .unbind() thermal zone callbacks (in 3
> places) which assumed the same trip points ordering in the driver
> and in the thermal core (that may not be true any more in the
> future). The .bind() callbacks used loops over trip point indices
> to call thermal_zone_bind_cooling_device() for the same cdev (once
> it had been verified) and all of the trip points, but they passed
> different 'upper' and 'lower' values to it for each trip.
>
> To retain the original functionality, the .should_bind() callbacks
> need to use the same 'upper' and 'lower' values that would be used
> by the corresponding .bind() callbacks when they are about about to
Nit: s/about about/about/
> return 'true'. To that end, the 'priv' field of each trip is set
> during the thermal zone initialization to point to the corresponding
> 'state' object containing the maximum and minimum cooling states of
> the cooling device.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
Please see more comments below, but this patch is going to conflict with
the series at [1] which is currently under review. How do you want to
handle that?
https://lore.kernel.org/netdev/cover.1722345311.git.petrm@nvidia.com/
> ---
>
> This patch only depends on patch [09/17].
>
> ---
> drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 121 +++++----------------
> 1 file changed, 34 insertions(+), 87 deletions(-)
>
> Index: linux-pm/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> +++ linux-pm/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
> @@ -165,52 +165,22 @@ static int mlxsw_get_cooling_device_idx(
> return -ENODEV;
> }
>
> -static int mlxsw_thermal_bind(struct thermal_zone_device *tzdev,
> - struct thermal_cooling_device *cdev)
> +static bool mlxsw_thermal_should_bind(struct thermal_zone_device *tzdev,
> + const struct thermal_trip *trip,
> + struct thermal_cooling_device *cdev,
> + struct cooling_spec *c)
> {
> struct mlxsw_thermal *thermal = thermal_zone_device_priv(tzdev);
> - struct device *dev = thermal->bus_info->dev;
> - int i, err;
> + const struct mlxsw_cooling_states *state = trip->priv;
>
> /* If the cooling device is one of ours bind it */
> if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> - return 0;
> + return false;
>
> - for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> - const struct mlxsw_cooling_states *state = &thermal->cooling_states[i];
> + c->upper = state->max_state;
> + c->lower = state->min_state;
>
> - err = thermal_zone_bind_cooling_device(tzdev, i, cdev,
> - state->max_state,
> - state->min_state,
> - THERMAL_WEIGHT_DEFAULT);
> - if (err < 0) {
> - dev_err(dev, "Failed to bind cooling device to trip %d\n", i);
> - return err;
> - }
> - }
> - return 0;
> -}
> -
> -static int mlxsw_thermal_unbind(struct thermal_zone_device *tzdev,
> - struct thermal_cooling_device *cdev)
> -{
> - struct mlxsw_thermal *thermal = thermal_zone_device_priv(tzdev);
> - struct device *dev = thermal->bus_info->dev;
> - int i;
> - int err;
> -
> - /* If the cooling device is our one unbind it */
> - if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> - return 0;
> -
> - for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> - err = thermal_zone_unbind_cooling_device(tzdev, i, cdev);
> - if (err < 0) {
> - dev_err(dev, "Failed to unbind cooling device\n");
> - return err;
> - }
> - }
> - return 0;
> + return true;
> }
>
> static int mlxsw_thermal_get_temp(struct thermal_zone_device *tzdev,
> @@ -239,59 +209,29 @@ static struct thermal_zone_params mlxsw_
> .no_hwmon = true,
> };
>
> -static struct thermal_zone_device_ops mlxsw_thermal_ops = {
> - .bind = mlxsw_thermal_bind,
> - .unbind = mlxsw_thermal_unbind,
> - .get_temp = mlxsw_thermal_get_temp,
> -};
Is there a reason to move 'mlxsw_thermal_ops' below?
> -
> -static int mlxsw_thermal_module_bind(struct thermal_zone_device *tzdev,
> - struct thermal_cooling_device *cdev)
> +static bool mlxsw_thermal_module_should_bind(struct thermal_zone_device *tzdev,
> + const struct thermal_trip *trip,
> + struct thermal_cooling_device *cdev,
> + struct cooling_spec *c)
> {
> struct mlxsw_thermal_module *tz = thermal_zone_device_priv(tzdev);
> struct mlxsw_thermal *thermal = tz->parent;
> - int i, j, err;
> + const struct mlxsw_cooling_states *state = trip->priv;
Please place it between 'tz' and 'thermal'. Networking code tries to
maintain reverse xmas tree ordering for local variables.
>
> /* If the cooling device is one of ours bind it */
> if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> - return 0;
> + return false;
>
> - for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> - const struct mlxsw_cooling_states *state = &tz->cooling_states[i];
> + c->upper = state->max_state;
> + c->lower = state->min_state;
>
> - err = thermal_zone_bind_cooling_device(tzdev, i, cdev,
> - state->max_state,
> - state->min_state,
> - THERMAL_WEIGHT_DEFAULT);
> - if (err < 0)
> - goto err_thermal_zone_bind_cooling_device;
> - }
> - return 0;
> -
> -err_thermal_zone_bind_cooling_device:
> - for (j = i - 1; j >= 0; j--)
> - thermal_zone_unbind_cooling_device(tzdev, j, cdev);
> - return err;
> + return true;
> }
>
> -static int mlxsw_thermal_module_unbind(struct thermal_zone_device *tzdev,
> - struct thermal_cooling_device *cdev)
> -{
> - struct mlxsw_thermal_module *tz = thermal_zone_device_priv(tzdev);
> - struct mlxsw_thermal *thermal = tz->parent;
> - int i;
> - int err;
> -
> - /* If the cooling device is one of ours unbind it */
> - if (mlxsw_get_cooling_device_idx(thermal, cdev) < 0)
> - return 0;
> -
> - for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++) {
> - err = thermal_zone_unbind_cooling_device(tzdev, i, cdev);
> - WARN_ON(err);
> - }
> - return err;
> -}
> +static struct thermal_zone_device_ops mlxsw_thermal_ops = {
> + .should_bind = mlxsw_thermal_should_bind,
> + .get_temp = mlxsw_thermal_get_temp,
> +};
>
> static int mlxsw_thermal_module_temp_get(struct thermal_zone_device *tzdev,
> int *p_temp)
> @@ -313,8 +253,7 @@ static int mlxsw_thermal_module_temp_get
> }
>
> static struct thermal_zone_device_ops mlxsw_thermal_module_ops = {
> - .bind = mlxsw_thermal_module_bind,
> - .unbind = mlxsw_thermal_module_unbind,
> + .should_bind = mlxsw_thermal_module_should_bind,
> .get_temp = mlxsw_thermal_module_temp_get,
> };
>
> @@ -342,8 +281,7 @@ static int mlxsw_thermal_gearbox_temp_ge
> }
>
> static struct thermal_zone_device_ops mlxsw_thermal_gearbox_ops = {
> - .bind = mlxsw_thermal_module_bind,
> - .unbind = mlxsw_thermal_module_unbind,
> + .should_bind = mlxsw_thermal_module_should_bind,
> .get_temp = mlxsw_thermal_gearbox_temp_get,
> };
>
> @@ -451,6 +389,7 @@ mlxsw_thermal_module_init(struct device
> struct mlxsw_thermal_area *area, u8 module)
> {
> struct mlxsw_thermal_module *module_tz;
> + int i;
>
> module_tz = &area->tz_module_arr[module];
> /* Skip if parent is already set (case of port split). */
> @@ -465,6 +404,8 @@ mlxsw_thermal_module_init(struct device
> sizeof(thermal->trips));
> memcpy(module_tz->cooling_states, default_cooling_states,
> sizeof(thermal->cooling_states));
> + for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++)
> + module_tz->trips[i].priv = &module_tz->cooling_states[i];
> }
>
> static void mlxsw_thermal_module_fini(struct mlxsw_thermal_module *module_tz)
> @@ -579,7 +520,7 @@ mlxsw_thermal_gearboxes_init(struct devi
> struct mlxsw_thermal_module *gearbox_tz;
> char mgpir_pl[MLXSW_REG_MGPIR_LEN];
> u8 gbox_num;
> - int i;
> + int i, j;
> int err;
>
> mlxsw_reg_mgpir_pack(mgpir_pl, area->slot_index);
> @@ -606,6 +547,9 @@ mlxsw_thermal_gearboxes_init(struct devi
> sizeof(thermal->trips));
> memcpy(gearbox_tz->cooling_states, default_cooling_states,
> sizeof(thermal->cooling_states));
> + for (j = 0; j < MLXSW_THERMAL_NUM_TRIPS; j++)
> + gearbox_tz->trips[j].priv = &gearbox_tz->cooling_states[j];
> +
> gearbox_tz->module = i;
> gearbox_tz->parent = thermal;
> gearbox_tz->slot_index = area->slot_index;
> @@ -722,6 +666,9 @@ int mlxsw_thermal_init(struct mlxsw_core
> thermal->bus_info = bus_info;
> memcpy(thermal->trips, default_thermal_trips, sizeof(thermal->trips));
> memcpy(thermal->cooling_states, default_cooling_states, sizeof(thermal->cooling_states));
> + for (i = 0; i < MLXSW_THERMAL_NUM_TRIPS; i++)
> + thermal->trips[i].priv = &thermal->cooling_states[i];
> +
> thermal->line_cards[0].slot_index = 0;
>
> err = mlxsw_reg_query(thermal->core, MLXSW_REG(mfcr), mfcr_pl);
>
>
>
Powered by blists - more mailing lists