[PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call

[PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call

[Krzysztof Kozlowski]

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.

Get rid of false lockdep alert and recursive call by setting
'no_thermal' property for this power supply class. The thermal zone for
charger manager won't be created (user space does not use it anyway).

The lockdep report:
[    2.540339] charger-manager charger-manager@0: Ignoring full-battery voltage threshold as it is not supplied
[    2.540351] charger-manager charger-manager@0: Ignoring full-battery full capacity threshold as it is not supplied
[    2.546296]
[    2.546302] =============================================
[    2.546305] [ INFO: possible recursive locking detected ]
[    2.546312] 3.17.0-rc6-next-20140926-00012-gbb13895e46af-dirty #39 Not tainted
[    2.546316] ---------------------------------------------
[    2.546321] swapper/0/1 is trying to acquire lock:
[    2.546348]  (&tz->lock){+.+...}, at: [<c0321d24>] thermal_zone_get_temp+0x38/0x68
[    2.546352]
[    2.546352] but task is already holding lock:
[    2.546369]  (&tz->lock){+.+...}, at: [<c0321d24>] thermal_zone_get_temp+0x38/0x68
[    2.546373]
[    2.546373] other info that might help us debug this:
[    2.546376]  Possible unsafe locking scenario:
[    2.546376]
[    2.546378]        CPU0
[    2.546380]        ----
[    2.546386]   lock(&tz->lock);
[    2.546392]   lock(&tz->lock);
[    2.546394]
[    2.546394]  *** DEADLOCK ***
[    2.546394]
[    2.546397]  May be due to missing lock nesting notation
[    2.546397]
[    2.546401] 2 locks held by swapper/0/1:
[    2.546430]  #0:  (&dev->mutex){......}, at: [<c02720c4>] __driver_attach+0x58/0x98
[    2.546448]  #1:  (&tz->lock){+.+...}, at: [<c0321d24>] thermal_zone_get_temp+0x38/0x68
[    2.546451]
[    2.546451] stack backtrace:
[    2.546460] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.17.0-rc6-next-20140926-00012-gbb13895e46af-dirty #39
[    2.546497] [<c00140f0>] (unwind_backtrace) from [<c0011228>] (show_stack+0x10/0x14)
[    2.546526] [<c0011228>] (show_stack) from [<c046158c>] (dump_stack+0x70/0xbc)
[    2.546554] [<c046158c>] (dump_stack) from [<c005e32c>] (validate_chain.isra.24+0x718/0x890)
[    2.546569] [<c005e32c>] (validate_chain.isra.24) from [<c005f0a0>] (__lock_acquire+0x498/0xa78)
[    2.546581] [<c005f0a0>] (__lock_acquire) from [<c005fb50>] (lock_acquire+0x78/0xb8)
[    2.546594] [<c005fb50>] (lock_acquire) from [<c0464260>] (mutex_lock_nested+0x64/0x458)
[    2.546605] [<c0464260>] (mutex_lock_nested) from [<c0321d24>] (thermal_zone_get_temp+0x38/0x68)
[    2.546634] [<c0321d24>] (thermal_zone_get_temp) from [<c031f1e0>] (charger_get_property+0x10c/0x348)
[    2.546649] [<c031f1e0>] (charger_get_property) from [<c031af18>] (power_supply_read_temp+0x28/0x58)
[    2.546662] [<c031af18>] (power_supply_read_temp) from [<c0321d38>] (thermal_zone_get_temp+0x4c/0x68)
[    2.546676] [<c0321d38>] (thermal_zone_get_temp) from [<c03233d8>] (thermal_zone_device_update+0x24/0x9c)
[    2.546687] [<c03233d8>] (thermal_zone_device_update) from [<c0323874>] (thermal_zone_device_register+0x424/0x550)
[    2.546701] [<c0323874>] (thermal_zone_device_register) from [<c031b3c0>] (__power_supply_register+0x2a4/0x348)
[    2.546714] [<c031b3c0>] (__power_supply_register) from [<c031ff64>] (charger_manager_probe+0x600/0xe5c)
[    2.546727] [<c031ff64>] (charger_manager_probe) from [<c0273384>] (platform_drv_probe+0x48/0xa4)
[    2.546746] [<c0273384>] (platform_drv_probe) from [<c0271f54>] (driver_probe_device+0x10c/0x224)
[    2.546760] [<c0271f54>] (driver_probe_device) from [<c0272100>] (__driver_attach+0x94/0x98)
[    2.546772] [<c0272100>] (__driver_attach) from [<c0270780>] (bus_for_each_dev+0x54/0x88)
[    2.546784] [<c0270780>] (bus_for_each_dev) from [<c027173c>] (bus_add_driver+0xd4/0x1d0)
[    2.546797] [<c027173c>] (bus_add_driver) from [<c027271c>] (driver_register+0x78/0xf4)
[    2.546809] [<c027271c>] (driver_register) from [<c0008984>] (do_one_initcall+0x80/0x1d4)
[    2.546829] [<c0008984>] (do_one_initcall) from [<c0612d60>] (kernel_init_freeable+0x10c/0x1d8)
[    2.546847] [<c0612d60>] (kernel_init_freeable) from [<c045c238>] (kernel_init+0x8/0xec)
[    2.546863] [<c045c238>] (kernel_init) from [<c000e828>] (ret_from_fork+0x14/0x2c)
[    2.551396] charger-manager charger-manager@0: 'chg-reg' regulator's externally_control is 0

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

---

Changes since v1:
1. Use newly introduced 'no_thermal' property to fix the issue
   definitely. Previously only one path was solved.
---
 drivers/power/charger-manager.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
index 7098a1ce2d3c..7a1177ea238d 100644
--- a/drivers/power/charger-manager.c
+++ b/drivers/power/charger-manager.c
@@ -970,6 +970,7 @@ static struct power_supply psy_default = {
 	.properties = default_charger_props,
 	.num_properties = ARRAY_SIZE(default_charger_props),
 	.get_property = charger_get_property,
+	.no_thermal = true,
 };
 
 /**
-- 
1.9.1

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

[MyungJoo Ham]

>   
>  Add a 'no_thermal' property to the power supply class. If true then
> thermal zone won't be created for this power supply in
> power_supply_register().
> 
> Power supply drivers may want to set it if they support
> POWER_SUPPLY_PROP_TEMP and they are forwarding this get property call to
> other thermal zone.
> 
> If they won't set it lockdep may report false positive deadlock for
> thermal zone's mutex because of nested calls to thermal_zone_get_temp().
> First is the call to thermal_zone_get_temp() of the driver's thermal
> zone. Thermal core gets POWER_SUPPLY_PROP_TEMP property from this
> driver. The driver then calls other thermal zone thermal_zone_get_temp()
> and returns result.
> 
> Example of such driver is charger manager.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Do we really need to add another variable in the psy struct?
In the previous thread, I thought that wasn't needed, though.


Cheers,
MyungJoo

> 
> ---
> 
> Changes since v1:
> 1. New patch (new idea).
> ---
>  drivers/power/power_supply_core.c | 3 +++
>  include/linux/power_supply.h      | 6 ++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 6cb7fe5c022d..694e8cddd5c1 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -417,6 +417,9 @@ static int psy_register_thermal(struct power_supply *psy)
>  {
>  	int i;
>  
> +	if (psy->no_thermal)
> +		return 0;
> +
>  	/* Register battery zone device psy reports temperature */
>  	for (i = 0; i < psy->num_properties; i++) {
>  		if (psy->properties[i] == POWER_SUPPLY_PROP_TEMP) {
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 3ed049673022..096dbced02ac 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -200,6 +200,12 @@ struct power_supply {
>  	void (*external_power_changed)(struct power_supply *psy);
>  	void (*set_charged)(struct power_supply *psy);
>  
> +	/*
> +	 * Set if thermal zone should not be created for this power supply.
> +	 * For example for virtual supplies forwarding calls to actual
> +	 * sensors or other supplies.
> +	 */
> +	bool no_thermal;
>  	/* For APM emulation, think legacy userspace. */
>  	int use_for_apm;
>  
> -- 
> 1.9.1
> 

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

[Krzysztof Kozlowski]

On pon, 2014-10-13 at 06:01 +0000, MyungJoo Ham wrote:
> >   
> >  Add a 'no_thermal' property to the power supply class. If true then
> > thermal zone won't be created for this power supply in
> > power_supply_register().
> > 
> > Power supply drivers may want to set it if they support
> > POWER_SUPPLY_PROP_TEMP and they are forwarding this get property call to
> > other thermal zone.
> > 
> > If they won't set it lockdep may report false positive deadlock for
> > thermal zone's mutex because of nested calls to thermal_zone_get_temp().
> > First is the call to thermal_zone_get_temp() of the driver's thermal
> > zone. Thermal core gets POWER_SUPPLY_PROP_TEMP property from this
> > driver. The driver then calls other thermal zone thermal_zone_get_temp()
> > and returns result.
> > 
> > Example of such driver is charger manager.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> Do we really need to add another variable in the psy struct?
> In the previous thread, I thought that wasn't needed, though.

I considered the idea mentioned by  Jonghwa Lee:

static int psy_register_thermal(struct power_supply *psy)
{
...
+       if (psy->tzd)
+               return 0; 

but there would be problem in determining the ownership of the thermal
zone - who should unregister that thermal zone? Charger manager's power
supply or fuel gauge's power supply?

We could NULL-ify it manually from charger's remove function before
calling power_supply_unregister... but it is really accessing a private
field of power_supply structure. 

I think this should be done in API-level.

Best regards,
Krzysztof

> 
> 
> Cheers,
> MyungJoo
> 
> > 
> > ---
> > 
> > Changes since v1:
> > 1. New patch (new idea).
> > ---
> >  drivers/power/power_supply_core.c | 3 +++
> >  include/linux/power_supply.h      | 6 ++++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> > index 6cb7fe5c022d..694e8cddd5c1 100644
> > --- a/drivers/power/power_supply_core.c
> > +++ b/drivers/power/power_supply_core.c
> > @@ -417,6 +417,9 @@ static int psy_register_thermal(struct power_supply *psy)
> >  {
> >  	int i;
> >  
> > +	if (psy->no_thermal)
> > +		return 0;
> > +
> >  	/* Register battery zone device psy reports temperature */
> >  	for (i = 0; i < psy->num_properties; i++) {
> >  		if (psy->properties[i] == POWER_SUPPLY_PROP_TEMP) {
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index 3ed049673022..096dbced02ac 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -200,6 +200,12 @@ struct power_supply {
> >  	void (*external_power_changed)(struct power_supply *psy);
> >  	void (*set_charged)(struct power_supply *psy);
> >  
> > +	/*
> > +	 * Set if thermal zone should not be created for this power supply.
> > +	 * For example for virtual supplies forwarding calls to actual
> > +	 * sensors or other supplies.
> > +	 */
> > +	bool no_thermal;
> >  	/* For APM emulation, think legacy userspace. */
> >  	int use_for_apm;
> >  
> > -- 
> > 1.9.1
> > 

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

Hi,

On Tue, Oct 07, 2014 at 05:47:37PM +0200, 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.
> 
> Get rid of false lockdep alert and recursive call by setting
> 'no_thermal' property for this power supply class. The thermal zone for
> charger manager won't be created (user space does not use it anyway).

also pulled into next, will also be included in 3.18-rc pull request.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]