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

[jonghwa3.lee@samsung.com]

On 2014년 10월 07일 21:50, Krzysztof Kozlowski wrote:

> On wto, 2014-10-07 at 19:18 +0900, jonghwa3.lee@samsung.com wrote:
>> On 2014년 10월 07일 16:13, Krzysztof Kozlowski wrote:
>>
>>>
>>> On wto, 2014-10-07 at 11:20 +0900, jonghwa3.lee@samsung.com 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@samsung.com>
>>>
>>> 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 :)
> 
> Yes, that would remove recursive call but this would also remove thermal
> zone for charger manager's power supply. Does anyone (user space?)
> depend and use charger managers thermal zone?
> 


AFAIK, no,, charger manager's thermal zone means nothing but just replica of
other thermal zone related with battery. It is better to remove it.

Thanks,
Jonghwa.

> Best regards,
> Krzysztof
> 
>