* [PATCH v2 1/2] power_supply: Add no_thermal property to prevent recursive get_temp calls
@ 2014-10-07 15:47 Krzysztof Kozlowski
2014-10-07 15:47 ` [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call Krzysztof Kozlowski
2014-10-27 18:29 ` [PATCH v2 1/2] power_supply: Add no_thermal property to prevent recursive get_temp calls Sebastian Reichel
0 siblings, 2 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-07 15:47 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Myungjoo Ham, Anton Vorontsov, Jonghwa Lee, Chanwoo Choi,
linux-pm, linux-kernel
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Krzysztof Kozlowski
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>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call
2014-10-07 15:47 [PATCH v2 1/2] power_supply: Add no_thermal property to prevent recursive get_temp calls Krzysztof Kozlowski
@ 2014-10-07 15:47 ` Krzysztof Kozlowski
2014-10-27 18:32 ` Sebastian Reichel
2014-10-27 18:29 ` [PATCH v2 1/2] power_supply: Add no_thermal property to prevent recursive get_temp calls Sebastian Reichel
1 sibling, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-07 15:47 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Myungjoo Ham, Anton Vorontsov, Jonghwa Lee, Chanwoo Choi,
linux-pm, linux-kernel
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call
@ 2014-10-13 6:01 MyungJoo Ham
2014-10-13 8:58 ` Krzysztof Kozlowski
0 siblings, 1 reply; 6+ messages in thread
From: MyungJoo Ham @ 2014-10-13 6:01 UTC (permalink / raw)
To: Krzysztof Kozlowski, Sebastian Reichel, Dmitry Eremin-Solenikov,
David Woodhouse, Anton Vorontsov, 이종화,
최찬우, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: 박경민, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
>
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call
2014-10-13 6:01 [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call MyungJoo Ham
@ 2014-10-13 8:58 ` Krzysztof Kozlowski
0 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-13 8:58 UTC (permalink / raw)
To: myungjoo.ham
Cc: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Anton Vorontsov, 이종화,
최찬우, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org, 박경민,
Marek Szyprowski, Bartlomiej Zolnierkiewicz
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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] power_supply: Add no_thermal property to prevent recursive get_temp calls
2014-10-07 15:47 [PATCH v2 1/2] power_supply: Add no_thermal property to prevent recursive get_temp calls Krzysztof Kozlowski
2014-10-07 15:47 ` [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call Krzysztof Kozlowski
@ 2014-10-27 18:29 ` Sebastian Reichel
1 sibling, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2014-10-27 18:29 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Dmitry Eremin-Solenikov, David Woodhouse, Myungjoo Ham,
Anton Vorontsov, Jonghwa Lee, Chanwoo Choi, linux-pm,
linux-kernel, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
[-- Attachment #1: Type: text/plain, Size: 985 bytes --]
Hi,
On Tue, Oct 07, 2014 at 05:47:36PM +0200, Krzysztof Kozlowski 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>
pulled into next, I will include it in 3.18-rc pull request.
-- Sebastian
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call
2014-10-07 15:47 ` [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call Krzysztof Kozlowski
@ 2014-10-27 18:32 ` Sebastian Reichel
0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2014-10-27 18:32 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Dmitry Eremin-Solenikov, David Woodhouse, Myungjoo Ham,
Anton Vorontsov, Jonghwa Lee, Chanwoo Choi, linux-pm,
linux-kernel, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-27 18:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-07 15:47 [PATCH v2 1/2] power_supply: Add no_thermal property to prevent recursive get_temp calls Krzysztof Kozlowski
2014-10-07 15:47 ` [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call Krzysztof Kozlowski
2014-10-27 18:32 ` Sebastian Reichel
2014-10-27 18:29 ` [PATCH v2 1/2] power_supply: Add no_thermal property to prevent recursive get_temp calls Sebastian Reichel
-- strict thread matches above, loose matches on Subject: below --
2014-10-13 6:01 [PATCH v2 2/2] power: charger-manager: Avoid recursive thermal get_temp call MyungJoo Ham
2014-10-13 8:58 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).