| 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: <3a0910c4-87f4-4c0f-b6b6-3f7bf553a5f0@collabora.com>
Date: Fri, 8 Mar 2024 10:26:29 +0100
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] power: supply: sbs-battery: Handle unsupported
PROP_TIME_TO_EMPTY_NOW
Il 07/03/24 23: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
>
> Identify during probe, based on the battery model, if this is one of the
> quirky batteries, and if so, don't register the
> POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW property.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> ---
> drivers/power/supply/sbs-battery.c | 45 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index a6c204c08232..85ff331cf87a 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -1112,6 +1112,49 @@ static const struct power_supply_desc sbs_default_desc = {
> .external_power_changed = sbs_external_power_changed,
> };
>
> +static const char * const unsupported_time_to_empty_now_models[] = {
> + "AP16L5J", "AP15O5L", "AP16L8J", "AP18C4K", "GG02047XL"
> +};
I think that you must make sure that this is seen as a quirk, rather than
"something normal" because - as you said - the SBS specification says that
the TIME_TO_EMPTY_NOW is *required*, so, this is a *deviation* from the spec
(so, the SBS implementation in those devices is *out of spec*!).
static const char * const quirk_remove_time_to_empty_now_models
or quirk_unsupported_time_to_empty_now_models
..the former, if you want to avoid having a variable name that is 5000 characters
long (:-P); the latter, if you don't care about that (there's no rule anyway).
Besides that, since I didn't like what I just saw, I looked for different
alternatives; the only other one is to de-constify the sbs_properties[] array,
which is something that I also dislike anyway.
I'm not sure if deconstifying that could be acceptable, but if it would, you
would be able to remove-reorder without copies of this and that.
Anyway - the only thing I really want here is to make sure that this is seen
as a quirk and a clear deviation from the specification - everything else is
a plus, and not really a blocker for me.
> +
> +static void sbs_remove_unsupported_properties(struct power_supply_config *psy_cfg,
> + struct power_supply_desc *sbs_desc)
P.S.: sbs_quirk_remove_unsupported_properties :-)
Cheers,
Angelo
> +{
> + enum power_supply_property *new_properties;
> + struct sbs_info *chip = psy_cfg->drv_data;
> + bool missing_time_to_empty_now = false;
> + const char *model_name;
> + unsigned int new_num_properties;
> + unsigned int i = 0, j = 0;
> +
> + model_name = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
> + if (IS_ERR(model_name))
> + return;
> +
> + for (i = 0; i < ARRAY_SIZE(unsupported_time_to_empty_now_models); i++) {
> + if (!strcmp(model_name, unsupported_time_to_empty_now_models[i])) {
> + missing_time_to_empty_now = true;
> + break;
> + }
> + }
> +
> + if (!missing_time_to_empty_now)
> + return;
> +
> + new_num_properties = ARRAY_SIZE(sbs_properties) - 1;
> + new_properties = devm_kzalloc(&chip->client->dev, new_num_properties * sizeof(sbs_properties[0]), GFP_KERNEL);
> +
> + for (i = 0; i < sbs_desc->num_properties; i++) {
> + if (sbs_desc->properties[i] == POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW)
> + continue;
> +
> + new_properties[j] = sbs_desc->properties[i];
> + j++;
> + }
> +
> + sbs_desc->properties = new_properties;
> + sbs_desc->num_properties = new_num_properties;
> +};
> +
> static int sbs_probe(struct i2c_client *client)
> {
> struct sbs_info *chip;
> @@ -1210,6 +1253,8 @@ static int sbs_probe(struct i2c_client *client)
> if (rc)
> return rc;
>
> + sbs_remove_unsupported_properties(&psy_cfg, sbs_desc);
> +
> chip->power_supply = devm_power_supply_register(&client->dev, sbs_desc,
> &psy_cfg);
> if (IS_ERR(chip->power_supply))
>
> ---
> base-commit: 11afac187274a6177a7ac82997f8691c0f469e41
> change-id: 20240307-sbs-time-empty-now-error-322bc074d3f2
>
> Best regards,
Powered by blists - more mailing lists