Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: use upcoming kernel interface for battery threshold configuration #321

Open
t-8ch opened this Issue Feb 8, 2018 · 77 comments

Comments

@t-8ch
Copy link

commented Feb 8, 2018

The upcoming 4.17 kernel (maybe even 4.16) will contain a new in-kernel API to expose information and controls like the battery threshold to userspace. [0]
It also comes with a reference implementation for new thinkpads (using the same logic as tpacpi-bat)

This could be used by TLP to replace tpacpi-bat.
(Maybe even a driver for the batteries supported by tp_smapi emerges)

I am willing to contribute a driver.
Should the driver be in addition to tpacpi-bat or will one implementation be enough?
(If the new logic in TLP is released after the kernel there shouldn't be a problem with dropping support for tpacpi-bat)

[0] https://patchwork.kernel.org/patch/10205339/
[1] https://patchwork.kernel.org/patch/10205347/
[2] https://patchwork.kernel.org/patch/10205353/
[3] https://patchwork.kernel.org/patch/10205359/

@linrunner

This comment has been minimized.

Copy link
Owner

commented Feb 8, 2018

Looks interesting, but it's only infrastructure. ~~The comment in [0] says

The thinkpad_acpi module can use this API to expose
sysfs attributes that it controls inside the ACPI battery driver
sysfs tree, under /sys/class/power_supply/BATN/.

So someone must actually add this to thinkpad_acpi. I remember Julian Andres Klode attempted to integrate thresholds in thinkpad_acpi years ago, but never finished the task.

tpacpi-bat is not a [kernel] driver. With sysfiles in place below /sys/class/power_supply/BATN/ there is no need for additional tools. TLP can write them itself.

@linrunner

This comment has been minimized.

Copy link
Owner

commented Feb 8, 2018

OK, OK. I missed the thinkpad_acpi implementation in [1] :-)

@t-8ch

This comment has been minimized.

Copy link
Author

commented Feb 8, 2018

With "driver"to implement I meant "logic inside TLP to talk to the new API".
(But if you want to implement it youself, even better :-) )

@linrunner

This comment has been minimized.

Copy link
Owner

commented Feb 9, 2018

You're welcome of course :-).

@t-8ch

This comment has been minimized.

Copy link
Author

commented Feb 9, 2018

Hm, the new API is still missing the force discharge and inhibit charge methods.
Maybe we wait for them and then throw out tpacpi-bat?

@linrunner

This comment has been minimized.

Copy link
Owner

commented Feb 9, 2018

A threefold selection logic ( tp-smapi - tpapci-bat - new API ) is needed anyway, not all people will have the new kernel at once. Maybe you could ask the kernel devs about force_discharge?

@t-8ch

This comment has been minimized.

Copy link
Author

commented Feb 9, 2018

Urgh :-)

We could split it:

  • I will get the other methods into the kernel.
  • You do the logic inside TLP.
@linrunner

This comment has been minimized.

Copy link
Owner

commented Feb 9, 2018

Fair enough :-)

@smclt30p

This comment has been minimized.

Copy link

commented Feb 9, 2018

Developer of the kernel driver here.

I implemented a whole new generic extension system for the ACPI battery driver inside the kernel, and once that gets sanitized and merged into mainline, I then will move onto the other stuff, like force discharge and inhibit charge as a separate patch. Those patches will be small as most of the infrastructure is inside this patch series, mainly 1/4. I also have stable, official documentation from Lenovo about the ACPI firmware API, so it WILL be implemented and will be stable.

Keep your heads up guys, the future is bright 👍

@t-8ch

This comment has been minimized.

Copy link
Author

commented Feb 10, 2018

@smclt30p Great! Thanks for your work.
Does your source of official documentation by any chance include hardware errata?
(Asking for a friend ...)
Sorry for being offtopic.

@smclt30p

This comment has been minimized.

Copy link

commented Feb 10, 2018

@smclt30p

This comment has been minimized.

Copy link

commented Feb 27, 2018

Update guys:

The patch has been merged and is coming in Linux 4.17:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/platform/x86/thinkpad_acpi.c?h=next-20180226#n9216

Following the release of 4.17, I will build the inhibit_charge and force_discharge methods.
Stay tuned!

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 2, 2018

Great :-)

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 12, 2018

@smclt30p : I need a "nickname" for the new API to distinguish it from tp-smapi and tpacpi-bat. Any thoughts?

@smclt30p

This comment has been minimized.

Copy link

commented Mar 12, 2018

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 12, 2018

Sounds a bit ... generic.

@smclt30p

This comment has been minimized.

Copy link

commented Mar 12, 2018

Well it's what it basically is. And this obsoletes tpacpi-bat basically, so do tpacpi-native.

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 13, 2018

Well, what about "acpibat-native" or abbreviated "acpibat"?

@linrunner

This comment has been minimized.

Copy link
Owner

commented Apr 5, 2018

@smclt30p :

Following the release of 4.17, I will build the inhibit_charge and force_discharge methods.

Will the files actually be called inhibit_charge and force_discharge?

@smclt30p

This comment has been minimized.

Copy link

commented Apr 6, 2018

Yes.

@smclt30p

This comment has been minimized.

Copy link

commented Apr 6, 2018

But we have hit a brick wall, so far the "status" attribute is broken because one of the patches got reverted because UPower is broken. And no upower maintainer is active?

See: https://bugs.freedesktop.org/show_bug.cgi?id=105482

@t-8ch

This comment has been minimized.

Copy link
Author

commented Apr 6, 2018

The linked issue is assigned to @hughsie, who is definitively still around.

@hughsie

This comment has been minimized.

Copy link

commented Apr 6, 2018

Can you define a bit what you mean by "upower is broken"?

@smclt30p

This comment has been minimized.

Copy link

commented Apr 6, 2018

@linrunner

This comment has been minimized.

Copy link
Owner

commented Apr 9, 2018

Just for my own reference: above patches as commits in Linus' tree (4.17)

[0] battery: Add the battery hooking API
[1] pm: add to_power_supply macro to the API
[2] thinkpad_acpi: Add support for battery thresholds
[3] battery: Add the ThinkPad "Not Charging" quirk
Revert of [3]

@smclt30p : doesn't the whole thing work even without the last patch for the status? I'm used to reading "unknown" when thresholds are effective.

@smclt30p

This comment has been minimized.

Copy link

commented Apr 9, 2018

Yes that patch has been reverted because it breaks upower and the userspace. It needs a new revision. I will look into it later this week.

@smclt30p

This comment has been minimized.

Copy link

commented Apr 10, 2018

I have submitted a new version of the status quirk: https://patchwork.kernel.org/patch/10332631/

Let's see how this goes. Do you have any comments about the above patch?

@linrunner

This comment has been minimized.

Copy link
Owner

commented Apr 17, 2018

@smclt30p : Patch works for me (T450s, 4.17-rc1).

Nevertheless the above upower bug report should be pursued further. I'd prefer having a distinct state like "not charging" when a threshold is effective.

@smclt30p

This comment has been minimized.

Copy link

commented Apr 18, 2018

@linrunner

This comment has been minimized.

Copy link
Owner

commented Aug 31, 2018

@t-8ch : great :-)

@mcsurly

This comment has been minimized.

Copy link

commented Oct 3, 2018

Soooo... @linrunner with the fix for multi-battery probing going in 4.19, and your BAT0 force_discharge patch going in as well.... when 4.19 hits Fedora would you be ready to make a release 1.1.1 or 1.2?
I think you could set NATACPI_ENABLE=1 by default at that point as well, but ... I'm comfortable changing that setting myself!
I think there is a lot of appeal of having TLP able to save battery wear on ThinkPads without requiring you to add another yum repo (and akmod kernel modeul) to pick up acpi_call.

@linrunner

This comment has been minimized.

Copy link
Owner

commented Oct 4, 2018

and your BAT0 force_discharge patch going in as well....

I didn't see the force_discharge patches from @smclt30p being merged. Do you have a reference?

@smclt30p

This comment has been minimized.

Copy link

commented Oct 4, 2018

@pschichtel

This comment has been minimized.

Copy link

commented Jan 15, 2019

Seems like the patch landed in 4.19.

@linrunner

This comment has been minimized.

Copy link
Owner

commented Feb 21, 2019

I'm trying to get the patch for multi-battery probing into Ubuntu's 4.18 too:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1812099

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 1, 2019

It's been commited: https://kernel.ubuntu.com/git/ubuntu/ubuntu-cosmic.git/commit/?h=master-next&id=f748e09ec66c2894e5b9e6b4582f90d158b1ef6e

ps. sooner or later it will find its way into 18.04 HWE too.

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 22, 2019

Just for reference the orphaned patches for force_discharge and inhibit_charge:

[4] https://patchwork.kernel.org/patch/10396259/
[5] https://patchwork.kernel.org/patch/10396265/
[6] https://patchwork.kernel.org/patch/10396271/

plus my modification of [5]

[5a] https://gist.github.com/linrunner/1e343a0125a14448246300a8106dfcf5

@smclt30p politely declined to carry on with them.

@smclt30p

This comment has been minimized.

Copy link

commented Mar 22, 2019

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 22, 2019

Of course they work, that's why I want them merged ...

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 22, 2019

@t-8ch : Thomas, can you help?

@t-8ch

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

I will try. I already tried to get the information from Lenovo (I'd like to know what I'm submitting), but it got a little bit stuck in discussions.
Will try again.

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 22, 2019

@smclt30p : are you permitted to forward @t-8ch the Lenovo docs?

@smclt30p

This comment has been minimized.

Copy link

commented Mar 22, 2019

@t-8ch

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

I will take a look, but I'd really prefer to have something to argue with.

@linrunner

This comment has been minimized.

Copy link
Owner

commented Mar 25, 2019

List of todos for the driver:

  • Kernel bug 202619: thinkpad_acpi fills logfile –> all older, unsupported ThinkPads
  • X1C6, E580: cannot reset stop thresh to 100 –> FAQ
@xiaoping378

This comment has been minimized.

Copy link

commented Apr 2, 2019

@linrunner so, the X1C6 can't use recalibrate even with 1.2??

@linrunner

This comment has been minimized.

Copy link
Owner

commented Apr 2, 2019

@xiaoping378 : the change is currently only available in the master branch. 1.1 works. Do you have a X1C6? Did you test?

@xiaoping378

This comment has been minimized.

Copy link

commented Apr 2, 2019

@linrunner
using x1c6 on ubuntu 18.04 (kernel 4.18.0-16),

sudo add-apt-repository ppa:linrunner/tlp
sudo apt-get install tlp tlp-rdw tp-smapi-dkms acpi-call-dkms

after enable CHARGE_THRESH, fullcharge and setcharge don't work, Fllow this restored it.

Now using dev-1.2 to recalibrate batteray


update:
recalibrate worked.
setcharge and fullcharage don't work(only worked once.)

@xiaoping378

This comment has been minimized.

Copy link

commented Apr 2, 2019

i used the old config file without the below configuration

NATACPI_ENABLE=1
TPACPI_ENABLE=1
TPSMAPI_ENABLE=1

added them, setcharge worked, ... fullcharge didn't work, anyway, setcharge can do everything for me.
thanks very much for your great work.

@linrunner

This comment has been minimized.

Copy link
Owner

commented Apr 2, 2019

@xiaoping378 : "fullcharge didn't work" is not clear to me. There is no difference between tlp fullchargeand tlp setcharge 96 100 regarding the X1C6 problem described in FAQ. Both commands won't reset the stop threshold on my X1C6.

May I ask you to open a separate issue giving

  • the exact commands you used
  • their outputs
  • and the output of tlp-stat -b right after both of your commands?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.