Message ID | 20190816075842.27333-1-committed@heine.so (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | power: supply: sbs-battery: only return health when battery present | expand |
On Fri, Aug 16, 2019 at 09:58:42AM +0200, Michael Nosthoff wrote: > when the battery is set to sbs-mode and no gpio detection is enabled > "health" is always returning a value even when the battery is not present. > All other fields return "not present". > This leads to a scenario where the driver is constantly switching between > "present" and "not present" state. This generates a lot of constant > traffic on the i2c. That depends on how often you're checking the "health" attribute, doesn't it? But anyway, the bug is real. > This commit changes the response of "health" to an error when the battery > is not responding leading to a consistent "not present" state. Ack, and thanks for the fix. > Fixes: 76b16f4cdfb8 ("power: supply: sbs-battery: don't assume > MANUFACTURER_DATA formats") > > Signed-off-by: Michael Nosthoff <committed@heine.so> > Cc: Brian Norris <briannorris@chromium.org> > Cc: <stable@vger.kernel.org> Reviewed-by: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> > --- > drivers/power/supply/sbs-battery.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c > index 2e86cc1e0e35..f8d74e9f7931 100644 > --- a/drivers/power/supply/sbs-battery.c > +++ b/drivers/power/supply/sbs-battery.c > @@ -314,17 +314,22 @@ static int sbs_get_battery_presence_and_health( > { > int ret; > > - if (psp == POWER_SUPPLY_PROP_PRESENT) { > - /* Dummy command; if it succeeds, battery is present. */ > - ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); > - if (ret < 0) > - val->intval = 0; /* battery disconnected */ > - else > - val->intval = 1; /* battery present */ > - } else { /* POWER_SUPPLY_PROP_HEALTH */ > + /* Dummy command; if it succeeds, battery is present. */ > + ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); > + > + if (ret < 0) { /* battery not present*/ > + if (psp == POWER_SUPPLY_PROP_PRESENT) { > + val->intval = 0; > + return 0; Technically, you don't need the 'return 0' (and if we care about symmetry: the TI version doesn't), since the caller knows that "not present" will yield errors. I'm not sure which version makes more sense. > + } > + return ret; > + } > + > + if (psp == POWER_SUPPLY_PROP_PRESENT) > + val->intval = 1; /* battery present */ > + else /* POWER_SUPPLY_PROP_HEALTH */ > /* SBS spec doesn't have a general health command. */ > val->intval = POWER_SUPPLY_HEALTH_UNKNOWN; > - } > > return 0; > } > @@ -626,6 +631,8 @@ static int sbs_get_property(struct power_supply *psy, > else > ret = sbs_get_battery_presence_and_health(client, psp, > val); > + > + /* this can only be true if no gpio is used */ > if (psp == POWER_SUPPLY_PROP_PRESENT) > return 0; > break; > -- > 2.20.1 >
+ Sebastian I'm not sure if he expects to be on the To/Cc, but that usually is the way to get a maintainer to take your patch. Brian On Wed, Aug 21, 2019 at 06:46:55PM -0700, Brian Norris wrote: > On Fri, Aug 16, 2019 at 09:58:42AM +0200, Michael Nosthoff wrote: > > when the battery is set to sbs-mode and no gpio detection is enabled > > "health" is always returning a value even when the battery is not present. > > All other fields return "not present". > > This leads to a scenario where the driver is constantly switching between > > "present" and "not present" state. This generates a lot of constant > > traffic on the i2c. > > That depends on how often you're checking the "health" attribute, > doesn't it? But anyway, the bug is real. > > > This commit changes the response of "health" to an error when the battery > > is not responding leading to a consistent "not present" state. > > Ack, and thanks for the fix. > > > Fixes: 76b16f4cdfb8 ("power: supply: sbs-battery: don't assume > > MANUFACTURER_DATA formats") > > > > Signed-off-by: Michael Nosthoff <committed@heine.so> > > Cc: Brian Norris <briannorris@chromium.org> > > Cc: <stable@vger.kernel.org> > > Reviewed-by: Brian Norris <briannorris@chromium.org> > Tested-by: Brian Norris <briannorris@chromium.org> > > > --- > > drivers/power/supply/sbs-battery.c | 25 ++++++++++++++++--------- > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c > > index 2e86cc1e0e35..f8d74e9f7931 100644 > > --- a/drivers/power/supply/sbs-battery.c > > +++ b/drivers/power/supply/sbs-battery.c > > @@ -314,17 +314,22 @@ static int sbs_get_battery_presence_and_health( > > { > > int ret; > > > > - if (psp == POWER_SUPPLY_PROP_PRESENT) { > > - /* Dummy command; if it succeeds, battery is present. */ > > - ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); > > - if (ret < 0) > > - val->intval = 0; /* battery disconnected */ > > - else > > - val->intval = 1; /* battery present */ > > - } else { /* POWER_SUPPLY_PROP_HEALTH */ > > + /* Dummy command; if it succeeds, battery is present. */ > > + ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); > > + > > + if (ret < 0) { /* battery not present*/ > > + if (psp == POWER_SUPPLY_PROP_PRESENT) { > > + val->intval = 0; > > + return 0; > > Technically, you don't need the 'return 0' (and if we care about > symmetry: the TI version doesn't), since the caller knows that "not > present" will yield errors. I'm not sure which version makes more sense. > > > + } > > + return ret; > > + } > > + > > + if (psp == POWER_SUPPLY_PROP_PRESENT) > > + val->intval = 1; /* battery present */ > > + else /* POWER_SUPPLY_PROP_HEALTH */ > > /* SBS spec doesn't have a general health command. */ > > val->intval = POWER_SUPPLY_HEALTH_UNKNOWN; > > - } > > > > return 0; > > } > > @@ -626,6 +631,8 @@ static int sbs_get_property(struct power_supply *psy, > > else > > ret = sbs_get_battery_presence_and_health(client, psp, > > val); > > + > > + /* this can only be true if no gpio is used */ > > if (psp == POWER_SUPPLY_PROP_PRESENT) > > return 0; > > break; > > -- > > 2.20.1 > >
Brian Norris wrote: > On Fri, Aug 16, 2019 at 09:58:42AM +0200, Michael Nosthoff wrote: >> when the battery is set to sbs-mode and no gpio detection is enabled >> "health" is always returning a value even when the battery is not present. >> All other fields return "not present". >> This leads to a scenario where the driver is constantly switching between >> "present" and "not present" state. This generates a lot of constant >> traffic on the i2c. > > That depends on how often you're checking the "health" attribute, > doesn't it? But anyway, the bug is real. At least on my Hardware I had constant traffic from the moment the device was probed. I have no userland process accessing the device. I'm guessing that it has something todo with the call to 'power_supply_changed'. This done at the end of 'sbs_get_property' if the presence state changed and no gpio is used. I suspect it triggers a readout of all the properties and leads to this endless loop? >> This commit changes the response of "health" to an error when the battery >> is not responding leading to a consistent "not present" state. > > Ack, and thanks for the fix. > >> Fixes: 76b16f4cdfb8 ("power: supply: sbs-battery: don't assume >> MANUFACTURER_DATA formats") >> >> Signed-off-by: Michael Nosthoff<committed@heine.so> >> Cc: Brian Norris<briannorris@chromium.org> >> Cc:<stable@vger.kernel.org> > > Reviewed-by: Brian Norris<briannorris@chromium.org> > Tested-by: Brian Norris<briannorris@chromium.org> thanks for review! > >> --- >> drivers/power/supply/sbs-battery.c | 25 ++++++++++++++++--------- >> 1 file changed, 16 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c >> index 2e86cc1e0e35..f8d74e9f7931 100644 >> --- a/drivers/power/supply/sbs-battery.c >> +++ b/drivers/power/supply/sbs-battery.c >> @@ -314,17 +314,22 @@ static int sbs_get_battery_presence_and_health( >> { >> int ret; >> >> - if (psp == POWER_SUPPLY_PROP_PRESENT) { >> - /* Dummy command; if it succeeds, battery is present. */ >> - ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); >> - if (ret< 0) >> - val->intval = 0; /* battery disconnected */ >> - else >> - val->intval = 1; /* battery present */ >> - } else { /* POWER_SUPPLY_PROP_HEALTH */ >> + /* Dummy command; if it succeeds, battery is present. */ >> + ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); >> + >> + if (ret< 0) { /* battery not present*/ >> + if (psp == POWER_SUPPLY_PROP_PRESENT) { >> + val->intval = 0; >> + return 0; > > Technically, you don't need the 'return 0' (and if we care about > symmetry: the TI version doesn't), since the caller knows that "not > present" will yield errors. I'm not sure which version makes more sense. > >> + } >> + return ret; >> + } >> + >> + if (psp == POWER_SUPPLY_PROP_PRESENT) >> + val->intval = 1; /* battery present */ >> + else /* POWER_SUPPLY_PROP_HEALTH */ >> /* SBS spec doesn't have a general health command. */ >> val->intval = POWER_SUPPLY_HEALTH_UNKNOWN; >> - } >> >> return 0; >> } >> @@ -626,6 +631,8 @@ static int sbs_get_property(struct power_supply *psy, >> else >> ret = sbs_get_battery_presence_and_health(client, psp, >> val); >> + >> + /* this can only be true if no gpio is used */ >> if (psp == POWER_SUPPLY_PROP_PRESENT) >> return 0; >> break; >> -- >> 2.20.1 >>
On Thu, Aug 22, 2019 at 12:43 AM Michael Nosthoff <committed@heine.so> wrote: > This done at the end of 'sbs_get_property' if the presence state changed and > no gpio is used. I suspect it triggers a readout of all the properties > and leads to this > endless loop? Ah, it's not all properties IIUC, but it does lead to power_supply_update_leds() -> power_supply_update_bat_leds() -> power_supply_get_property(... POWER_SUPPLY_PROP_STATUS ...). That would be enough to kick off the loop you're noticing. Brian
Hi, On Fri, Aug 16, 2019 at 09:58:42AM +0200, Michael Nosthoff wrote: > when the battery is set to sbs-mode and no gpio detection is enabled > "health" is always returning a value even when the battery is not present. > All other fields return "not present". > This leads to a scenario where the driver is constantly switching between > "present" and "not present" state. This generates a lot of constant > traffic on the i2c. > > This commit changes the response of "health" to an error when the battery > is not responding leading to a consistent "not present" state. > > Fixes: 76b16f4cdfb8 ("power: supply: sbs-battery: don't assume > MANUFACTURER_DATA formats") > > Signed-off-by: Michael Nosthoff <committed@heine.so> > Cc: Brian Norris <briannorris@chromium.org> > Cc: <stable@vger.kernel.org> > --- Thanks, queued. -- Sebastian > drivers/power/supply/sbs-battery.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c > index 2e86cc1e0e35..f8d74e9f7931 100644 > --- a/drivers/power/supply/sbs-battery.c > +++ b/drivers/power/supply/sbs-battery.c > @@ -314,17 +314,22 @@ static int sbs_get_battery_presence_and_health( > { > int ret; > > - if (psp == POWER_SUPPLY_PROP_PRESENT) { > - /* Dummy command; if it succeeds, battery is present. */ > - ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); > - if (ret < 0) > - val->intval = 0; /* battery disconnected */ > - else > - val->intval = 1; /* battery present */ > - } else { /* POWER_SUPPLY_PROP_HEALTH */ > + /* Dummy command; if it succeeds, battery is present. */ > + ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); > + > + if (ret < 0) { /* battery not present*/ > + if (psp == POWER_SUPPLY_PROP_PRESENT) { > + val->intval = 0; > + return 0; > + } > + return ret; > + } > + > + if (psp == POWER_SUPPLY_PROP_PRESENT) > + val->intval = 1; /* battery present */ > + else /* POWER_SUPPLY_PROP_HEALTH */ > /* SBS spec doesn't have a general health command. */ > val->intval = POWER_SUPPLY_HEALTH_UNKNOWN; > - } > > return 0; > } > @@ -626,6 +631,8 @@ static int sbs_get_property(struct power_supply *psy, > else > ret = sbs_get_battery_presence_and_health(client, psp, > val); > + > + /* this can only be true if no gpio is used */ > if (psp == POWER_SUPPLY_PROP_PRESENT) > return 0; > break; > -- > 2.20.1 >
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c index 2e86cc1e0e35..f8d74e9f7931 100644 --- a/drivers/power/supply/sbs-battery.c +++ b/drivers/power/supply/sbs-battery.c @@ -314,17 +314,22 @@ static int sbs_get_battery_presence_and_health( { int ret; - if (psp == POWER_SUPPLY_PROP_PRESENT) { - /* Dummy command; if it succeeds, battery is present. */ - ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); - if (ret < 0) - val->intval = 0; /* battery disconnected */ - else - val->intval = 1; /* battery present */ - } else { /* POWER_SUPPLY_PROP_HEALTH */ + /* Dummy command; if it succeeds, battery is present. */ + ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr); + + if (ret < 0) { /* battery not present*/ + if (psp == POWER_SUPPLY_PROP_PRESENT) { + val->intval = 0; + return 0; + } + return ret; + } + + if (psp == POWER_SUPPLY_PROP_PRESENT) + val->intval = 1; /* battery present */ + else /* POWER_SUPPLY_PROP_HEALTH */ /* SBS spec doesn't have a general health command. */ val->intval = POWER_SUPPLY_HEALTH_UNKNOWN; - } return 0; } @@ -626,6 +631,8 @@ static int sbs_get_property(struct power_supply *psy, else ret = sbs_get_battery_presence_and_health(client, psp, val); + + /* this can only be true if no gpio is used */ if (psp == POWER_SUPPLY_PROP_PRESENT) return 0; break;
when the battery is set to sbs-mode and no gpio detection is enabled "health" is always returning a value even when the battery is not present. All other fields return "not present". This leads to a scenario where the driver is constantly switching between "present" and "not present" state. This generates a lot of constant traffic on the i2c. This commit changes the response of "health" to an error when the battery is not responding leading to a consistent "not present" state. Fixes: 76b16f4cdfb8 ("power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats") Signed-off-by: Michael Nosthoff <committed@heine.so> Cc: Brian Norris <briannorris@chromium.org> Cc: <stable@vger.kernel.org> --- drivers/power/supply/sbs-battery.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)