Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0e986fbb-e76b-9663-26c2-b84d887b4c98@linaro.org>
Date:   Fri, 16 Sep 2022 19:06:42 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v3 30/30] thermal/drivers/intel: Use generic
 thermal_zone_get_trip() function
Content-Language: en-US
To:     rafael@kernel.org,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Jiang Jian <jiangjian@cdjrlc.com>
References: <20220906164720.330701-1-daniel.lezcano@linaro.org>
 <20220906164720.330701-31-daniel.lezcano@linaro.org>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <20220906164720.330701-31-daniel.lezcano@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


Hi Srinivas,

I've Cc'ed you on this patch in case you have any comment

Thanks

   -- D.

On 06/09/2022 18:47, Daniel Lezcano wrote:
> 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@linaro.org>
> ---
>   drivers/thermal/intel/x86_pkg_temp_thermal.c | 120 ++++++++++---------
>   1 file changed, 66 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/thermal/intel/x86_pkg_temp_thermal.c b/drivers/thermal/intel/x86_pkg_temp_thermal.c
> index a0e234fce71a..e7c3b78d959c 100644
> --- a/drivers/thermal/intel/x86_pkg_temp_thermal.c
> +++ b/drivers/thermal/intel/x86_pkg_temp_thermal.c
> @@ -53,6 +53,7 @@ struct zone_device {
>   	u32				msr_pkg_therm_high;
>   	struct delayed_work		work;
>   	struct thermal_zone_device	*tzone;
> +	struct thermal_trip		*trips;
>   	struct cpumask			cpumask;
>   };
>   
> @@ -138,40 +139,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd, int *temp)
>   	return -EINVAL;
>   }
>   
> -static int sys_get_trip_temp(struct thermal_zone_device *tzd,
> -			     int trip, int *temp)
> -{
> -	struct zone_device *zonedev = tzd->devdata;
> -	unsigned long thres_reg_value;
> -	u32 mask, shift, eax, edx;
> -	int ret;
> -
> -	if (trip >= MAX_NUMBER_OF_TRIPS)
> -		return -EINVAL;
> -
> -	if (trip) {
> -		mask = THERM_MASK_THRESHOLD1;
> -		shift = THERM_SHIFT_THRESHOLD1;
> -	} else {
> -		mask = THERM_MASK_THRESHOLD0;
> -		shift = THERM_SHIFT_THRESHOLD0;
> -	}
> -
> -	ret = rdmsr_on_cpu(zonedev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
> -			   &eax, &edx);
> -	if (ret < 0)
> -		return ret;
> -
> -	thres_reg_value = (eax & mask) >> shift;
> -	if (thres_reg_value)
> -		*temp = zonedev->tj_max - thres_reg_value * 1000;
> -	else
> -		*temp = THERMAL_TEMP_INVALID;
> -	pr_debug("sys_get_trip_temp %d\n", *temp);
> -
> -	return 0;
> -}
> -
>   static int
>   sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp)
>   {
> @@ -212,18 +179,9 @@ sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp)
>   			l, h);
>   }
>   
> -static int sys_get_trip_type(struct thermal_zone_device *thermal, int trip,
> -			     enum thermal_trip_type *type)
> -{
> -	*type = THERMAL_TRIP_PASSIVE;
> -	return 0;
> -}
> -
>   /* Thermal zone callback registry */
>   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,
>   };
>   
> @@ -328,6 +286,48 @@ static int pkg_thermal_notify(u64 msr_val)
>   	return 0;
>   }
>   
> +static struct thermal_trip *pkg_temp_thermal_trips_init(int cpu, int tj_max, int num_trips)
> +{
> +	struct thermal_trip *trips;
> +	unsigned long thres_reg_value;
> +	u32 mask, shift, eax, edx;
> +	int ret, i;
> +
> +	trips = kzalloc(sizeof(*trips) * num_trips, GFP_KERNEL);
> +	if (!trips)
> +		return ERR_PTR(-ENOMEM);
> +	
> +	for (i = 0; i < num_trips; i++) {
> +
> +		if (i) {
> +			mask = THERM_MASK_THRESHOLD1;
> +			shift = THERM_SHIFT_THRESHOLD1;
> +		} else {
> +			mask = THERM_MASK_THRESHOLD0;
> +			shift = THERM_SHIFT_THRESHOLD0;
> +		}
> +
> +		ret = rdmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT,
> +				   &eax, &edx);
> +		if (ret < 0) {
> +			kfree(trips);
> +			return ERR_PTR(ret);
> +		}
> +
> +		thres_reg_value = (eax & mask) >> shift;
> +
> +		trips[i].temperature = thres_reg_value ?
> +			tj_max - thres_reg_value * 1000 : THERMAL_TEMP_INVALID;
> +
> +		trips[i].type = THERMAL_TRIP_PASSIVE;
> +		
> +		pr_debug("%s: cpu=%d, trip=%d, temp=%d\n",
> +			 __func__, cpu, i, trips[i].temperature);
> +	}
> +
> +	return trips;
> +}
> +
>   static int pkg_temp_thermal_device_add(unsigned int cpu)
>   {
>   	int id = topology_logical_die_id(cpu);
> @@ -353,24 +353,27 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
>   	if (!zonedev)
>   		return -ENOMEM;
>   
> +	zonedev->trips = pkg_temp_thermal_trips_init(cpu, tj_max, thres_count);
> +	if (IS_ERR(zonedev->trips)) {
> +		err = PTR_ERR(zonedev->trips);
> +		goto out_kfree_zonedev;
> +	}
> +	
>   	INIT_DELAYED_WORK(&zonedev->work, pkg_temp_thermal_threshold_work_fn);
>   	zonedev->cpu = cpu;
>   	zonedev->tj_max = tj_max;
> -	zonedev->tzone = thermal_zone_device_register("x86_pkg_temp",
> -			thres_count,
> +	zonedev->tzone = thermal_zone_device_register_with_trips("x86_pkg_temp",
> +			zonedev->trips, thres_count,
>   			(thres_count == MAX_NUMBER_OF_TRIPS) ? 0x03 : 0x01,
>   			zonedev, &tzone_ops, &pkg_temp_tz_params, 0, 0);
>   	if (IS_ERR(zonedev->tzone)) {
>   		err = PTR_ERR(zonedev->tzone);
> -		kfree(zonedev);
> -		return err;
> +		goto out_kfree_trips;
>   	}
>   	err = thermal_zone_device_enable(zonedev->tzone);
> -	if (err) {
> -		thermal_zone_device_unregister(zonedev->tzone);
> -		kfree(zonedev);
> -		return err;
> -	}
> +	if (err)
> +		goto out_unregister_tz;
> +
>   	/* Store MSR value for package thermal interrupt, to restore at exit */
>   	rdmsr(MSR_IA32_PACKAGE_THERM_INTERRUPT, zonedev->msr_pkg_therm_low,
>   	      zonedev->msr_pkg_therm_high);
> @@ -379,7 +382,14 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
>   	raw_spin_lock_irq(&pkg_temp_lock);
>   	zones[id] = zonedev;
>   	raw_spin_unlock_irq(&pkg_temp_lock);
> -	return 0;
> +
> +out_unregister_tz:	
> +	thermal_zone_device_unregister(zonedev->tzone);
> +out_kfree_trips:
> +	kfree(zonedev->trips);
> +out_kfree_zonedev:
> +	kfree(zonedev);
> +	return err;
>   }
>   
>   static int pkg_thermal_cpu_offline(unsigned int cpu)
> @@ -463,8 +473,10 @@ static int pkg_thermal_cpu_offline(unsigned int cpu)
>   	raw_spin_unlock_irq(&pkg_temp_lock);
>   
>   	/* Final cleanup if this is the last cpu */
> -	if (lastcpu)
> +	if (lastcpu) {
> +		kfree(zonedev->trips);
>   		kfree(zonedev);
> +	}
>   	return 0;
>   }
>   


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog