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

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

On wto, 2014-10-07 at 22:05 +0900, jonghwa3.lee@samsung.com wrote:
> 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.

Great! I'll send a patch fixing both paths of recursive call.

Best regards,
Krzysztof