Messages in this thread | | | Subject | Re: [PATCH] power: charger-manager: Avoid recursive thermal get_temp call | From | Krzysztof Kozlowski <> | Date | Tue, 07 Oct 2014 17:05:34 +0200 |
| |
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
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |