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(-)
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;
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 */elseval->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 */-- 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 */elseval->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 */-- 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 Nosthoffcommitted@heine.so Cc: Brian Norrisbriannorris@chromium.org Cc:stable@vger.kernel.org
Reviewed-by: Brian Norrisbriannorris@chromium.org Tested-by: Brian Norrisbriannorris@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 */elseval->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 */-- 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 */elseval->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 */-- 2.20.1
linux-stable-mirror@lists.linaro.org
-
Brian Norris -
Michael Nosthoff -
Sebastian Reichel