Re: [PATCH v2] power: supply: charger-manager: Fix info message in check_charging_duration()

[Jonathan Bakker]

On 2020-09-02 9:46 a.m., Colin Ian King wrote:
> On 02/09/2020 17:43, Gustavo A. R. Silva wrote:
>> On Wed, Sep 02, 2020 at 09:29:31AM -0700, Randy Dunlap wrote:
>>> On 9/2/20 9:23 AM, Gustavo A. R. Silva wrote:
>>>> A few months ago, commit e132fc6bb89b ("power: supply: charger-manager: Make decisions focussed on battery status")
>>>> changed the expression in the if statement from "duration > desc->discharging_max_duration_ms"
>>>> to "duration > desc->charging_max_duration_ms", but the arguments for dev_info() were left unchanged.
>>>> Apparently, due to a copy-paste error.
>>>>
>>>> Fix this by using the proper arguments for dev_info().
>>>>
>>>> Also, while there, replace "exceed" with "exceeds", for both messages.
>>>>
>>>> Addresses-Coverity-ID: 1496803 ("Copy-paste error")
>>>> Fixes: e132fc6bb89b ("power: supply: charger-manager: Make decisions focussed on battery status")
>>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>>  -  Replace "exceed" with "exceeds"
>>>>
>>>>  drivers/power/supply/charger-manager.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c
>>>> index 07992821e252..a6d5dbd55e37 100644
>>>> --- a/drivers/power/supply/charger-manager.c
>>>> +++ b/drivers/power/supply/charger-manager.c
>>>> @@ -464,7 +464,7 @@ static int check_charging_duration(struct charger_manager *cm)
>>>>  		duration = curr - cm->charging_start_time;
>>>>  
>>>>  		if (duration > desc->charging_max_duration_ms) {
>>>> -			dev_info(cm->dev, "Charging duration exceed %ums\n",
>>>> +			dev_info(cm->dev, "Charging duration exceeds %ums\n",
>>>>  				 desc->charging_max_duration_ms);
>>>>  			ret = true;
>>>>  		}
>>>> @@ -472,8 +472,8 @@ static int check_charging_duration(struct charger_manager *cm)
>>>>  		duration = curr - cm->charging_end_time;
>>>>  
>>>>  		if (duration > desc->charging_max_duration_ms) {
>>>> -			dev_info(cm->dev, "Discharging duration exceed %ums\n",
>>>> -				 desc->discharging_max_duration_ms);
>>>> +			dev_info(cm->dev, "Charging duration exceeds %ums\n",
>>>> +				 desc->charging_max_duration_ms);
>>>>  			ret = true;
>>>>  		}
>>>>  	}
>>>>
>>>
>>> Hi,
>>>
>>> It looks to me like the second block (else if) should be about discharging,
>>> not charging, more like Colin King's patch had it:
>>>
>>
>> I had the same impression for a moment, but what makes me think this is
>> more about charging than discharging, is this line:
>>
>> 471         } else if (cm->battery_status == POWER_SUPPLY_STATUS_NOT_CHARGING) {
>>
>> which was introduced by the same commit:
>>
>> e132fc6bb89b ("power: supply: charger-manager: Make decisions focussed on battery status")
>>
>> let's find out... :)
> 
> It's a 50/50 bet :-)

I believe it should be as Colin King's patch had it, ie

-		if (duration > desc->charging_max_duration_ms) {
+		if (duration > desc->discharging_max_duration_ms) {

The battery_status as POWER_SUPPLY_STATUS_NOT_CHARGING only occurs when we would be charging,
except that we're above or below the allowable temperature readings.  This retains the logic
of prior to e132fc6bb89b ("power: supply: charger-manager: Make decisions
focussed on battery status")

Plus, this is the only place discharging_max_duration_ms is actually used.  It appears to
be the time that the battery can discharge (while being plugged in but out of temperature range)
before restarting charging is tried (which will probably then fail on the next monitor session
due to being above temp).

Thanks,
Jonathan

> 
>>
>> Thanks
>> --
>> Gustavo
>>
>