Re: [PATCH v1 13/17] mlxsw: core_thermal: Use the .should_bind() thermal zone callback

[Ido Schimmel]

On Tue, Jul 30, 2024 at 08:34:45PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> 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@xxxxxxxxx>

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@xxxxxxxxxx/

> ---
> 
> 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);
> 
> 
>