Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 2016年03月16日 03:49, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Mar 15, 2016 at 05:12:12PM +0800, Wei Ni wrote:
>>
>>
>> On 2016年03月15日 03:16, Eduardo Valentin wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Fri, Mar 11, 2016 at 11:11:12AM +0800, Wei Ni wrote:
>>>> Add support for hardware critical thermal limits to the
>>>> SOC_THERM driver. It use the Linux thermal framework to
>>>> create critical trip temp, and set it to SOC_THERM hardware.
>>>> If these limits are breached, the chip will reset, and if
>>>> appropriately configured, will turn off the PMIC.
>>>>
>>>> This support is critical for safe usage of the chip.
>>>>
>>>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  drivers/thermal/tegra/soctherm.c          | 166 +++++++++++++++++++++++++++++-
>>>>  drivers/thermal/tegra/soctherm.h          |   7 ++
>>>>  drivers/thermal/tegra/tegra124-soctherm.c |  24 +++++
>>>>  drivers/thermal/tegra/tegra210-soctherm.c |  24 +++++
>>>>  4 files changed, 216 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
>>>> index 02ac6d2e5a20..dbaab160baba 100644
>>>> --- a/drivers/thermal/tegra/soctherm.c
>>>> +++ b/drivers/thermal/tegra/soctherm.c
>>>> @@ -73,9 +73,14 @@
>>>>  #define REG_SET_MASK(r, m, v)	(((r) & ~(m)) | \
>>>>  				 (((v) & (m >> (ffs(m) - 1))) << (ffs(m) - 1)))
>>>>  
>>>> +static const int min_low_temp = -127000;
>>>> +static const int max_high_temp = 127000;
>>>> +
>>>>  struct tegra_thermctl_zone {
>>>>  	void __iomem *reg;
>>>> -	u32 mask;
>>>> +	struct device *dev;
>>>> +	struct thermal_zone_device *tz;
>>>
>>>
>>> Why not using tz->dev for the *dev above?
>>
>> The tz is thermal_zone_device, this structure doesn't have *dev.
>> It only have the member "struct device device;", but this device is created for
>> the thermal class, not this tegra_soctherm device.
>>
>>>
>>>> +	const struct tegra_tsensor_group *sg;
>>>>  };
>>>>  
>>>>  struct tegra_soctherm {
>>>> @@ -145,22 +150,158 @@ static int tegra_thermctl_get_temp(void *data, int *out_temp)
>>>>  	u32 val;
>>>>  
>>>>  	val = readl(zone->reg);
>>>> -	val = REG_GET_MASK(val, zone->mask);
>>>> +	val = REG_GET_MASK(val, zone->sg->sensor_temp_mask);
>>>>  	*out_temp = translate_temp(val);
>>>>  
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int
>>>> +thermtrip_program(struct device *dev, const struct tegra_tsensor_group *sg,
>>>> +		  int trip_temp);
>>>> +
>>>> +static int tegra_thermctl_set_trip_temp(void *data, int trip, int temp)
>>>> +{
>>>> +	struct tegra_thermctl_zone *zone = data;
>>>> +	struct thermal_zone_device *tz = zone->tz;
>>>> +	const struct tegra_tsensor_group *sg = zone->sg;
>>>> +	struct device *dev = zone->dev;
>>>> +	enum thermal_trip_type type;
>>>> +	int ret;
>>>> +
>>>> +	if (!tz)
>>>> +		return -EINVAL;
>>>
>>>
>>> Is the above check needed? If you saw a case in which your function is
>>> called without tz, would it be the case we have a but in the probe (or
>>> even worse, in thermal-core)?
>>
>> This tz isn't from thermal-core, it's from the "void *data".
>> This *data is the private structure "struct tegra_thermctl_zone *zone = data;".
>> It is registered in devm_thermal_zone_of_sensor_register(*dev, sensor_id, *data,
>> *ops). And when it register successful, I will set zone->tz = z, in here, the
>> zone is the private data.
>> Let's consider a special case, once the thermal_zone_of_sensor_register
>> successful and didn't run to "zone->tz = z" yet, then the thermal_core implement
>> .set_trip(), then it may cause problems in here, although it's difficult to hit
>> this case. So I think we need to do this check.
> 
> 
> Can you be more specific? I don't recall a case that core would call any
> driver callbacks before setting up the data structures properly.

Sorry for the confusion, I mean this data structure is the private data pointer,
it doesn't handled by thermal_core. Let me explain more:
In this tegra soctherm driver, I run following steps in probe routine:
step1:
z = devm_thermal_zone_of_sensor_register(*dev, sensor_id, zone, ops);
register thermal zone device, in here, the parameter "zone" is the private data
pointer "structure tegra_thermctl_zone".
step 2:
After register, it return the "z", and I set it to the private data:
zone->tz = z;

In the .set_trip() callback, it will pass back this private data pointer.
So if the callback was called between step1 and step2, at that time the zone->tz
didn't be set yet, it will cause problems, although I didn't hit this case.

This check doesn't relate with core driver, it is used to check my private data
pointer.

Wei.

> 
>>>
> 
> * Unknown Key
> * 0x7DA4E256
>