Re: [PATCH] power: charger-manager: Avoid recursive thermal get_temp call

[jonghwa3 . lee]

On 2014ë 10ì 07ì 16:13, Krzysztof Kozlowski wrote:

> 
> On wto, 2014-10-07 at 11:20 +0900, jonghwa3.lee@xxxxxxxxxxx wrote:
>> Hi,
>> On 2014ë 10ì 07ì 01:00, Krzysztof Kozlowski wrote:
>>
>>> The charger manager supports POWER_SUPPLY_PROP_TEMP property and acts
>>> as a thermal zone if any of these conditions match:
>>> 1. Fuel gauge used by charger manager supports POWER_SUPPLY_PROP_TEMP.
>>> 2. 'cm-thermal-zone' property is present in DTS (then it will supersede
>>>    the fuel gauge temperature property).
>>>
>>> However in case 1 (fuel gauge reports temperature and 'cm-thermal-zone'
>>> is not set) the charger manager forwards its get_temp calls to fuel
>>> gauge thermal zone.
>>>
>>> This leads to reporting by lockdep a false positive deadlock for thermal
>>> zone's mutex because of nested calls to thermal_zone_get_temp(). This is
>>> false positive because these are different mutexes: one for charger
>>> manager thermal zone and second for fuel gauge thermal zone.
>>>
>>
>>
>> Yes, this happens because power_supply_subsystem automatically creates
>> thermal_zone when POWER_SUPPLY_PROP_TEMP is available at the time of
>> power_supply registering. And as you point out, it makes duplicate thermal_zone
>> when some power_supply references other power_supply's. I hope it would become
>> to be a selectable option or change the manner of charger-manager itself (if the
>> charger-manager is only one who references other power_supply's temperature
>> sensing ability).
>>
>> Anyway, the code looks fine to me.
>>
>> Acked-by : Jonghwa Lee <jonghwa3.lee@xxxxxxxxxxx>
> 
> Thank you for looking at patch. However later I started to wonder
> whether my fix is sufficient. For the case when fuel gauge is used as
> source of temperature it is. But for the case when external sensor is
> used it is not - still there will be recursive call and false positive
> from lockdep.
> 
> Also minor fix is needed in my patch - s/IS_ENABLED/config_enabled/.
> 
> I can send a v2 fixing this but first question - what about second
> recursive issue (when external sensor is used instead of fuel gauge)?
> 


Yes, you're right, it still had problem for external temperature sensor case.
How about we change the core not to make duplicate thermal zone if it already
existed. It's not the common case, but it looks worthy.

like as below,

static int psy_register_thermal(struct power_supply *psy)
{
...
+	if (psy->tzd)
+		return 0;
...

We also needs some modification at charger-manager driver, however we can avoid
recursive call problem with this way :)

Thanks,
Jonghwa

> Best regards,
> Krzysztof
> 
> 
>>
>> Thanks,
>> Jonghwa.
>>
>>> Get rid of false lockdep alert and recursive call by accessing fuel gauge
>>> temperature through power supply property instead of thermal zone API.
>>>
>>
>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
>>> ---
>>>  drivers/power/charger-manager.c       | 21 +++++++++++----------
>>>  include/linux/power/charger-manager.h |  2 --
>>>  2 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
>>> index c1ed3c99c059..871fb91429c8 100644
>>> --- a/drivers/power/charger-manager.c
>>> +++ b/drivers/power/charger-manager.c
>>> @@ -559,16 +559,18 @@ static int cm_get_battery_temperature(struct charger_manager *cm,
>>>  	if (!cm->desc->measure_battery_temp)
>>>  		return -ENODEV;
>>>  
>>> -#ifdef CONFIG_THERMAL
>>> -	ret = thermal_zone_get_temp(cm->tzd_batt, (unsigned long *)temp);
>>> -	if (!ret)
>>> -		/* Calibrate temperature unit */
>>> -		*temp /= 100;
>>> -#else
>>> -	ret = cm->fuel_gauge->get_property(cm->fuel_gauge,
>>> +	if (IS_ENABLED(CONFIG_THERMAL) && cm->tzd_batt) {
>>> +		ret = thermal_zone_get_temp(cm->tzd_batt, (unsigned long *)temp);
>>> +		if (!ret)
>>> +			/* Calibrate temperature unit */
>>> +			*temp /= 100;
>>> +	} else {
>>> +		/* No external thermometer or no CONFIG_THERMAL */
>>> +		ret = cm->fuel_gauge->get_property(cm->fuel_gauge,
>>>  				POWER_SUPPLY_PROP_TEMP,
>>>  				(union power_supply_propval *)temp);
>>> -#endif
>>> +	}
>>> +
>>>  	return ret;
>>>  }
>>>  
>>> @@ -1501,9 +1503,8 @@ static int cm_init_thermal_data(struct charger_manager *cm)
>>>  		cm->charger_psy.num_properties++;
>>>  		cm->desc->measure_battery_temp = true;
>>>  	}
>>> -#ifdef CONFIG_THERMAL
>>> -	cm->tzd_batt = cm->fuel_gauge->tzd;
>>>  
>>> +#ifdef CONFIG_THERMAL
>>>  	if (ret && desc->thermal_zone) {
>>>  		cm->tzd_batt =
>>>  			thermal_zone_get_zone_by_name(desc->thermal_zone);
>>> diff --git a/include/linux/power/charger-manager.h b/include/linux/power/charger-manager.h
>>> index 07e7945a1ff2..743ed6d472c6 100644
>>> --- a/include/linux/power/charger-manager.h
>>> +++ b/include/linux/power/charger-manager.h
>>> @@ -256,9 +256,7 @@ struct charger_manager {
>>>  	struct power_supply *fuel_gauge;
>>>  	struct power_supply **charger_stat;
>>>  
>>> -#ifdef CONFIG_THERMAL
>>>  	struct thermal_zone_device *tzd_batt;
>>> -#endif
>>>  	bool charger_enabled;
>>>  
>>>  	unsigned long fullbatt_vchk_jiffies_at;
>>
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/