[PATCH v2] power_supply: fix return value of get_property

[PATCH v2] power_supply: fix return value of get_property

[Rhyland Klein]

power_supply_get_property() should ideally return -EAGAIN if it is
called while the power_supply is being registered. There was no way
previously to determine if use_cnt == 0 meant that the power_supply
wasn't fully registered yet, or if it had already been unregistered.

Add a new boolean to the power_supply struct to simply show if
registration is completed. Lastly, modify the check in
power_supply_show_property() to also ignore -EAGAIN when so it
doesn't complain about not returning the property.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
v2:
 - Modify power_supply_show_property() to not complain if it
   sees a return value of -EAGAIN after calling
   power_supply_get_property().

 drivers/power/power_supply_core.c  | 6 +++++-
 drivers/power/power_supply_sysfs.c | 2 +-
 include/linux/power_supply.h       | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index b13cd074c52a..a39a47672979 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -491,8 +491,11 @@ int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
 {
-	if (atomic_read(&psy->use_cnt) <= 0)
+	if (atomic_read(&psy->use_cnt) <= 0) {
+		if (!psy->initialized)
+			return -EAGAIN;
 		return -ENODEV;
+	}
 
 	return psy->desc->get_property(psy, psp, val);
 }
@@ -775,6 +778,7 @@ __power_supply_register(struct device *parent,
 	if (rc)
 		goto create_triggers_failed;
 
+	psy->initialized = true;
 	/*
 	 * Update use_cnt after any uevents (most notably from device_add()).
 	 * We are here still during driver's probe but
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 80fed98832f9..bcde8d13476a 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -83,7 +83,7 @@ static ssize_t power_supply_show_property(struct device *dev,
 			if (ret == -ENODATA)
 				dev_dbg(dev, "driver has no data for `%s' property\n",
 					attr->attr.name);
-			else if (ret != -ENODEV)
+			else if (ret != -ENODEV && ret != -EAGAIN)
 				dev_err(dev, "driver failed to report `%s' property: %zd\n",
 					attr->attr.name, ret);
 			return ret;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 751061790626..3965503315ef 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -248,6 +248,7 @@ struct power_supply {
 	struct delayed_work deferred_register_work;
 	spinlock_t changed_lock;
 	bool changed;
+	bool initialized;
 	atomic_t use_cnt;
 #ifdef CONFIG_THERMAL
 	struct thermal_zone_device *tzd;
-- 
1.9.1

Re: [PATCH v2] power_supply: fix return value of get_property

[Krzysztof Kozlowski]

On 06/16/2016 12:13 AM, Rhyland Klein wrote:
> power_supply_get_property() should ideally return -EAGAIN if it is
> called while the power_supply is being registered. There was no way
> previously to determine if use_cnt == 0 meant that the power_supply
> wasn't fully registered yet, or if it had already been unregistered.
> 
> Add a new boolean to the power_supply struct to simply show if
> registration is completed. Lastly, modify the check in
> power_supply_show_property() to also ignore -EAGAIN when so it
> doesn't complain about not returning the property.
> 
> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> ---
> v2:
>  - Modify power_supply_show_property() to not complain if it
>    sees a return value of -EAGAIN after calling
>    power_supply_get_property().
> 
>  drivers/power/power_supply_core.c  | 6 +++++-
>  drivers/power/power_supply_sysfs.c | 2 +-
>  include/linux/power_supply.h       | 1 +
>  3 files changed, 7 insertions(+), 2 deletions(-)

I don't like it for two reasons:
1. There is still a short window when the information will be
inaccurate. See comment below.

2. Although the code is not very complicated but it adds another field
and some checks just for differentiating EAGAIN/ENODEV. It is
unnecessary complexity.

> 
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index b13cd074c52a..a39a47672979 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -491,8 +491,11 @@ int power_supply_get_property(struct power_supply *psy,
>  			    enum power_supply_property psp,
>  			    union power_supply_propval *val)
>  {
> -	if (atomic_read(&psy->use_cnt) <= 0)
> +	if (atomic_read(&psy->use_cnt) <= 0) {
> +		if (!psy->initialized)
> +			return -EAGAIN;
>  		return -ENODEV;
> +	}
>  
>  	return psy->desc->get_property(psy, psp, val);
>  }
> @@ -775,6 +778,7 @@ __power_supply_register(struct device *parent,
>  	if (rc)
>  		goto create_triggers_failed;
>  
> +	psy->initialized = true;

If someone calls power_supply_get_property() here, then ENODEV will be
returned which is wrong.

The problem is not solved entirely... I am not convinced that introduced
complexity is worth fixing it.

Best regards,
Krzysztof

Re: [PATCH v2] power_supply: fix return value of get_property

[Rhyland Klein]

On 6/16/2016 4:43 AM, Krzysztof Kozlowski wrote:
> On 06/16/2016 12:13 AM, Rhyland Klein wrote:
>> power_supply_get_property() should ideally return -EAGAIN if it is
>> called while the power_supply is being registered. There was no way
>> previously to determine if use_cnt == 0 meant that the power_supply
>> wasn't fully registered yet, or if it had already been unregistered.
>>
>> Add a new boolean to the power_supply struct to simply show if
>> registration is completed. Lastly, modify the check in
>> power_supply_show_property() to also ignore -EAGAIN when so it
>> doesn't complain about not returning the property.
>>
>> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
>> ---
>> v2:
>>  - Modify power_supply_show_property() to not complain if it
>>    sees a return value of -EAGAIN after calling
>>    power_supply_get_property().
>>
>>  drivers/power/power_supply_core.c  | 6 +++++-
>>  drivers/power/power_supply_sysfs.c | 2 +-
>>  include/linux/power_supply.h       | 1 +
>>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> I don't like it for two reasons:
> 1. There is still a short window when the information will be
> inaccurate. See comment below.
> 
> 2. Although the code is not very complicated but it adds another field
> and some checks just for differentiating EAGAIN/ENODEV. It is
> unnecessary complexity.
> 
>>
>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
>> index b13cd074c52a..a39a47672979 100644
>> --- a/drivers/power/power_supply_core.c
>> +++ b/drivers/power/power_supply_core.c
>> @@ -491,8 +491,11 @@ int power_supply_get_property(struct power_supply *psy,
>>  			    enum power_supply_property psp,
>>  			    union power_supply_propval *val)
>>  {
>> -	if (atomic_read(&psy->use_cnt) <= 0)
>> +	if (atomic_read(&psy->use_cnt) <= 0) {
>> +		if (!psy->initialized)
>> +			return -EAGAIN;
>>  		return -ENODEV;
>> +	}
>>  
>>  	return psy->desc->get_property(psy, psp, val);
>>  }
>> @@ -775,6 +778,7 @@ __power_supply_register(struct device *parent,
>>  	if (rc)
>>  		goto create_triggers_failed;
>>  
>> +	psy->initialized = true;
> 
> If someone calls power_supply_get_property() here, then ENODEV will be
> returned which is wrong.
> 
> The problem is not solved entirely... I am not convinced that introduced
> complexity is worth fixing it.
> 

Right now, without this patch, this causes the thermal function
"update_temperature" in:

thermal_zone_device_register->
  thermal_zone_device_update->
    update_temperature
      (->thermal_zone_get_temp() from the original stack)

to print a warning as it sees ret != -EAGAIN. This causes a warning
"failed to read out thermal zone". I think the idea there is if anything
other than "try again" it complains. While this doesn't cause functional
problems, I do think avoid the warning is worth while.

I think that there is an onus on the power_supply code to be accurate in
its return codes, and EAGAIN is the correct one we should be returning.
I don't see how someone would call power_supply_get_property, but I
agree there is the possibility that if someone did call there, that it
could return the wrong value.

We could wrap the setting of initialized and use_cnt inside a mutex,
which should prevent anyone calling anything in between if we wanted to
completely plug that hole. I am not fond of the idea of adding a struct
member for such a small, specific case, but as we found before, I don't
think there is another way to differentiate otherwise.

-rhyland


-- 
nvpublic

Re: [PATCH v2] power_supply: fix return value of get_property

[Rhyland Klein]

On 6/16/2016 11:40 AM, Rhyland Klein wrote:
> On 6/16/2016 4:43 AM, Krzysztof Kozlowski wrote:
>> On 06/16/2016 12:13 AM, Rhyland Klein wrote:
>>> power_supply_get_property() should ideally return -EAGAIN if it is
>>> called while the power_supply is being registered. There was no way
>>> previously to determine if use_cnt == 0 meant that the power_supply
>>> wasn't fully registered yet, or if it had already been unregistered.
>>>
>>> Add a new boolean to the power_supply struct to simply show if
>>> registration is completed. Lastly, modify the check in
>>> power_supply_show_property() to also ignore -EAGAIN when so it
>>> doesn't complain about not returning the property.
>>>
>>> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
>>> ---
>>> v2:
>>>  - Modify power_supply_show_property() to not complain if it
>>>    sees a return value of -EAGAIN after calling
>>>    power_supply_get_property().
>>>
>>>  drivers/power/power_supply_core.c  | 6 +++++-
>>>  drivers/power/power_supply_sysfs.c | 2 +-
>>>  include/linux/power_supply.h       | 1 +
>>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> I don't like it for two reasons:
>> 1. There is still a short window when the information will be
>> inaccurate. See comment below.
>>
>> 2. Although the code is not very complicated but it adds another field
>> and some checks just for differentiating EAGAIN/ENODEV. It is
>> unnecessary complexity.
>>
>>>
>>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
>>> index b13cd074c52a..a39a47672979 100644
>>> --- a/drivers/power/power_supply_core.c
>>> +++ b/drivers/power/power_supply_core.c
>>> @@ -491,8 +491,11 @@ int power_supply_get_property(struct power_supply *psy,
>>>  			    enum power_supply_property psp,
>>>  			    union power_supply_propval *val)
>>>  {
>>> -	if (atomic_read(&psy->use_cnt) <= 0)
>>> +	if (atomic_read(&psy->use_cnt) <= 0) {
>>> +		if (!psy->initialized)
>>> +			return -EAGAIN;
>>>  		return -ENODEV;
>>> +	}
>>>  
>>>  	return psy->desc->get_property(psy, psp, val);
>>>  }
>>> @@ -775,6 +778,7 @@ __power_supply_register(struct device *parent,
>>>  	if (rc)
>>>  		goto create_triggers_failed;
>>>  
>>> +	psy->initialized = true;
>>
>> If someone calls power_supply_get_property() here, then ENODEV will be
>> returned which is wrong.
>>
>> The problem is not solved entirely... I am not convinced that introduced
>> complexity is worth fixing it.
>>
> 
> Right now, without this patch, this causes the thermal function
> "update_temperature" in:
> 
> thermal_zone_device_register->
>   thermal_zone_device_update->
>     update_temperature
>       (->thermal_zone_get_temp() from the original stack)
> 
> to print a warning as it sees ret != -EAGAIN. This causes a warning
> "failed to read out thermal zone". I think the idea there is if anything
> other than "try again" it complains. While this doesn't cause functional
> problems, I do think avoid the warning is worth while.
> 
> I think that there is an onus on the power_supply code to be accurate in
> its return codes, and EAGAIN is the correct one we should be returning.
> I don't see how someone would call power_supply_get_property, but I
> agree there is the possibility that if someone did call there, that it
> could return the wrong value.
> 
> We could wrap the setting of initialized and use_cnt inside a mutex,
> which should prevent anyone calling anything in between if we wanted to
> completely plug that hole. I am not fond of the idea of adding a struct
> member for such a small, specific case, but as we found before, I don't
> think there is another way to differentiate otherwise.
> 

Sebastian, do you have an opinion on this?

-rhyland


-- 
nvpublic

Re: [PATCH v2] power_supply: fix return value of get_property

[Sebastian Reichel]

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

Hi,

On Tue, Jun 21, 2016 at 02:08:05PM -0400, Rhyland Klein wrote:
> On 6/16/2016 11:40 AM, Rhyland Klein wrote:
> > On 6/16/2016 4:43 AM, Krzysztof Kozlowski wrote:
> >> On 06/16/2016 12:13 AM, Rhyland Klein wrote:
> >>> power_supply_get_property() should ideally return -EAGAIN if it is
> >>> called while the power_supply is being registered. There was no way
> >>> previously to determine if use_cnt == 0 meant that the power_supply
> >>> wasn't fully registered yet, or if it had already been unregistered.
> >>>
> >>> Add a new boolean to the power_supply struct to simply show if
> >>> registration is completed. Lastly, modify the check in
> >>> power_supply_show_property() to also ignore -EAGAIN when so it
> >>> doesn't complain about not returning the property.
> >>>
> >>> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> >>> ---
> >>> v2:
> >>>  - Modify power_supply_show_property() to not complain if it
> >>>    sees a return value of -EAGAIN after calling
> >>>    power_supply_get_property().
> >>>
> >>>  drivers/power/power_supply_core.c  | 6 +++++-
> >>>  drivers/power/power_supply_sysfs.c | 2 +-
> >>>  include/linux/power_supply.h       | 1 +
> >>>  3 files changed, 7 insertions(+), 2 deletions(-)
> >>
> >> I don't like it for two reasons:
> >> 1. There is still a short window when the information will be
> >> inaccurate. See comment below.
> >>
> >> 2. Although the code is not very complicated but it adds another field
> >> and some checks just for differentiating EAGAIN/ENODEV. It is
> >> unnecessary complexity.
> >>
> >>>
> >>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> >>> index b13cd074c52a..a39a47672979 100644
> >>> --- a/drivers/power/power_supply_core.c
> >>> +++ b/drivers/power/power_supply_core.c
> >>> @@ -491,8 +491,11 @@ int power_supply_get_property(struct power_supply *psy,
> >>>  			    enum power_supply_property psp,
> >>>  			    union power_supply_propval *val)
> >>>  {
> >>> -	if (atomic_read(&psy->use_cnt) <= 0)
> >>> +	if (atomic_read(&psy->use_cnt) <= 0) {
> >>> +		if (!psy->initialized)
> >>> +			return -EAGAIN;
> >>>  		return -ENODEV;
> >>> +	}
> >>>  
> >>>  	return psy->desc->get_property(psy, psp, val);
> >>>  }
> >>> @@ -775,6 +778,7 @@ __power_supply_register(struct device *parent,
> >>>  	if (rc)
> >>>  		goto create_triggers_failed;
> >>>  
> >>> +	psy->initialized = true;
> >>
> >> If someone calls power_supply_get_property() here, then ENODEV will be
> >> returned which is wrong.
> >>
> >> The problem is not solved entirely... I am not convinced that introduced
> >> complexity is worth fixing it.
> >>
> > 
> > Right now, without this patch, this causes the thermal function
> > "update_temperature" in:
> > 
> > thermal_zone_device_register->
> >   thermal_zone_device_update->
> >     update_temperature
> >       (->thermal_zone_get_temp() from the original stack)
> > 
> > to print a warning as it sees ret != -EAGAIN. This causes a warning
> > "failed to read out thermal zone". I think the idea there is if anything
> > other than "try again" it complains. While this doesn't cause functional
> > problems, I do think avoid the warning is worth while.
> > 
> > I think that there is an onus on the power_supply code to be accurate in
> > its return codes, and EAGAIN is the correct one we should be returning.
> > I don't see how someone would call power_supply_get_property, but I
> > agree there is the possibility that if someone did call there, that it
> > could return the wrong value.
> > 
> > We could wrap the setting of initialized and use_cnt inside a mutex,
> > which should prevent anyone calling anything in between if we wanted to
> > completely plug that hole. I am not fond of the idea of adding a struct
> > member for such a small, specific case, but as we found before, I don't
> > think there is another way to differentiate otherwise.
> > 
> 
> Sebastian, do you have an opinion on this?

Instead of adding a mutex, just set "psy->initialized = true" after
increment of use_cnt. I'm fine with the patch otherwise.

-- Sebastian

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