[PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Rhyland Klein]

Change power_supply_read_temp() to use power_supply_get_property()
so that it will check the use_cnt and ensure it is > 0. The use_cnt
will be incremented at the end of __power_supply_register, so this
will block to case where get_property can be called before the supply
is fully registered. This fixes the issue show in the stack below:

[    1.452598] power_supply_read_temp+0x78/0x80
[    1.458680] thermal_zone_get_temp+0x5c/0x11c
[    1.464765] thermal_zone_device_update+0x34/0xb4
[    1.471195] thermal_zone_device_register+0x87c/0x8cc
[    1.477974] __power_supply_register+0x364/0x424
[    1.484317] power_supply_register_no_ws+0x10/0x18
[    1.490833] bq27xxx_battery_setup+0x10c/0x164
[    1.497003] bq27xxx_battery_i2c_probe+0xd0/0x1b0
[    1.503435] i2c_device_probe+0x174/0x240
[    1.509172] driver_probe_device+0x1fc/0x29c
[    1.515167] __driver_attach+0xa4/0xa8
[    1.520643] bus_for_each_dev+0x58/0x98
[    1.526204] driver_attach+0x20/0x28
[    1.531505] bus_add_driver+0x1c8/0x22c
[    1.537067] driver_register+0x68/0x108
[    1.542630] i2c_register_driver+0x38/0x7c
[    1.548457] bq27xxx_battery_i2c_driver_init+0x18/0x20
[    1.555321] do_one_initcall+0x38/0x12c
[    1.560886] kernel_init_freeable+0x148/0x1ec
[    1.566972] kernel_init+0x10/0xfc
[    1.572101] ret_from_fork+0x10/0x40

Also make the same change to ps_get_max_charge_cntl_limit() and
ps_get_cur_chrage_cntl_limit() to be safe. Lastly, change the return
value of power_supply_get_property() to -EAGAIN from -ENODEV if
use_cnt <= 0.

Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
Cc: stable@vger.kernel.org
Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
v4:
 - Fixed return check conditions
 - Removed change to power_supply_get_property return code
 - removed whitespace change

v3:
 - Changed calls to ->get_property() to use common
   power_supply_get_property()
 - reworded description, added "Fixes" line
 - Changed return value of power_supply_get_property() to -EAGAIN

v2:
 - Added cc stable
 - changed return to -EAGAIN in case of use_cnt < 1
 - Removed WARNING
 - return value check added in additional patch:
   https://patchwork.kernel.org/patch/9158805/

 drivers/power/power_supply_core.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 456987c88baa..b13cd074c52a 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -565,11 +565,12 @@ static int power_supply_read_temp(struct thermal_zone_device *tzd,
 
 	WARN_ON(tzd == NULL);
 	psy = tzd->devdata;
-	ret = psy->desc->get_property(psy, POWER_SUPPLY_PROP_TEMP, &val);
+	ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_TEMP, &val);
+	if (ret)
+		return ret;
 
 	/* Convert tenths of degree Celsius to milli degree Celsius. */
-	if (!ret)
-		*temp = val.intval * 100;
+	*temp = val.intval * 100;
 
 	return ret;
 }
@@ -612,10 +613,12 @@ static int ps_get_max_charge_cntl_limit(struct thermal_cooling_device *tcd,
 	int ret;
 
 	psy = tcd->devdata;
-	ret = psy->desc->get_property(psy,
-		POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, &val);
-	if (!ret)
-		*state = val.intval;
+	ret = power_supply_get_property(psy,
+			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, &val);
+	if (ret)
+		return ret;
+
+	*state = val.intval;
 
 	return ret;
 }
@@ -628,10 +631,12 @@ static int ps_get_cur_chrage_cntl_limit(struct thermal_cooling_device *tcd,
 	int ret;
 
 	psy = tcd->devdata;
-	ret = psy->desc->get_property(psy,
-		POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
-	if (!ret)
-		*state = val.intval;
+	ret = power_supply_get_property(psy,
+			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
+	if (ret)
+		return ret;
+
+	*state = val.intval;
 
 	return ret;
 }
-- 
1.9.1

Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Krzysztof Kozlowski]

On 06/09/2016 11:28 PM, Rhyland Klein wrote:
> Change power_supply_read_temp() to use power_supply_get_property()
> so that it will check the use_cnt and ensure it is > 0. The use_cnt
> will be incremented at the end of __power_supply_register, so this
> will block to case where get_property can be called before the supply
> is fully registered. This fixes the issue show in the stack below:
> 
> [    1.452598] power_supply_read_temp+0x78/0x80
> [    1.458680] thermal_zone_get_temp+0x5c/0x11c
> [    1.464765] thermal_zone_device_update+0x34/0xb4
> [    1.471195] thermal_zone_device_register+0x87c/0x8cc
> [    1.477974] __power_supply_register+0x364/0x424
> [    1.484317] power_supply_register_no_ws+0x10/0x18
> [    1.490833] bq27xxx_battery_setup+0x10c/0x164
> [    1.497003] bq27xxx_battery_i2c_probe+0xd0/0x1b0
> [    1.503435] i2c_device_probe+0x174/0x240
> [    1.509172] driver_probe_device+0x1fc/0x29c
> [    1.515167] __driver_attach+0xa4/0xa8
> [    1.520643] bus_for_each_dev+0x58/0x98
> [    1.526204] driver_attach+0x20/0x28
> [    1.531505] bus_add_driver+0x1c8/0x22c
> [    1.537067] driver_register+0x68/0x108
> [    1.542630] i2c_register_driver+0x38/0x7c
> [    1.548457] bq27xxx_battery_i2c_driver_init+0x18/0x20
> [    1.555321] do_one_initcall+0x38/0x12c
> [    1.560886] kernel_init_freeable+0x148/0x1ec
> [    1.566972] kernel_init+0x10/0xfc
> [    1.572101] ret_from_fork+0x10/0x40
> 
> Also make the same change to ps_get_max_charge_cntl_limit() and
> ps_get_cur_chrage_cntl_limit() to be safe. Lastly, change the return
> value of power_supply_get_property() to -EAGAIN from -ENODEV if
> use_cnt <= 0.
> 
> Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> ---
> v4:
>  - Fixed return check conditions
>  - Removed change to power_supply_get_property return code
>  - removed whitespace change
> 
> v3:
>  - Changed calls to ->get_property() to use common
>    power_supply_get_property()
>  - reworded description, added "Fixes" line
>  - Changed return value of power_supply_get_property() to -EAGAIN
> 
> v2:
>  - Added cc stable
>  - changed return to -EAGAIN in case of use_cnt < 1
>  - Removed WARNING
>  - return value check added in additional patch:
>    https://patchwork.kernel.org/patch/9158805/
> 
>  drivers/power/power_supply_core.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)

Looks okay:

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

Best regards,
Krzysztof

Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Rhyland Klein]

On 6/10/2016 5:43 AM, Krzysztof Kozlowski wrote:
> On 06/09/2016 11:28 PM, Rhyland Klein wrote:
>> Change power_supply_read_temp() to use power_supply_get_property()
>> so that it will check the use_cnt and ensure it is > 0. The use_cnt
>> will be incremented at the end of __power_supply_register, so this
>> will block to case where get_property can be called before the supply
>> is fully registered. This fixes the issue show in the stack below:
>>
>> [    1.452598] power_supply_read_temp+0x78/0x80
>> [    1.458680] thermal_zone_get_temp+0x5c/0x11c
>> [    1.464765] thermal_zone_device_update+0x34/0xb4
>> [    1.471195] thermal_zone_device_register+0x87c/0x8cc
>> [    1.477974] __power_supply_register+0x364/0x424
>> [    1.484317] power_supply_register_no_ws+0x10/0x18
>> [    1.490833] bq27xxx_battery_setup+0x10c/0x164
>> [    1.497003] bq27xxx_battery_i2c_probe+0xd0/0x1b0
>> [    1.503435] i2c_device_probe+0x174/0x240
>> [    1.509172] driver_probe_device+0x1fc/0x29c
>> [    1.515167] __driver_attach+0xa4/0xa8
>> [    1.520643] bus_for_each_dev+0x58/0x98
>> [    1.526204] driver_attach+0x20/0x28
>> [    1.531505] bus_add_driver+0x1c8/0x22c
>> [    1.537067] driver_register+0x68/0x108
>> [    1.542630] i2c_register_driver+0x38/0x7c
>> [    1.548457] bq27xxx_battery_i2c_driver_init+0x18/0x20
>> [    1.555321] do_one_initcall+0x38/0x12c
>> [    1.560886] kernel_init_freeable+0x148/0x1ec
>> [    1.566972] kernel_init+0x10/0xfc
>> [    1.572101] ret_from_fork+0x10/0x40
>>
>> Also make the same change to ps_get_max_charge_cntl_limit() and
>> ps_get_cur_chrage_cntl_limit() to be safe. Lastly, change the return
>> value of power_supply_get_property() to -EAGAIN from -ENODEV if
>> use_cnt <= 0.
>>
>> Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
>> ---
>> v4:
>>  - Fixed return check conditions
>>  - Removed change to power_supply_get_property return code
>>  - removed whitespace change
>>
>> v3:
>>  - Changed calls to ->get_property() to use common
>>    power_supply_get_property()
>>  - reworded description, added "Fixes" line
>>  - Changed return value of power_supply_get_property() to -EAGAIN
>>
>> v2:
>>  - Added cc stable
>>  - changed return to -EAGAIN in case of use_cnt < 1
>>  - Removed WARNING
>>  - return value check added in additional patch:
>>    https://patchwork.kernel.org/patch/9158805/
>>
>>  drivers/power/power_supply_core.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> Looks okay:
> 
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> Best regards,
> Krzysztof
> 
> 

I sent another patch which is a follow up to this one, which changes the
return value of power_supply_get_property() to return -EAGAIN if it is
called during __power_supply_register(). Turns out the thermal framework
(at least) will treat EAGAIN differently and since we were on the fence
about what to return, it made sense to go ahead and do that to make
everyone happy.

https://lkml.org/lkml/2016/6/13/943

-rhyland


-- 
nvpublic

Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Rhyland Klein]

On 6/9/2016 5:28 PM, Rhyland Klein wrote:
> Change power_supply_read_temp() to use power_supply_get_property()
> so that it will check the use_cnt and ensure it is > 0. The use_cnt
> will be incremented at the end of __power_supply_register, so this
> will block to case where get_property can be called before the supply
> is fully registered. This fixes the issue show in the stack below:
> 
> [    1.452598] power_supply_read_temp+0x78/0x80
> [    1.458680] thermal_zone_get_temp+0x5c/0x11c
> [    1.464765] thermal_zone_device_update+0x34/0xb4
> [    1.471195] thermal_zone_device_register+0x87c/0x8cc
> [    1.477974] __power_supply_register+0x364/0x424
> [    1.484317] power_supply_register_no_ws+0x10/0x18
> [    1.490833] bq27xxx_battery_setup+0x10c/0x164
> [    1.497003] bq27xxx_battery_i2c_probe+0xd0/0x1b0
> [    1.503435] i2c_device_probe+0x174/0x240
> [    1.509172] driver_probe_device+0x1fc/0x29c
> [    1.515167] __driver_attach+0xa4/0xa8
> [    1.520643] bus_for_each_dev+0x58/0x98
> [    1.526204] driver_attach+0x20/0x28
> [    1.531505] bus_add_driver+0x1c8/0x22c
> [    1.537067] driver_register+0x68/0x108
> [    1.542630] i2c_register_driver+0x38/0x7c
> [    1.548457] bq27xxx_battery_i2c_driver_init+0x18/0x20
> [    1.555321] do_one_initcall+0x38/0x12c
> [    1.560886] kernel_init_freeable+0x148/0x1ec
> [    1.566972] kernel_init+0x10/0xfc
> [    1.572101] ret_from_fork+0x10/0x40
> 
> Also make the same change to ps_get_max_charge_cntl_limit() and
> ps_get_cur_chrage_cntl_limit() to be safe. Lastly, change the return
> value of power_supply_get_property() to -EAGAIN from -ENODEV if
> use_cnt <= 0.
> 
> Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> ---
> v4:
>  - Fixed return check conditions
>  - Removed change to power_supply_get_property return code
>  - removed whitespace change
> 
> v3:
>  - Changed calls to ->get_property() to use common
>    power_supply_get_property()
>  - reworded description, added "Fixes" line
>  - Changed return value of power_supply_get_property() to -EAGAIN
> 
> v2:
>  - Added cc stable
>  - changed return to -EAGAIN in case of use_cnt < 1
>  - Removed WARNING
>  - return value check added in additional patch:
>    https://patchwork.kernel.org/patch/9158805/
> 
>  drivers/power/power_supply_core.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 456987c88baa..b13cd074c52a 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -565,11 +565,12 @@ static int power_supply_read_temp(struct thermal_zone_device *tzd,
>  
>  	WARN_ON(tzd == NULL);
>  	psy = tzd->devdata;
> -	ret = psy->desc->get_property(psy, POWER_SUPPLY_PROP_TEMP, &val);
> +	ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_TEMP, &val);
> +	if (ret)
> +		return ret;
>  
>  	/* Convert tenths of degree Celsius to milli degree Celsius. */
> -	if (!ret)
> -		*temp = val.intval * 100;
> +	*temp = val.intval * 100;
>  
>  	return ret;
>  }
> @@ -612,10 +613,12 @@ static int ps_get_max_charge_cntl_limit(struct thermal_cooling_device *tcd,
>  	int ret;
>  
>  	psy = tcd->devdata;
> -	ret = psy->desc->get_property(psy,
> -		POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, &val);
> -	if (!ret)
> -		*state = val.intval;
> +	ret = power_supply_get_property(psy,
> +			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, &val);
> +	if (ret)
> +		return ret;
> +
> +	*state = val.intval;
>  
>  	return ret;
>  }
> @@ -628,10 +631,12 @@ static int ps_get_cur_chrage_cntl_limit(struct thermal_cooling_device *tcd,
>  	int ret;
>  
>  	psy = tcd->devdata;
> -	ret = psy->desc->get_property(psy,
> -		POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
> -	if (!ret)
> -		*state = val.intval;
> +	ret = power_supply_get_property(psy,
> +			POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT, &val);
> +	if (ret)
> +		return ret;
> +
> +	*state = val.intval;
>  
>  	return ret;
>  }
> 

Sebastian, might this be accepted soon? We can't enable the bq27xxx
driver for Tegra210 Smaug without running into the crash this prevents.

-rhyland

-- 
nvpublic

Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Sebastian Reichel]

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

Hi Rhyland,

On Tue, Jun 21, 2016 at 02:06:55PM -0400, Rhyland Klein wrote:
> Sebastian, might this be accepted soon? We can't enable the bq27xxx
> driver for Tegra210 Smaug without running into the crash this prevents.

Yes, I have already queued this in my fixes branch some days ago,
but did not yet pushed it. It's pushed out now:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=fixes&id=5bc28b93a36e3cb3acc2870fb75cb6ffb182fece

-- Sebastian

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

Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Rhyland Klein]

On 6/22/2016 10:23 AM, Sebastian Reichel wrote:
> * PGP Signed by an unknown key
> 
> Hi Rhyland,
> 
> On Tue, Jun 21, 2016 at 02:06:55PM -0400, Rhyland Klein wrote:
>> Sebastian, might this be accepted soon? We can't enable the bq27xxx
>> driver for Tegra210 Smaug without running into the crash this prevents.
> 
> Yes, I have already queued this in my fixes branch some days ago,
> but did not yet pushed it. It's pushed out now:
> 
> https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=fixes&id=5bc28b93a36e3cb3acc2870fb75cb6ffb182fece
> 

Thanks!

-rhyland

-- 
nvpublic

Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Rhyland Klein]

On 6/22/2016 10:23 AM, Sebastian Reichel wrote:
> * PGP Signed by an unknown key
> 
> Hi Rhyland,
> 
> On Tue, Jun 21, 2016 at 02:06:55PM -0400, Rhyland Klein wrote:
>> Sebastian, might this be accepted soon? We can't enable the bq27xxx
>> driver for Tegra210 Smaug without running into the crash this prevents.
> 
> Yes, I have already queued this in my fixes branch some days ago,
> but did not yet pushed it. It's pushed out now:
> 
> https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=fixes&id=5bc28b93a36e3cb3acc2870fb75cb6ffb182fece
> 
> -- Sebastian
> 
> * Unknown Key
> * 0xC83BFA9A
> 

Is this also going to go into linux-next?

-rhyland

-- 
nvpublic

Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Sebastian Reichel]

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

Hi,

On Thu, Jun 23, 2016 at 01:43:47PM -0400, Rhyland Klein wrote:
> On 6/22/2016 10:23 AM, Sebastian Reichel wrote:
> Is this also going to go into linux-next?

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=02184c60eba8491ea574cd17b8ba766c86d468f2

-- Sebastian

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

Re: [PATCH v4] power_supply: power_supply_read_temp only if use_cnt > 0

[Rhyland Klein]

On 6/28/2016 2:20 PM, Sebastian Reichel wrote:
> * PGP Signed by an unknown key
> 
> Hi,
> 
> On Thu, Jun 23, 2016 at 01:43:47PM -0400, Rhyland Klein wrote:
>> On 6/22/2016 10:23 AM, Sebastian Reichel wrote:
>> Is this also going to go into linux-next?
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=02184c60eba8491ea574cd17b8ba766c86d468f2
> 

Thanks!

-rhyland


-- 
nvpublic