This patchset improves support for SBS compliant batteries. Due to
the changes, the battery now exposes 32 power supply properties and
(un)plugging it generates a backtrace containing the following message
without the first patch in this series:
---------------------------
WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
add_uevent_var: too many keys
---------------------------
For references this is what an SBS battery status looks like after
the patch series has been applied:
cat /sys/class/power_supply/sbs-0-000b/uevent
POWER_SUPPLY_NAME=sbs-0-000b
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CYCLE_COUNT=12
POWER_SUPPLY_VOLTAGE_NOW=11441000
POWER_SUPPLY_CURRENT_NOW=-26000
POWER_SUPPLY_CURRENT_AVG=-24000
POWER_SUPPLY_CAPACITY=76
POWER_SUPPLY_CAPACITY_ERROR_MARGIN=1
POWER_SUPPLY_TEMP=198
POWER_SUPPLY_TIME_TO_EMPTY_AVG=438600
POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
POWER_SUPPLY_SERIAL_NUMBER=0000
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
POWER_SUPPLY_ENERGY_NOW=31090000
POWER_SUPPLY_ENERGY_FULL=42450000
POWER_SUPPLY_ENERGY_FULL_DESIGN=41040000
POWER_SUPPLY_CHARGE_NOW=2924000
POWER_SUPPLY_CHARGE_FULL=3898000
POWER_SUPPLY_CHARGE_FULL_DESIGN=3800000
POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=3000000
POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=12300000
POWER_SUPPLY_MANUFACTURE_YEAR=2017
POWER_SUPPLY_MANUFACTURE_MONTH=7
POWER_SUPPLY_MANUFACTURE_DAY=3
POWER_SUPPLY_MANUFACTURER=UR18650A
POWER_SUPPLY_MODEL_NAME=GEHC
-- Sebastian
Jean-Francois Dagenais (1):
power: supply: sbs-battery: add ability to disable charger broadcasts
Sebastian Reichel (18):
kobject: increase allowed number of uevent variables
power: supply: core: add capacity error margin property
power: supply: core: add manufacture date properties
power: supply: core: add POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED
power: supply: sbs-battery: Add TI BQ20Z65 support
power: supply: sbs-battery: add
POWER_SUPPLY_PROP_CAPACITY_ERROR_MARGIN support
power: supply: sbs-battery: simplify read_read_string_data
power: supply: sbs-battery: add PEC support
power: supply: sbs-battery: add POWER_SUPPLY_PROP_CURRENT_AVG support
power: supply: sbs-battery: Improve POWER_SUPPLY_PROP_TECHNOLOGY
support
power: supply: sbs-battery: add
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT/VOLTAGE_MAX support
power: supply: sbs-battery: add MANUFACTURE_DATE support
power: supply: sbs-battery: add
POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED support
power: supply: sbs-battery: fix idle battery status
power: supply: sbs-battery: switch from of_property_* to
device_property_*
power: supply: sbs-battery: switch to i2c's probe_new
power: supply: sbs-battery: constify power-supply property array
dt-bindings: power: sbs-battery: Convert to yaml
Documentation/ABI/testing/sysfs-class-power | 45 ++-
.../power/supply/sbs,sbs-battery.yaml | 83 +++++
.../bindings/power/supply/sbs_sbs-battery.txt | 27 --
drivers/power/supply/power_supply_sysfs.c | 5 +
drivers/power/supply/sbs-battery.c | 348 +++++++++++++-----
include/linux/kobject.h | 2 +-
include/linux/power_supply.h | 5 +
7 files changed, 404 insertions(+), 111 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/supply/sbs,sbs-battery.yaml
delete mode 100644 Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
--
2.26.2
Expose averaged current information, which is part of the SBS
standard and should be supported by all batteries.
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/sbs-battery.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index ab774d491269..611a11385496 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -25,7 +25,8 @@ enum {
REG_MANUFACTURER_DATA,
REG_TEMPERATURE,
REG_VOLTAGE,
- REG_CURRENT,
+ REG_CURRENT_NOW,
+ REG_CURRENT_AVG,
REG_MAX_ERR,
REG_CAPACITY,
REG_TIME_TO_EMPTY,
@@ -92,8 +93,10 @@ static const struct chip_data {
SBS_DATA(POWER_SUPPLY_PROP_TEMP, 0x08, 0, 65535),
[REG_VOLTAGE] =
SBS_DATA(POWER_SUPPLY_PROP_VOLTAGE_NOW, 0x09, 0, 20000),
- [REG_CURRENT] =
+ [REG_CURRENT_NOW] =
SBS_DATA(POWER_SUPPLY_PROP_CURRENT_NOW, 0x0A, -32768, 32767),
+ [REG_CURRENT_AVG] =
+ SBS_DATA(POWER_SUPPLY_PROP_CURRENT_AVG, 0x0B, -32768, 32767),
[REG_MAX_ERR] =
SBS_DATA(POWER_SUPPLY_PROP_CAPACITY_ERROR_MARGIN, 0x0c, 0, 100),
[REG_CAPACITY] =
@@ -142,6 +145,7 @@ static enum power_supply_property sbs_properties[] = {
POWER_SUPPLY_PROP_CYCLE_COUNT,
POWER_SUPPLY_PROP_VOLTAGE_NOW,
POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_CURRENT_AVG,
POWER_SUPPLY_PROP_CAPACITY,
POWER_SUPPLY_PROP_CAPACITY_ERROR_MARGIN,
POWER_SUPPLY_PROP_TEMP,
@@ -324,7 +328,7 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
{
int ret;
- ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
+ ret = sbs_read_word_data(client, sbs_data[REG_CURRENT_NOW].addr);
if (ret < 0)
return ret;
@@ -521,6 +525,7 @@ static void sbs_unit_adjustment(struct i2c_client *client,
case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
case POWER_SUPPLY_PROP_CURRENT_NOW:
+ case POWER_SUPPLY_PROP_CURRENT_AVG:
case POWER_SUPPLY_PROP_CHARGE_NOW:
case POWER_SUPPLY_PROP_CHARGE_FULL:
case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
@@ -699,6 +704,7 @@ static int sbs_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_CYCLE_COUNT:
case POWER_SUPPLY_PROP_VOLTAGE_NOW:
case POWER_SUPPLY_PROP_CURRENT_NOW:
+ case POWER_SUPPLY_PROP_CURRENT_AVG:
case POWER_SUPPLY_PROP_TEMP:
case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
--
2.26.2
Add support for reporting the MaxError register from
battery fuel gauges following the smart battery standard.
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/sbs-battery.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index a15783802ef8..4356fdf25d4a 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -26,6 +26,7 @@ enum {
REG_TEMPERATURE,
REG_VOLTAGE,
REG_CURRENT,
+ REG_MAX_ERR,
REG_CAPACITY,
REG_TIME_TO_EMPTY,
REG_TIME_TO_FULL,
@@ -85,6 +86,8 @@ static const struct chip_data {
SBS_DATA(POWER_SUPPLY_PROP_VOLTAGE_NOW, 0x09, 0, 20000),
[REG_CURRENT] =
SBS_DATA(POWER_SUPPLY_PROP_CURRENT_NOW, 0x0A, -32768, 32767),
+ [REG_MAX_ERR] =
+ SBS_DATA(POWER_SUPPLY_PROP_CAPACITY_ERROR_MARGIN, 0x0c, 0, 100),
[REG_CAPACITY] =
SBS_DATA(POWER_SUPPLY_PROP_CAPACITY, 0x0D, 0, 100),
[REG_REMAINING_CAPACITY] =
@@ -132,6 +135,7 @@ static enum power_supply_property sbs_properties[] = {
POWER_SUPPLY_PROP_VOLTAGE_NOW,
POWER_SUPPLY_PROP_CURRENT_NOW,
POWER_SUPPLY_PROP_CAPACITY,
+ POWER_SUPPLY_PROP_CAPACITY_ERROR_MARGIN,
POWER_SUPPLY_PROP_TEMP,
POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG,
POWER_SUPPLY_PROP_TIME_TO_FULL_AVG,
@@ -676,6 +680,7 @@ static int sbs_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
case POWER_SUPPLY_PROP_CAPACITY:
+ case POWER_SUPPLY_PROP_CAPACITY_ERROR_MARGIN:
ret = sbs_get_property_index(client, psp);
if (ret < 0)
break;
--
2.26.2
SBS battery driver exposes 32 power supply properties now,
which will result in uevent failure on (un)plugging the
battery. Other drivers (e.g. bq27xxx) are also coming close
to this limit, so increase it.
Signed-off-by: Sebastian Reichel <[email protected]>
---
include/linux/kobject.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index e2ca0a292e21..75e822569e39 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -29,7 +29,7 @@
#include <linux/uidgid.h>
#define UEVENT_HELPER_PATH_LEN 256
-#define UEVENT_NUM_ENVP 32 /* number of env pointers */
+#define UEVENT_NUM_ENVP 64 /* number of env pointers */
#define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
#ifdef CONFIG_UEVENT_HELPER
--
2.26.2
Expose maximum charge current/voltage information requested
by the battery.
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/sbs-battery.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index b697a42d9ccf..e6f61baa9bed 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -46,6 +46,8 @@ enum {
REG_CHEMISTRY,
REG_MANUFACTURER,
REG_MODEL_NAME,
+ REG_CHARGE_CURRENT,
+ REG_CHARGE_VOLTAGE,
};
#define REG_ADDR_SPEC_INFO 0x1A
@@ -114,6 +116,10 @@ static const struct chip_data {
SBS_DATA(POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG, 0x12, 0, 65535),
[REG_TIME_TO_FULL] =
SBS_DATA(POWER_SUPPLY_PROP_TIME_TO_FULL_AVG, 0x13, 0, 65535),
+ [REG_CHARGE_CURRENT] =
+ SBS_DATA(POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX, 0x14, 0, 65535),
+ [REG_CHARGE_VOLTAGE] =
+ SBS_DATA(POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX, 0x15, 0, 65535),
[REG_STATUS] =
SBS_DATA(POWER_SUPPLY_PROP_STATUS, 0x16, 0, 65535),
[REG_CAPACITY_LEVEL] =
@@ -163,6 +169,8 @@ static enum power_supply_property sbs_properties[] = {
POWER_SUPPLY_PROP_CHARGE_NOW,
POWER_SUPPLY_PROP_CHARGE_FULL,
POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
/* Properties of type `const char *' */
POWER_SUPPLY_PROP_MANUFACTURER,
POWER_SUPPLY_PROP_MODEL_NAME
@@ -531,6 +539,8 @@ static void sbs_unit_adjustment(struct i2c_client *client,
case POWER_SUPPLY_PROP_CURRENT_NOW:
case POWER_SUPPLY_PROP_CURRENT_AVG:
case POWER_SUPPLY_PROP_CHARGE_NOW:
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
case POWER_SUPPLY_PROP_CHARGE_FULL:
case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
val->intval *= BASE_UNIT_CONVERSION;
@@ -749,6 +759,8 @@ static int sbs_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
case POWER_SUPPLY_PROP_CAPACITY:
case POWER_SUPPLY_PROP_CAPACITY_ERROR_MARGIN:
ret = sbs_get_property_index(client, psp);
--
2.26.2
Add support for BQ20Z65 manufacturer data to the sbs-battery
driver. Implementation has been verified using the public TRM
available from [0] and tested using a GE Flex 3S2P battery.
[0] http://www.ti.com/lit/pdf/sluu386
Signed-off-by: Sebastian Reichel <[email protected]>
---
.../bindings/power/supply/sbs_sbs-battery.txt | 1 +
drivers/power/supply/sbs-battery.c | 15 ++++++++++-----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt b/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
index 4e78e51018eb..fa5a8b516dbf 100644
--- a/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
@@ -6,6 +6,7 @@ Required properties :
part number compatible string might be used in order to take care of
vendor specific registers.
Known <vendor>,<part-number>:
+ ti,bq20z65
ti,bq20z75
Optional properties :
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 6acd242eed48..a15783802ef8 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -149,8 +149,8 @@ static enum power_supply_property sbs_properties[] = {
POWER_SUPPLY_PROP_MODEL_NAME
};
-/* Supports special manufacturer commands from TI BQ20Z75 IC. */
-#define SBS_FLAGS_TI_BQ20Z75 BIT(0)
+/* Supports special manufacturer commands from TI BQ20Z65 and BQ20Z75 IC. */
+#define SBS_FLAGS_TI_BQ20ZX5 BIT(0)
struct sbs_info {
struct i2c_client *client;
@@ -626,7 +626,7 @@ static int sbs_get_property(struct power_supply *psy,
switch (psp) {
case POWER_SUPPLY_PROP_PRESENT:
case POWER_SUPPLY_PROP_HEALTH:
- if (chip->flags & SBS_FLAGS_TI_BQ20Z75)
+ if (chip->flags & SBS_FLAGS_TI_BQ20ZX5)
ret = sbs_get_ti_battery_presence_and_health(client,
psp, val);
else
@@ -950,7 +950,7 @@ static int sbs_suspend(struct device *dev)
if (chip->poll_time > 0)
cancel_delayed_work_sync(&chip->work);
- if (chip->flags & SBS_FLAGS_TI_BQ20Z75) {
+ if (chip->flags & SBS_FLAGS_TI_BQ20ZX5) {
/* Write to manufacturer access with sleep command. */
ret = sbs_write_word_data(client,
sbs_data[REG_MANUFACTURER_DATA].addr,
@@ -970,6 +970,7 @@ static SIMPLE_DEV_PM_OPS(sbs_pm_ops, sbs_suspend, NULL);
#endif
static const struct i2c_device_id sbs_id[] = {
+ { "bq20z65", 0 },
{ "bq20z75", 0 },
{ "sbs-battery", 1 },
{}
@@ -978,9 +979,13 @@ MODULE_DEVICE_TABLE(i2c, sbs_id);
static const struct of_device_id sbs_dt_ids[] = {
{ .compatible = "sbs,sbs-battery" },
+ {
+ .compatible = "ti,bq20z65",
+ .data = (void *)SBS_FLAGS_TI_BQ20ZX5,
+ },
{
.compatible = "ti,bq20z75",
- .data = (void *)SBS_FLAGS_TI_BQ20Z75,
+ .data = (void *)SBS_FLAGS_TI_BQ20ZX5,
},
{ }
};
--
2.26.2
Some battery fuel gauges know when the battery needs to
be recalibrated before providing usable values. This
should be reported via the health property.
Signed-off-by: Sebastian Reichel <[email protected]>
---
Documentation/ABI/testing/sysfs-class-power | 2 +-
drivers/power/supply/power_supply_sysfs.c | 1 +
include/linux/power_supply.h | 1 +
3 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index e6d7348766b2..216d61a22f1e 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -205,7 +205,7 @@ Description:
Valid values: "Unknown", "Good", "Overheat", "Dead",
"Over voltage", "Unspecified failure", "Cold",
"Watchdog timer expire", "Safety timer expire",
- "Over current"
+ "Over current", "Calibration required"
What: /sys/class/power_supply/<supply_name>/precharge_current
Date: June 2017
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 78d5382e69f1..bc79560229b5 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -100,6 +100,7 @@ static const char * const POWER_SUPPLY_HEALTH_TEXT[] = {
[POWER_SUPPLY_HEALTH_WATCHDOG_TIMER_EXPIRE] = "Watchdog timer expire",
[POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE] = "Safety timer expire",
[POWER_SUPPLY_HEALTH_OVERCURRENT] = "Over current",
+ [POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED] = "Calibration required",
};
static const char * const POWER_SUPPLY_TECHNOLOGY_TEXT[] = {
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 63ffe2a0a87b..ac1345a48ad0 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -61,6 +61,7 @@ enum {
POWER_SUPPLY_HEALTH_WATCHDOG_TIMER_EXPIRE,
POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE,
POWER_SUPPLY_HEALTH_OVERCURRENT,
+ POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED,
};
enum {
--
2.26.2
The SBS battery implements SMBus block reads. Currently the
driver "emulates" this by doing an I2C byte read for the
length followed by an I2C block read. The I2C subsystem
actually provides a proper API for doing SMBus block reads,
which can and should be used instead. The current implementation
does not properly handle packet error checking (PEC).
This change requires, that I2C bus drivers support I2C_M_RECV_LEN
or directly provide the SMBus API to access device manufacturer
and model name.
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/power/supply/sbs-battery.c | 65 ++++++------------------------
1 file changed, 12 insertions(+), 53 deletions(-)
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 4356fdf25d4a..a9a1d28dabbe 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -202,66 +202,32 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address,
char *values)
{
struct sbs_info *chip = i2c_get_clientdata(client);
- s32 ret = 0, block_length = 0;
- int retries_length, retries_block;
- u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
-
- retries_length = chip->i2c_retry_count;
- retries_block = chip->i2c_retry_count;
+ int retries = chip->i2c_retry_count;
+ s32 ret = 0;
- /* Adapter needs to support these two functions */
if (!i2c_check_functionality(client->adapter,
- I2C_FUNC_SMBUS_BYTE_DATA |
- I2C_FUNC_SMBUS_I2C_BLOCK)){
+ I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
return -ENODEV;
}
- /* Get the length of block data */
- while (retries_length > 0) {
- ret = i2c_smbus_read_byte_data(client, address);
- if (ret >= 0)
- break;
- retries_length--;
- }
-
- if (ret < 0) {
- dev_dbg(&client->dev,
- "%s: i2c read at address 0x%x failed\n",
- __func__, address);
- return ret;
- }
-
- /* block_length does not include NULL terminator */
- block_length = ret;
- if (block_length > I2C_SMBUS_BLOCK_MAX) {
- dev_err(&client->dev,
- "%s: Returned block_length is longer than 0x%x\n",
- __func__, I2C_SMBUS_BLOCK_MAX);
- return -EINVAL;
- }
-
/* Get the block data */
- while (retries_block > 0) {
- ret = i2c_smbus_read_i2c_block_data(
- client, address,
- block_length + 1, block_buffer);
+ while (retries > 0) {
+ ret = i2c_smbus_read_block_data(client, address, values);
if (ret >= 0)
break;
- retries_block--;
+ retries--;
}
if (ret < 0) {
- dev_dbg(&client->dev,
- "%s: i2c read at address 0x%x failed\n",
- __func__, address);
+ dev_dbg(&client->dev, "%s: failed to read block 0x%x: %d\n",
+ __func__, address, ret);
return ret;
}
- /* block_buffer[0] == block_length */
- memcpy(values, block_buffer + 1, block_length);
- values[block_length] = '\0';
+ /* add string termination */
+ values[ret] = '\0';
- return ret;
+ return 0;
}
static int sbs_write_word_data(struct i2c_client *client, u8 address,
@@ -465,14 +431,7 @@ static int sbs_get_battery_property(struct i2c_client *client,
static int sbs_get_battery_string_property(struct i2c_client *client,
int reg_offset, enum power_supply_property psp, char *val)
{
- s32 ret;
-
- ret = sbs_read_string_data(client, sbs_data[reg_offset].addr, val);
-
- if (ret < 0)
- return ret;
-
- return 0;
+ return sbs_read_string_data(client, sbs_data[reg_offset].addr, val);
}
static void sbs_unit_adjustment(struct i2c_client *client,
--
2.26.2
On Wed, May 13, 2020 at 08:55:57PM +0200, Sebastian Reichel wrote:
> SBS battery driver exposes 32 power supply properties now,
> which will result in uevent failure on (un)plugging the
> battery. Other drivers (e.g. bq27xxx) are also coming close
> to this limit, so increase it.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> include/linux/kobject.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index e2ca0a292e21..75e822569e39 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -29,7 +29,7 @@
> #include <linux/uidgid.h>
>
> #define UEVENT_HELPER_PATH_LEN 256
> -#define UEVENT_NUM_ENVP 32 /* number of env pointers */
> +#define UEVENT_NUM_ENVP 64 /* number of env pointers */
> #define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
>
> #ifdef CONFIG_UEVENT_HELPER
Acked-by: Greg Kroah-Hartman <[email protected]>
On 2020/05/13, Sebastian Reichel wrote:
> SBS battery driver exposes 32 power supply properties now,
> which will result in uevent failure on (un)plugging the
> battery. Other drivers (e.g. bq27xxx) are also coming close
> to this limit, so increase it.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> include/linux/kobject.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index e2ca0a292e21..75e822569e39 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -29,7 +29,7 @@
> #include <linux/uidgid.h>
>
> #define UEVENT_HELPER_PATH_LEN 256
> -#define UEVENT_NUM_ENVP 32 /* number of env pointers */
> +#define UEVENT_NUM_ENVP 64 /* number of env pointers */
To be on the safe side I've checked systemd/udev. It's using ordered hashmap,
so it's perfectly capable of handling the extra entries.
Reviewed-by: Emil Velikov <[email protected]>
-Emil
On Wed, 13 May 2020 20:56:01 +0200, Sebastian Reichel wrote:
> Add support for BQ20Z65 manufacturer data to the sbs-battery
> driver. Implementation has been verified using the public TRM
> available from [0] and tested using a GE Flex 3S2P battery.
>
> [0] http://www.ti.com/lit/pdf/sluu386
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> .../bindings/power/supply/sbs_sbs-battery.txt | 1 +
> drivers/power/supply/sbs-battery.c | 15 ++++++++++-----
> 2 files changed, 11 insertions(+), 5 deletions(-)
>
Acked-by: Rob Herring <[email protected]>
Hi,
I queued this series to power-supply's for-next branch.
-- Sebastian
On Wed, May 13, 2020 at 08:55:56PM +0200, Sebastian Reichel wrote:
> This patchset improves support for SBS compliant batteries. Due to
> the changes, the battery now exposes 32 power supply properties and
> (un)plugging it generates a backtrace containing the following message
> without the first patch in this series:
>
> ---------------------------
> WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
> add_uevent_var: too many keys
> ---------------------------
>
> For references this is what an SBS battery status looks like after
> the patch series has been applied:
>
> cat /sys/class/power_supply/sbs-0-000b/uevent
> POWER_SUPPLY_NAME=sbs-0-000b
> POWER_SUPPLY_TYPE=Battery
> POWER_SUPPLY_STATUS=Discharging
> POWER_SUPPLY_CAPACITY_LEVEL=Normal
> POWER_SUPPLY_HEALTH=Good
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CYCLE_COUNT=12
> POWER_SUPPLY_VOLTAGE_NOW=11441000
> POWER_SUPPLY_CURRENT_NOW=-26000
> POWER_SUPPLY_CURRENT_AVG=-24000
> POWER_SUPPLY_CAPACITY=76
> POWER_SUPPLY_CAPACITY_ERROR_MARGIN=1
> POWER_SUPPLY_TEMP=198
> POWER_SUPPLY_TIME_TO_EMPTY_AVG=438600
> POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
> POWER_SUPPLY_SERIAL_NUMBER=0000
> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
> POWER_SUPPLY_ENERGY_NOW=31090000
> POWER_SUPPLY_ENERGY_FULL=42450000
> POWER_SUPPLY_ENERGY_FULL_DESIGN=41040000
> POWER_SUPPLY_CHARGE_NOW=2924000
> POWER_SUPPLY_CHARGE_FULL=3898000
> POWER_SUPPLY_CHARGE_FULL_DESIGN=3800000
> POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=3000000
> POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=12300000
> POWER_SUPPLY_MANUFACTURE_YEAR=2017
> POWER_SUPPLY_MANUFACTURE_MONTH=7
> POWER_SUPPLY_MANUFACTURE_DAY=3
> POWER_SUPPLY_MANUFACTURER=UR18650A
> POWER_SUPPLY_MODEL_NAME=GEHC
>
> -- Sebastian
>
> Jean-Francois Dagenais (1):
> power: supply: sbs-battery: add ability to disable charger broadcasts
>
> Sebastian Reichel (18):
> kobject: increase allowed number of uevent variables
> power: supply: core: add capacity error margin property
> power: supply: core: add manufacture date properties
> power: supply: core: add POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED
> power: supply: sbs-battery: Add TI BQ20Z65 support
> power: supply: sbs-battery: add
> POWER_SUPPLY_PROP_CAPACITY_ERROR_MARGIN support
> power: supply: sbs-battery: simplify read_read_string_data
> power: supply: sbs-battery: add PEC support
> power: supply: sbs-battery: add POWER_SUPPLY_PROP_CURRENT_AVG support
> power: supply: sbs-battery: Improve POWER_SUPPLY_PROP_TECHNOLOGY
> support
> power: supply: sbs-battery: add
> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT/VOLTAGE_MAX support
> power: supply: sbs-battery: add MANUFACTURE_DATE support
> power: supply: sbs-battery: add
> POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED support
> power: supply: sbs-battery: fix idle battery status
> power: supply: sbs-battery: switch from of_property_* to
> device_property_*
> power: supply: sbs-battery: switch to i2c's probe_new
> power: supply: sbs-battery: constify power-supply property array
> dt-bindings: power: sbs-battery: Convert to yaml
>
> Documentation/ABI/testing/sysfs-class-power | 45 ++-
> .../power/supply/sbs,sbs-battery.yaml | 83 +++++
> .../bindings/power/supply/sbs_sbs-battery.txt | 27 --
> drivers/power/supply/power_supply_sysfs.c | 5 +
> drivers/power/supply/sbs-battery.c | 348 +++++++++++++-----
> include/linux/kobject.h | 2 +-
> include/linux/power_supply.h | 5 +
> 7 files changed, 404 insertions(+), 111 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/power/supply/sbs,sbs-battery.yaml
> delete mode 100644 Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
>
> --
> 2.26.2
>
Hi!
> This patchset improves support for SBS compliant batteries. Due to
> the changes, the battery now exposes 32 power supply properties and
> (un)plugging it generates a backtrace containing the following message
> without the first patch in this series:
>
> ---------------------------
> WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
> add_uevent_var: too many keys
> ---------------------------
>
> For references this is what an SBS battery status looks like after
> the patch series has been applied:
>
> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
Is that correct, BTW? sounds like these should not be equal...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Sebastian,
On 13.05.2020 20:55, Sebastian Reichel wrote:
> This patchset improves support for SBS compliant batteries. Due to
> the changes, the battery now exposes 32 power supply properties and
> (un)plugging it generates a backtrace containing the following message
> without the first patch in this series:
>
> ---------------------------
> WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
> add_uevent_var: too many keys
> ---------------------------
>
> For references this is what an SBS battery status looks like after
> the patch series has been applied:
>
> cat /sys/class/power_supply/sbs-0-000b/uevent
> POWER_SUPPLY_NAME=sbs-0-000b
> POWER_SUPPLY_TYPE=Battery
> POWER_SUPPLY_STATUS=Discharging
> POWER_SUPPLY_CAPACITY_LEVEL=Normal
> POWER_SUPPLY_HEALTH=Good
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CYCLE_COUNT=12
> POWER_SUPPLY_VOLTAGE_NOW=11441000
> POWER_SUPPLY_CURRENT_NOW=-26000
> POWER_SUPPLY_CURRENT_AVG=-24000
> POWER_SUPPLY_CAPACITY=76
> POWER_SUPPLY_CAPACITY_ERROR_MARGIN=1
> POWER_SUPPLY_TEMP=198
> POWER_SUPPLY_TIME_TO_EMPTY_AVG=438600
> POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
> POWER_SUPPLY_SERIAL_NUMBER=0000
> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
> POWER_SUPPLY_ENERGY_NOW=31090000
> POWER_SUPPLY_ENERGY_FULL=42450000
> POWER_SUPPLY_ENERGY_FULL_DESIGN=41040000
> POWER_SUPPLY_CHARGE_NOW=2924000
> POWER_SUPPLY_CHARGE_FULL=3898000
> POWER_SUPPLY_CHARGE_FULL_DESIGN=3800000
> POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=3000000
> POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=12300000
> POWER_SUPPLY_MANUFACTURE_YEAR=2017
> POWER_SUPPLY_MANUFACTURE_MONTH=7
> POWER_SUPPLY_MANUFACTURE_DAY=3
> POWER_SUPPLY_MANUFACTURER=UR18650A
> POWER_SUPPLY_MODEL_NAME=GEHC
This patch landed in linux-next dated 20200529. Sadly it causes a
regression on Samsung Exynos-based Chromebooks (Exynos5250 Snow,
Exynos5420 Peach-Pi and Exynos5800 Peach-Pit). System boots to
userspace, but then, when udev populates /dev, booting hangs:
[ 4.435167] VFS: Mounted root (ext4 filesystem) readonly on device
179:51.
[ 4.457477] devtmpfs: mounted
[ 4.460235] Freeing unused kernel memory: 1024K
[ 4.464022] Run /sbin/init as init process
INIT: version 2.88 booting
[info] Using makefile-style concurrent boot in runlevel S.
[ 5.102096] random: crng init done
[....] Starting the hotplug events dispatcher: systemd-udevdstarting
version 236
[ ok .
[....] Synthesizing the initial hotplug events...[ ok done.
[....] Waiting for /dev to be fully populated...[ 34.409914]
TPS65090_RAILSDCDC1: disabling
[ 34.412977] TPS65090_RAILSDCDC2: disabling
[ 34.417021] TPS65090_RAILSDCDC3: disabling
[ 34.423848] TPS65090_RAILSLDO1: disabling
[ 34.429068] TPS65090_RAILSLDO2: disabling
Bisect between v5.7-rc1 and next-20200529 pointed me to the first bad
commit: [c4b12a2f3f3de670f6be5e96092a2cab0b877f1a] power: supply:
sbs-battery: simplify read_read_string_data. However reverting it in
linux-next doesn't fix the issue, so the next commits are also relevant
to this issue.
Let me know how can I help debugging it.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi Marek,
On Mon, Jun 01, 2020 at 12:40:27PM +0200, Marek Szyprowski wrote:
> On 13.05.2020 20:55, Sebastian Reichel wrote:
> > This patchset improves support for SBS compliant batteries. Due to
> > the changes, the battery now exposes 32 power supply properties and
> > (un)plugging it generates a backtrace containing the following message
> > without the first patch in this series:
> >
> > ---------------------------
> > WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
> > add_uevent_var: too many keys
> > ---------------------------
> >
> > For references this is what an SBS battery status looks like after
> > the patch series has been applied:
> >
> > cat /sys/class/power_supply/sbs-0-000b/uevent
> > POWER_SUPPLY_NAME=sbs-0-000b
> > POWER_SUPPLY_TYPE=Battery
> > POWER_SUPPLY_STATUS=Discharging
> > POWER_SUPPLY_CAPACITY_LEVEL=Normal
> > POWER_SUPPLY_HEALTH=Good
> > POWER_SUPPLY_PRESENT=1
> > POWER_SUPPLY_TECHNOLOGY=Li-ion
> > POWER_SUPPLY_CYCLE_COUNT=12
> > POWER_SUPPLY_VOLTAGE_NOW=11441000
> > POWER_SUPPLY_CURRENT_NOW=-26000
> > POWER_SUPPLY_CURRENT_AVG=-24000
> > POWER_SUPPLY_CAPACITY=76
> > POWER_SUPPLY_CAPACITY_ERROR_MARGIN=1
> > POWER_SUPPLY_TEMP=198
> > POWER_SUPPLY_TIME_TO_EMPTY_AVG=438600
> > POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
> > POWER_SUPPLY_SERIAL_NUMBER=0000
> > POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
> > POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
> > POWER_SUPPLY_ENERGY_NOW=31090000
> > POWER_SUPPLY_ENERGY_FULL=42450000
> > POWER_SUPPLY_ENERGY_FULL_DESIGN=41040000
> > POWER_SUPPLY_CHARGE_NOW=2924000
> > POWER_SUPPLY_CHARGE_FULL=3898000
> > POWER_SUPPLY_CHARGE_FULL_DESIGN=3800000
> > POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=3000000
> > POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=12300000
> > POWER_SUPPLY_MANUFACTURE_YEAR=2017
> > POWER_SUPPLY_MANUFACTURE_MONTH=7
> > POWER_SUPPLY_MANUFACTURE_DAY=3
> > POWER_SUPPLY_MANUFACTURER=UR18650A
> > POWER_SUPPLY_MODEL_NAME=GEHC
>
> This patch landed in linux-next dated 20200529. Sadly it causes a
> regression on Samsung Exynos-based Chromebooks (Exynos5250 Snow,
> Exynos5420 Peach-Pi and Exynos5800 Peach-Pit). System boots to
> userspace, but then, when udev populates /dev, booting hangs:
>
> [??? 4.435167] VFS: Mounted root (ext4 filesystem) readonly on device
> 179:51.
> [??? 4.457477] devtmpfs: mounted
> [??? 4.460235] Freeing unused kernel memory: 1024K
> [??? 4.464022] Run /sbin/init as init process
> INIT: version 2.88 booting
> [info] Using makefile-style concurrent boot in runlevel S.
> [??? 5.102096] random: crng init done
> [....] Starting the hotplug events dispatcher: systemd-udevdstarting
> version 236
> [ ok .
> [....] Synthesizing the initial hotplug events...[ ok done.
> [....] Waiting for /dev to be fully populated...[?? 34.409914]
> TPS65090_RAILSDCDC1: disabling
> [?? 34.412977] TPS65090_RAILSDCDC2: disabling
> [?? 34.417021] TPS65090_RAILSDCDC3: disabling
> [?? 34.423848] TPS65090_RAILSLDO1: disabling
> [?? 34.429068] TPS65090_RAILSLDO2: disabling
:(
log does not look useful either.
> Bisect between v5.7-rc1 and next-20200529 pointed me to the first bad
> commit: [c4b12a2f3f3de670f6be5e96092a2cab0b877f1a] power: supply:
> sbs-battery: simplify read_read_string_data.
ok. I tested this on an to-be-upstreamed i.MX6 based system
and arch/arm/boot/dts/imx53-ppd.dts. I think the difference
is, that i2c-exynos5 does not expose I2C_FUNC_SMBUS_READ_BLOCK_DATA.
I hoped all systems using SBS battery support this, but now
I see I2C_FUNC_SMBUS_EMUL only supports writing block data.
Looks like I need to add another patch implementing that
using the old code with added PEC support.
In any case that should only return -ENODEV for the property
(and uevent), but not break boot. So something fishy is going
on.
> However reverting it in linux-next doesn't fix the issue, so the
> next commits are also relevant to this issue.
The next patch, which adds PEC support depends on the simplification
of sbs_read_string_data. The old, open coded variant will result in
PEC failure for string properties (which should not stop boot either
of course). Can you try reverting both?
If that helps I will revert those two instead of dropping the whole
series for this merge window.
> Let me know how can I help debugging it.
I suspect, that this is userspace endlessly retrying reading the
battery uevent when an error is returned. Could you check this?
Should be easy to see by adding some printfs.
That would mean a faulty battery could stall complete boot without
a useful error message, which is bad and needs to be fixed.
Sorry for the inconvience and thanks for your report,
-- Sebastian
Hi,
On Fri, May 29, 2020 at 06:27:04PM +0200, Pavel Machek wrote:
> > This patchset improves support for SBS compliant batteries. Due to
> > the changes, the battery now exposes 32 power supply properties and
> > (un)plugging it generates a backtrace containing the following message
> > without the first patch in this series:
> >
> > ---------------------------
> > WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
> > add_uevent_var: too many keys
> > ---------------------------
> >
> > For references this is what an SBS battery status looks like after
> > the patch series has been applied:
> >
> > POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
> > POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
>
> Is that correct, BTW? sounds like these should not be equal...
(Some) GE batteries have weird values stored in the SBS chip.
For example manufacturer and model name are swapped:
POWER_SUPPLY_MANUFACTURER=UR18650A
POWER_SUPPLY_MODEL_NAME=GEHC
I carefully checked manufacturer/model name when writing these
patches some time ago and came to the conclusion that the batteries
do report it the wrong way around.
I will have a look for the design voltages (which are not modified
by this patchset), but I expect this to be another GE specific thing.
-- Sebastian
Hi Sebastian,
On 01.06.2020 19:05, Sebastian Reichel wrote:
> On Mon, Jun 01, 2020 at 12:40:27PM +0200, Marek Szyprowski wrote:
>> On 13.05.2020 20:55, Sebastian Reichel wrote:
>>> This patchset improves support for SBS compliant batteries. Due to
>>> the changes, the battery now exposes 32 power supply properties and
>>> (un)plugging it generates a backtrace containing the following message
>>> without the first patch in this series:
>>>
>>> ---------------------------
>>> WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
>>> add_uevent_var: too many keys
>>> ---------------------------
>>>
>>> For references this is what an SBS battery status looks like after
>>> the patch series has been applied:
>>>
>>> cat /sys/class/power_supply/sbs-0-000b/uevent
>>> POWER_SUPPLY_NAME=sbs-0-000b
>>> POWER_SUPPLY_TYPE=Battery
>>> POWER_SUPPLY_STATUS=Discharging
>>> POWER_SUPPLY_CAPACITY_LEVEL=Normal
>>> POWER_SUPPLY_HEALTH=Good
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>>> POWER_SUPPLY_CYCLE_COUNT=12
>>> POWER_SUPPLY_VOLTAGE_NOW=11441000
>>> POWER_SUPPLY_CURRENT_NOW=-26000
>>> POWER_SUPPLY_CURRENT_AVG=-24000
>>> POWER_SUPPLY_CAPACITY=76
>>> POWER_SUPPLY_CAPACITY_ERROR_MARGIN=1
>>> POWER_SUPPLY_TEMP=198
>>> POWER_SUPPLY_TIME_TO_EMPTY_AVG=438600
>>> POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
>>> POWER_SUPPLY_SERIAL_NUMBER=0000
>>> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
>>> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
>>> POWER_SUPPLY_ENERGY_NOW=31090000
>>> POWER_SUPPLY_ENERGY_FULL=42450000
>>> POWER_SUPPLY_ENERGY_FULL_DESIGN=41040000
>>> POWER_SUPPLY_CHARGE_NOW=2924000
>>> POWER_SUPPLY_CHARGE_FULL=3898000
>>> POWER_SUPPLY_CHARGE_FULL_DESIGN=3800000
>>> POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=3000000
>>> POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=12300000
>>> POWER_SUPPLY_MANUFACTURE_YEAR=2017
>>> POWER_SUPPLY_MANUFACTURE_MONTH=7
>>> POWER_SUPPLY_MANUFACTURE_DAY=3
>>> POWER_SUPPLY_MANUFACTURER=UR18650A
>>> POWER_SUPPLY_MODEL_NAME=GEHC
>> This patch landed in linux-next dated 20200529. Sadly it causes a
>> regression on Samsung Exynos-based Chromebooks (Exynos5250 Snow,
>> Exynos5420 Peach-Pi and Exynos5800 Peach-Pit). System boots to
>> userspace, but then, when udev populates /dev, booting hangs:
>>
>> [ 4.435167] VFS: Mounted root (ext4 filesystem) readonly on device
>> 179:51.
>> [ 4.457477] devtmpfs: mounted
>> [ 4.460235] Freeing unused kernel memory: 1024K
>> [ 4.464022] Run /sbin/init as init process
>> INIT: version 2.88 booting
>> [info] Using makefile-style concurrent boot in runlevel S.
>> [ 5.102096] random: crng init done
>> [....] Starting the hotplug events dispatcher: systemd-udevdstarting
>> version 236
>> [ ok .
>> [....] Synthesizing the initial hotplug events...[ ok done.
>> [....] Waiting for /dev to be fully populated...[ 34.409914]
>> TPS65090_RAILSDCDC1: disabling
>> [ 34.412977] TPS65090_RAILSDCDC2: disabling
>> [ 34.417021] TPS65090_RAILSDCDC3: disabling
>> [ 34.423848] TPS65090_RAILSLDO1: disabling
>> [ 34.429068] TPS65090_RAILSLDO2: disabling
> :(
>
> log does not look useful either.
>
>> Bisect between v5.7-rc1 and next-20200529 pointed me to the first bad
>> commit: [c4b12a2f3f3de670f6be5e96092a2cab0b877f1a] power: supply:
>> sbs-battery: simplify read_read_string_data.
> ok. I tested this on an to-be-upstreamed i.MX6 based system
> and arch/arm/boot/dts/imx53-ppd.dts. I think the difference
> is, that i2c-exynos5 does not expose I2C_FUNC_SMBUS_READ_BLOCK_DATA.
> I hoped all systems using SBS battery support this, but now
> I see I2C_FUNC_SMBUS_EMUL only supports writing block data.
> Looks like I need to add another patch implementing that
> using the old code with added PEC support.
>
> In any case that should only return -ENODEV for the property
> (and uevent), but not break boot. So something fishy is going
> on.
>
>> However reverting it in linux-next doesn't fix the issue, so the
>> next commits are also relevant to this issue.
> The next patch, which adds PEC support depends on the simplification
> of sbs_read_string_data. The old, open coded variant will result in
> PEC failure for string properties (which should not stop boot either
> of course). Can you try reverting both?
Indeed, reverting both (and fixing the conflict) restores proper boot.
> If that helps I will revert those two instead of dropping the whole
> series for this merge window.
>
>> Let me know how can I help debugging it.
> I suspect, that this is userspace endlessly retrying reading the
> battery uevent when an error is returned. Could you check this?
> Should be easy to see by adding some printfs.
I've added some debug messages in sbs_get_property() and it read the
same properties many times. However I've noticed that if I wait long
enough booting finally continues.
> That would mean a faulty battery could stall complete boot without
> a useful error message, which is bad and needs to be fixed.
>
> Sorry for the inconvience and thanks for your report,
No problem, finding regressions is one of the linux-next goal.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi,
On Tue, Jun 02, 2020 at 09:17:09AM +0200, Marek Szyprowski wrote:
> Hi Sebastian,
>
> On 01.06.2020 19:05, Sebastian Reichel wrote:
> > On Mon, Jun 01, 2020 at 12:40:27PM +0200, Marek Szyprowski wrote:
> >> On 13.05.2020 20:55, Sebastian Reichel wrote:
> >>> This patchset improves support for SBS compliant batteries. Due to
> >>> the changes, the battery now exposes 32 power supply properties and
> >>> (un)plugging it generates a backtrace containing the following message
> >>> without the first patch in this series:
> >>>
> >>> ---------------------------
> >>> WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
> >>> add_uevent_var: too many keys
> >>> ---------------------------
> >>>
> >>> For references this is what an SBS battery status looks like after
> >>> the patch series has been applied:
> >>>
> >>> cat /sys/class/power_supply/sbs-0-000b/uevent
> >>> POWER_SUPPLY_NAME=sbs-0-000b
> >>> POWER_SUPPLY_TYPE=Battery
> >>> POWER_SUPPLY_STATUS=Discharging
> >>> POWER_SUPPLY_CAPACITY_LEVEL=Normal
> >>> POWER_SUPPLY_HEALTH=Good
> >>> POWER_SUPPLY_PRESENT=1
> >>> POWER_SUPPLY_TECHNOLOGY=Li-ion
> >>> POWER_SUPPLY_CYCLE_COUNT=12
> >>> POWER_SUPPLY_VOLTAGE_NOW=11441000
> >>> POWER_SUPPLY_CURRENT_NOW=-26000
> >>> POWER_SUPPLY_CURRENT_AVG=-24000
> >>> POWER_SUPPLY_CAPACITY=76
> >>> POWER_SUPPLY_CAPACITY_ERROR_MARGIN=1
> >>> POWER_SUPPLY_TEMP=198
> >>> POWER_SUPPLY_TIME_TO_EMPTY_AVG=438600
> >>> POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
> >>> POWER_SUPPLY_SERIAL_NUMBER=0000
> >>> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
> >>> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
> >>> POWER_SUPPLY_ENERGY_NOW=31090000
> >>> POWER_SUPPLY_ENERGY_FULL=42450000
> >>> POWER_SUPPLY_ENERGY_FULL_DESIGN=41040000
> >>> POWER_SUPPLY_CHARGE_NOW=2924000
> >>> POWER_SUPPLY_CHARGE_FULL=3898000
> >>> POWER_SUPPLY_CHARGE_FULL_DESIGN=3800000
> >>> POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=3000000
> >>> POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=12300000
> >>> POWER_SUPPLY_MANUFACTURE_YEAR=2017
> >>> POWER_SUPPLY_MANUFACTURE_MONTH=7
> >>> POWER_SUPPLY_MANUFACTURE_DAY=3
> >>> POWER_SUPPLY_MANUFACTURER=UR18650A
> >>> POWER_SUPPLY_MODEL_NAME=GEHC
> >> This patch landed in linux-next dated 20200529. Sadly it causes a
> >> regression on Samsung Exynos-based Chromebooks (Exynos5250 Snow,
> >> Exynos5420 Peach-Pi and Exynos5800 Peach-Pit). System boots to
> >> userspace, but then, when udev populates /dev, booting hangs:
> >>
> >> [??? 4.435167] VFS: Mounted root (ext4 filesystem) readonly on device
> >> 179:51.
> >> [??? 4.457477] devtmpfs: mounted
> >> [??? 4.460235] Freeing unused kernel memory: 1024K
> >> [??? 4.464022] Run /sbin/init as init process
> >> INIT: version 2.88 booting
> >> [info] Using makefile-style concurrent boot in runlevel S.
> >> [??? 5.102096] random: crng init done
> >> [....] Starting the hotplug events dispatcher: systemd-udevdstarting
> >> version 236
> >> [ ok .
> >> [....] Synthesizing the initial hotplug events...[ ok done.
> >> [....] Waiting for /dev to be fully populated...[?? 34.409914]
> >> TPS65090_RAILSDCDC1: disabling
> >> [?? 34.412977] TPS65090_RAILSDCDC2: disabling
> >> [?? 34.417021] TPS65090_RAILSDCDC3: disabling
> >> [?? 34.423848] TPS65090_RAILSLDO1: disabling
> >> [?? 34.429068] TPS65090_RAILSLDO2: disabling
> > :(
> >
> > log does not look useful either.
> >
> >> Bisect between v5.7-rc1 and next-20200529 pointed me to the first bad
> >> commit: [c4b12a2f3f3de670f6be5e96092a2cab0b877f1a] power: supply:
> >> sbs-battery: simplify read_read_string_data.
> > ok. I tested this on an to-be-upstreamed i.MX6 based system
> > and arch/arm/boot/dts/imx53-ppd.dts. I think the difference
> > is, that i2c-exynos5 does not expose I2C_FUNC_SMBUS_READ_BLOCK_DATA.
> > I hoped all systems using SBS battery support this, but now
> > I see I2C_FUNC_SMBUS_EMUL only supports writing block data.
> > Looks like I need to add another patch implementing that
> > using the old code with added PEC support.
> >
> > In any case that should only return -ENODEV for the property
> > (and uevent), but not break boot. So something fishy is going
> > on.
> >
> >> However reverting it in linux-next doesn't fix the issue, so the
> >> next commits are also relevant to this issue.
> > The next patch, which adds PEC support depends on the simplification
> > of sbs_read_string_data. The old, open coded variant will result in
> > PEC failure for string properties (which should not stop boot either
> > of course). Can you try reverting both?
> Indeed, reverting both (and fixing the conflict) restores proper boot.
Ok, I pushed out a revert of those two patches. They should land in
tomorrows linux-next release. Please test it.
> > If that helps I will revert those two instead of dropping the whole
> > series for this merge window.
> >
> >> Let me know how can I help debugging it.
> > I suspect, that this is userspace endlessly retrying reading the
> > battery uevent when an error is returned. Could you check this?
> > Should be easy to see by adding some printfs.
> I've added some debug messages in sbs_get_property() and it read the
> same properties many times. However I've noticed that if I wait long
> enough booting finally continues.
So basically userspace slows down itself massively by trying to
re-read uevent over and over when an error occurs. Does not seem
like a sensible thing to do. I will have a look at this when I find
some time.
-- Sebastian
Hi Sebastian,
On 02.06.2020 20:01, Sebastian Reichel wrote:
> On Tue, Jun 02, 2020 at 09:17:09AM +0200, Marek Szyprowski wrote:
>> On 01.06.2020 19:05, Sebastian Reichel wrote:
>>> On Mon, Jun 01, 2020 at 12:40:27PM +0200, Marek Szyprowski wrote:
>>>> On 13.05.2020 20:55, Sebastian Reichel wrote:
>>>>> This patchset improves support for SBS compliant batteries. Due to
>>>>> the changes, the battery now exposes 32 power supply properties and
>>>>> (un)plugging it generates a backtrace containing the following message
>>>>> without the first patch in this series:
>>>>>
>>>>> ---------------------------
>>>>> WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
>>>>> add_uevent_var: too many keys
>>>>> ---------------------------
>>>>>
>>>>> For references this is what an SBS battery status looks like after
>>>>> the patch series has been applied:
>>>>>
>>>>> cat /sys/class/power_supply/sbs-0-000b/uevent
>>>>> POWER_SUPPLY_NAME=sbs-0-000b
>>>>> POWER_SUPPLY_TYPE=Battery
>>>>> POWER_SUPPLY_STATUS=Discharging
>>>>> POWER_SUPPLY_CAPACITY_LEVEL=Normal
>>>>> POWER_SUPPLY_HEALTH=Good
>>>>> POWER_SUPPLY_PRESENT=1
>>>>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>>>>> POWER_SUPPLY_CYCLE_COUNT=12
>>>>> POWER_SUPPLY_VOLTAGE_NOW=11441000
>>>>> POWER_SUPPLY_CURRENT_NOW=-26000
>>>>> POWER_SUPPLY_CURRENT_AVG=-24000
>>>>> POWER_SUPPLY_CAPACITY=76
>>>>> POWER_SUPPLY_CAPACITY_ERROR_MARGIN=1
>>>>> POWER_SUPPLY_TEMP=198
>>>>> POWER_SUPPLY_TIME_TO_EMPTY_AVG=438600
>>>>> POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
>>>>> POWER_SUPPLY_SERIAL_NUMBER=0000
>>>>> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
>>>>> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
>>>>> POWER_SUPPLY_ENERGY_NOW=31090000
>>>>> POWER_SUPPLY_ENERGY_FULL=42450000
>>>>> POWER_SUPPLY_ENERGY_FULL_DESIGN=41040000
>>>>> POWER_SUPPLY_CHARGE_NOW=2924000
>>>>> POWER_SUPPLY_CHARGE_FULL=3898000
>>>>> POWER_SUPPLY_CHARGE_FULL_DESIGN=3800000
>>>>> POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=3000000
>>>>> POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=12300000
>>>>> POWER_SUPPLY_MANUFACTURE_YEAR=2017
>>>>> POWER_SUPPLY_MANUFACTURE_MONTH=7
>>>>> POWER_SUPPLY_MANUFACTURE_DAY=3
>>>>> POWER_SUPPLY_MANUFACTURER=UR18650A
>>>>> POWER_SUPPLY_MODEL_NAME=GEHC
>>>> This patch landed in linux-next dated 20200529. Sadly it causes a
>>>> regression on Samsung Exynos-based Chromebooks (Exynos5250 Snow,
>>>> Exynos5420 Peach-Pi and Exynos5800 Peach-Pit). System boots to
>>>> userspace, but then, when udev populates /dev, booting hangs:
>>>>
>>>> [ 4.435167] VFS: Mounted root (ext4 filesystem) readonly on device
>>>> 179:51.
>>>> [ 4.457477] devtmpfs: mounted
>>>> [ 4.460235] Freeing unused kernel memory: 1024K
>>>> [ 4.464022] Run /sbin/init as init process
>>>> INIT: version 2.88 booting
>>>> [info] Using makefile-style concurrent boot in runlevel S.
>>>> [ 5.102096] random: crng init done
>>>> [....] Starting the hotplug events dispatcher: systemd-udevdstarting
>>>> version 236
>>>> [ ok .
>>>> [....] Synthesizing the initial hotplug events...[ ok done.
>>>> [....] Waiting for /dev to be fully populated...[ 34.409914]
>>>> TPS65090_RAILSDCDC1: disabling
>>>> [ 34.412977] TPS65090_RAILSDCDC2: disabling
>>>> [ 34.417021] TPS65090_RAILSDCDC3: disabling
>>>> [ 34.423848] TPS65090_RAILSLDO1: disabling
>>>> [ 34.429068] TPS65090_RAILSLDO2: disabling
>>> :(
>>>
>>> log does not look useful either.
>>>
>>>> Bisect between v5.7-rc1 and next-20200529 pointed me to the first bad
>>>> commit: [c4b12a2f3f3de670f6be5e96092a2cab0b877f1a] power: supply:
>>>> sbs-battery: simplify read_read_string_data.
>>> ok. I tested this on an to-be-upstreamed i.MX6 based system
>>> and arch/arm/boot/dts/imx53-ppd.dts. I think the difference
>>> is, that i2c-exynos5 does not expose I2C_FUNC_SMBUS_READ_BLOCK_DATA.
>>> I hoped all systems using SBS battery support this, but now
>>> I see I2C_FUNC_SMBUS_EMUL only supports writing block data.
>>> Looks like I need to add another patch implementing that
>>> using the old code with added PEC support.
>>>
>>> In any case that should only return -ENODEV for the property
>>> (and uevent), but not break boot. So something fishy is going
>>> on.
>>>
>>>> However reverting it in linux-next doesn't fix the issue, so the
>>>> next commits are also relevant to this issue.
>>> The next patch, which adds PEC support depends on the simplification
>>> of sbs_read_string_data. The old, open coded variant will result in
>>> PEC failure for string properties (which should not stop boot either
>>> of course). Can you try reverting both?
>> Indeed, reverting both (and fixing the conflict) restores proper boot.
> Ok, I pushed out a revert of those two patches. They should land in
> tomorrows linux-next release. Please test it.
Today's linux-next (20200603) boots fine on the Samsung Exynos-based
Chromebooks. Let me know how if you need any help debugging the issues
to resurrect those patches.
>>> If that helps I will revert those two instead of dropping the whole
>>> series for this merge window.
>>>
>>>> Let me know how can I help debugging it.
>>> I suspect, that this is userspace endlessly retrying reading the
>>> battery uevent when an error is returned. Could you check this?
>>> Should be easy to see by adding some printfs.
>> I've added some debug messages in sbs_get_property() and it read the
>> same properties many times. However I've noticed that if I wait long
>> enough booting finally continues.
> So basically userspace slows down itself massively by trying to
> re-read uevent over and over when an error occurs. Does not seem
> like a sensible thing to do. I will have a look at this when I find
> some time.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland