| 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: <82ec6164-223b-940a-6992-48ccbe47a615@lenovo.com>
Date: Tue, 16 Nov 2021 15:52:52 -0500
From: Mark Pearson <markpearson@...ovo.com>
To: Thomas Weißschuh <linux@...ssschuh.net>,
<linux-pm@...r.kernel.org>, Sebastian Reichel <sre@...nel.org>,
<ibm-acpi-devel@...ts.sourceforge.net>,
<platform-driver-x86@...r.kernel.org>,
Mark Gross <markgross@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Henrique de Moraes Holschuh <hmh@....eng.br>
CC: <linux-kernel@...r.kernel.org>, <linrunner@....net>,
<bberg@...hat.com>, <hadess@...ess.net>,
<nicolopiazzalunga@...il.com>, <njoshi1@...ovo.com>,
<smclt30p@...il.com>
Subject: Re: [External] [PATCH 4/4] platform/x86: thinkpad_acpi: support
inhibit-charge
Hi Thomas,
On 2021-11-13 05:42, Thomas Weißschuh wrote:
> This adds support for the inhibit-charge charge_behaviour through the
> embedded controller of ThinkPads.
>
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
>
> ---
>
> This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/>> ---
> drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
> 1 file changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index e8c98e9aff71..7cd6475240b2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9321,6 +9321,8 @@ static struct ibm_struct mute_led_driver_data = {
> #define SET_STOP "BCSS"
> #define GET_DISCHARGE "BDSG"
> #define SET_DISCHARGE "BDSS"
> +#define GET_INHIBIT "PSSG"
> +#define SET_INHIBIT "BICS"
>
> enum {
> BAT_ANY = 0,
> @@ -9338,6 +9340,7 @@ enum {
> THRESHOLD_START,
> THRESHOLD_STOP,
> FORCE_DISCHARGE,
> + INHIBIT_CHARGE,
> };
>
> struct tpacpi_battery_data {
> @@ -9409,6 +9412,13 @@ static int tpacpi_battery_get(int what, int battery, int *ret)
> /* The force discharge status is in bit 0 */
> *ret = *ret & 0x01;
> return 0;
> + case INHIBIT_CHARGE:
> + /* This is actually reading peak shift state, like tpacpi-bat does */
> + if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
> + return -ENODEV;
> + /* The inhibit charge status is in bit 0 */
> + *ret = *ret & 0x01;
> + return 0;
> default:
> pr_crit("wrong parameter: %d", what);
> return -EINVAL;
> @@ -9447,6 +9457,22 @@ static int tpacpi_battery_set(int what, int battery, int value)
> return -ENODEV;
> }
> return 0;
> + case INHIBIT_CHARGE:
> + /* When setting inhibit charge, we set a default value of
> + * always breaking on AC detach and the effective time is set to
> + * be permanent.
> + * The battery ID is in bits 4-5, 2 bits,
> + * the effective time is in bits 8-23, 2 bytes.
> + * A time of FFFF indicates forever.
> + */
> + param = value;
> + param |= battery << 4;
> + param |= 0xFFFF << 8;
> + if (ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_INHIBIT, &ret, param))) {
> + pr_err("failed to set inhibit charge on %d", battery);
> + return -ENODEV;
> + }
> + return 0;
> default:
> pr_crit("wrong parameter: %d", what);
> return -EINVAL;
> @@ -9467,6 +9493,8 @@ static int tpacpi_battery_probe(int battery)
> * 4) Check for support
> * 5) Get the current force discharge status
> * 6) Check for support
> + * 7) Get the current inhibit charge status
> + * 8) Check for support
> */
> if (acpi_has_method(hkey_handle, GET_START)) {
> if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
> @@ -9513,6 +9541,16 @@ static int tpacpi_battery_probe(int battery)
> battery_info.batteries[battery].charge_behaviours |=
> BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE);
> }
> + if (acpi_has_method(hkey_handle, GET_INHIBIT)) {
> + if (ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, &ret, battery))) {
> + pr_err("Error probing battery inhibit charge; %d\n", battery);
> + return -ENODEV;
> + }
> + /* Support is marked in bit 5 */
> + if (ret & BIT(5))
> + battery_info.batteries[battery].charge_behaviours |=
> + BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE);
> + }
>
> battery_info.batteries[battery].charge_behaviours |=
> BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO);
> @@ -9673,6 +9711,11 @@ static ssize_t charge_behaviour_show(struct device *dev,
> return -ENODEV;
> if (ret)
> active = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
> + } else if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)) {
> + if (tpacpi_battery_get(INHIBIT_CHARGE, battery, &ret))
> + return -ENODEV;
> + if (ret)
> + active = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
> }
>
> return power_supply_charge_behaviour_show(dev, available, active, buf);
> @@ -9710,12 +9753,20 @@ static ssize_t charge_behaviour_store(struct device *dev,
> switch (selected) {
> case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
> ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
> - if (ret < 0)
> + ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
> + if (ret)
> return ret;
> break;
> case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
> ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 1);
> - if (ret < 0)
> + ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
> + if (ret)
> + return ret;
> + break;
> + case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
> + ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
> + ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 1) || ret;
> + if (ret)
> return ret;
> break;
> default:
>
I can confirm the bit fields are correct for these calls (as for the
previous patch)
Couple of things to note, based on feedback previously from the FW team
that I found when digging thru my battery related emails.
"Lenovo doesn't officially support the use of these calls - they're
intended for internal use" (at this point in time - there is some
discussion about battery recalibration support but I don't have details
I can share there yet).
The FW team also noted that:
"Actual battery charging/discharging behaviors are managed by ECFW. So
the request of BDSS/BICS method may be rejected by ECFW for the current
battery mode/status. You have to check if the request of the method is
actually applied or not"
So for the calls above (and for the BDSS calls in the previous patch) it
would be good to do a read back of the setting to ensure it has
completed successfully.
Hope that helps
Mark
Powered by blists - more mailing lists