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

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

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?

Best regards,
Krzysztof