linux-kernel - Re: [PATCH] power: charger-manager: Avoid recursive thermal get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-id: <1412694334.1185.1.camel@AMDC1943>
Date:	Tue, 07 Oct 2014 17:05:34 +0200
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	jonghwa3.lee@...sung.com
Cc:	Sebastian Reichel <sre@...nel.org>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	Myungjoo Ham <myungjoo.ham@...sung.com>,
	Anton Vorontsov <anton@...msg.org>,
	Chanwoo Choi <cw00.choi@...sung.com>, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] power: charger-manager: Avoid recursive thermal get_temp
 call

On wto, 2014-10-07 at 22:05 +0900, jonghwa3.lee@...sung.com wrote:
> On 2014년 10월 07일 21:50, Krzysztof Kozlowski wrote:
> 
> > On wto, 2014-10-07 at 19:18 +0900, jonghwa3.lee@...sung.com wrote:
> >> On 2014년 10월 07일 16:13, Krzysztof Kozlowski wrote:
> >>
> >>>
> >>> On wto, 2014-10-07 at 11:20 +0900, jonghwa3.lee@...sung.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@...sung.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


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