| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9ba1b4df-78dc-4f24-bb61-d7b168da18f8@collabora.com>
Date: Tue, 16 Apr 2024 09:46:56 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Nícolas F. R. A. Prado <nfraprado@...labora.com>,
Sebastian Reichel <sre@...nel.org>
Cc: kernel@...labora.com, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] power: supply: sbs-battery: Handle unsupported
PROP_TIME_TO_EMPTY_NOW
Il 15/04/24 21:05, Nícolas F. R. A. Prado ha scritto:
> Despite the RunTimeToEmpty() (0x11) function being defined in the SBS
> specification as required, it seems that not all batteries implement it.
> On platforms with such batteries, reading the property will cause an
> error to be printed:
>
> power_supply sbs-8-000b: driver failed to report `time_to_empty_now' property: -5
>
> This not only pollutes the log, distracting from real problems on the
> device, but also prevents the uevent file from being read since it
> contains all properties, including the faulty one.
>
> The following table summarizes the findings for a handful of platforms:
>
> Platform Status Manufacturer Model
> ------------------------------------------------------------------------
> mt8186-corsola-steelix-sku131072 OK BYD L22B3PG0
> mt8195-cherry-tomato-r2 NOT OK PANASON AP16L5J
> mt8192-asurada-spherion-r0 NOT OK PANASON AP15O5L
> mt8183-kukui-jacuzzi-juniper-sku16 NOT OK LGC KT0 AP16L8J
> mt8173-elm-hana OK Sunwoda L18D3PG1
> sc7180-trogdor-lazor-limozeen-nots-r5 NOT OK Murata AP18C4K
> sc7180-trogdor-kingoftown NOT OK 333-AC-0D-A GG02047XL
> rk3399-gru-kevin OK SDI 4352D51
>
> Detect if this is one of the quirky batteries during presence update, so
> that hot-plugging works as expected, and if so report -ENODATA for
> POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW, which removes it from uevent and
> prevents throwing errors.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> ---
> Changes in v2:
> - Reworked patch to lay down and use a proper quirk infrastructure, and
> update the quirks on the presence update callback so it works properly
> even when hot-plugging different batteries
> - Link to v1: https://lore.kernel.org/r/20240307-sbs-time-empty-now-error-v1-1-18d0f8702330@collabora.com
> ---
> drivers/power/supply/sbs-battery.c | 55 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index a6c204c08232..92acbda9e78e 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -214,6 +214,7 @@ struct sbs_info {
> struct delayed_work work;
> struct mutex mode_lock;
> u32 flags;
> + u32 quirks;
> int technology;
> char strings[NR_STRING_BUFFERS][I2C_SMBUS_BLOCK_MAX + 1];
> };
> @@ -263,6 +264,54 @@ static void sbs_disable_charger_broadcasts(struct sbs_info *chip)
> dev_dbg(&chip->client->dev, "%s\n", __func__);
> }
>
> +/* Required by the spec, but missing in some implementations */
> +#define SBS_QUIRK_BROKEN_TTE_NOW BIT(0)
> +
> +struct sbs_quirk_entry {
> + const char *manufacturer;
> + const char *model;
> + u32 flags;
> +};
> +
> +static const struct sbs_quirk_entry sbs_quirks[] = {
> + {"PANASON", "AP16L5J", SBS_QUIRK_BROKEN_TTE_NOW},
> + {"PANASON", "AP15O5L", SBS_QUIRK_BROKEN_TTE_NOW},
> + {"LGC KT0", "AP16L8J", SBS_QUIRK_BROKEN_TTE_NOW},
> + {"Murata", "AP18C4K", SBS_QUIRK_BROKEN_TTE_NOW},
> + {"333-AC-0D-A", "GG02047XL", SBS_QUIRK_BROKEN_TTE_NOW},
> +};
> +
> +static const char *sbs_get_constant_string(struct sbs_info *chip,
> + enum power_supply_property psp);
> +
> +static void sbs_update_quirks(struct sbs_info *chip)
> +{
> + const char *model;
> + const char *manufacturer;
> + unsigned int i = 0;
Please reorder:
const char *manufacturer;
const char *model;
unsigned int i;
..and please remove (like shown, of course) the double initialization of the
`i` variable, as you're initializing it again later in your for loop.
> +
> + /* reset quirks from battery before the hot-plug event */
> + chip->quirks = 0;
> +
> + manufacturer = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MANUFACTURER);
> + model = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
> + if (IS_ERR(manufacturer) || IS_ERR(model)) {
> + dev_warn(&chip->client->dev, "Couldn't read manufacturer and model to set quirks\n");
> + return;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(sbs_quirks); i++) {
> + if (strcmp(manufacturer, sbs_quirks[i].manufacturer))
> + continue;
> + if (strcmp(model, sbs_quirks[i].model))
> + continue;
> + chip->quirks |= sbs_quirks[i].flags;
> + }
> +
> + if (chip->quirks & SBS_QUIRK_BROKEN_TTE_NOW)
> + dev_info(&chip->client->dev, "Added quirk disabling TIME_TO_EMPTY_NOW\n");
I don't really expect many quirks, but having a dev_info() print for every
quirk that gets set would make this driver too chatty, IMO.
Please, either turn that into a dev_dbg() or print a mask of the quirks .. or both.
Cheers,
Angelo
> +}
> +
> static int sbs_update_presence(struct sbs_info *chip, bool is_present)
> {
> struct i2c_client *client = chip->client;
> @@ -323,6 +372,8 @@ static int sbs_update_presence(struct sbs_info *chip, bool is_present)
> dev_dbg(&client->dev, "PEC: %s\n", (client->flags & I2C_CLIENT_PEC) ?
> "enabled" : "disabled");
>
> + sbs_update_quirks(chip);
> +
> if (!chip->is_present && is_present && !chip->charger_broadcasts)
> sbs_disable_charger_broadcasts(chip);
>
> @@ -614,6 +665,10 @@ static int sbs_get_battery_property(struct i2c_client *client,
> struct sbs_info *chip = i2c_get_clientdata(client);
> s32 ret;
>
> + if (psp == POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW &&
> + chip->quirks & SBS_QUIRK_BROKEN_TTE_NOW)
> + return -ENODATA;
> +
> ret = sbs_read_word_data(client, sbs_data[reg_offset].addr);
> if (ret < 0)
> return ret;
>
> ---
> base-commit: 11afac187274a6177a7ac82997f8691c0f469e41
> change-id: 20240307-sbs-time-empty-now-error-322bc074d3f2
>
> Best regards,
Powered by blists - more mailing lists