I +1 the idea to use modules!

@yegortimoshenko ok, let's discuss some points. I want a to provide constructive arguments in my defense.

add E470 profile

there are many devices in thinkpad family. Let's add profiles only for buggy ones. Lenovo Z510 and Thinkpad T430s are good examples of profiles, which fix real problems with hardware.

OTOH, T440s and T440p are bad examples, in that they enable tpm-rng kernel module. Why? What is that? Shouldn't that be called tpm_rng instead? Which kernel package provides it? Also, it has some security controversies. There is no way to disable that module in end-configuration (this is because module system allows add values to list option, but doesn't allow remove them). It also has a TODO which implies this should be refactored eventually...

But if we add tpm_rng as an option to generic ThinkPad, we can define more details: add description, URLs on topic, specify which models this should work with, disable by default, additional settings (it is probably not wise to enable this option without setting rngd daemon).

set-and-forget whole-system hardware profiles

Having people use generic profiles is problematic, because we can't meaningfully fixup/workaround model-specific problems in the future.

I agree with latter, we want workaround model-specific problems later. But I don't see how making separate profile helps here. Instead we can do some runtime checks in "generic" model to apply fixes. In theory, we can also add an option:

options.hardware.laptop.model

and require user to set correct model ("E470" in my case), and then apply changes based on this.

I think a lot of this work better belongs to NixOS

Dunno. I'm not very fond of intel/Thinkpad specific workarounds in NixOS. Also, it's easier to define options here, in-place, than define them in nixpkgs and later declare them in nixos-hardware. It also adds dependency on specific nixpkgs revision/release - not a great thing.

Maybe later?

I think acpi_call availability is hardware-specific

It indeed is, only newer thinkpads support it. Though it is a must-have for TLP to work properly. 6 profiles had imported it explicitly. T430, T440p, T460s, X1, X250, X270 supports it, but didn't enable. Was that a bug or a feature? Nobody knows. I can only assume it was a bug. In that case 12 profiles need acpi_call. This now makes it a good candidate to be "default"!

If we never merge this, then potential users of NixOS on X1 can never find out that their TLP settings don't work properly. Or, worse, they can fix stuff themselves and not upstream here.

with no opinionated config options

the only opinionated settings are acpi_call and cpu.intel, but those are more "default", rather than "opinions". If we pick some random ThinkPad, it is 95% Intel based, and 80% new enough for acpi_call (very rough numbers). So generic module will work in 80% cases at least.

There is room for improvements here.

Overall, I think this is out of scope.

I wanted battery powersaving recommendations since this repo was introduced. So I'd like such "recommendations" to be part of this repo.

btw, cc @eyJhb and @azazel75 on X1 bug assumption. What is your nix-shell -p tlp --run 'sudo tlp stat | grep tpacpi-bat'?

EDIT: ah, X1 and X1 6th gen are different profiles, sorry. X1 6th gen does have acpi_call module

@danbst Thanks for doing this, I think this repo can be the right place to condense "sane/good defaults". The answer to your question is:

tpacpi-bat = active
tpacpi-bat.BAT0.startThreshold                              =     75 [%]
tpacpi-bat.BAT0.stopThreshold                               =     80 [%]
tpacpi-bat.BAT0.forceDischarge                              =      0

@danbst running with the below, but it is of course changed, and I run a Lenovo Thinkpad x230 ;)

tpacpi-bat = active
tpacpi-bat.BAT0.startThreshold                              =     60 [%]
tpacpi-bat.BAT0.stopThreshold                               =     85 [%]
tpacpi-bat.BAT0.forceDischarge                              =      0

OTOH, T440s and T440p are bad examples, in that they enable tpm-rng kernel module. Also, it has some security controversies.

Linux kernel policy is to use inputs from all available devices, including black box TPM instructions/modules like those on Intel chips (source, sorry it's inflammatory but it's the only one I could find).

Meaning, it's not really possible to control /dev/{u,}random sources and whether user trusts them, because of the kernel policy, not because of this repo. We just enable hardware support.

But if we add tpm_rng as an option to generic ThinkPad, we can define more details: add description, URLs on topic, specify which models this should work with, disable by default, additional settings (it is probably not wise to enable this option without setting rngd daemon).

Again, this is better suited for Nixpkgs. Fundamentally, the only reason people use this repo at all is so that they don't have to set up hardware themselves (we set up everything there is on the motherboard).

There is no way to disable that module in end-configuration (this is because module system allows add values to list option, but doesn't allow remove them).

boot.blacklistedKernelModules.

By the way, this is a solvable problem, NixOS module system can support exclusions without a major rewrite. As a temporary workaround, there are various excludePackages options.

options.hardware.laptop.model

User-defined enums are bad idea: there are quite a few ways to write down the same model. And no one will set it post-hoc, while we should be able to push quirk fixes to devices that might have previously not required any. We enforce leaf profiles so that we can dispatch workarounds based on specific model of the device.

Shouldn't that be called tpm_rng instead?

torvalds/linux@6e592a0#diff-7002202c012eaed630466c5a9d898562L318

I agree with latter, we want workaround model-specific problems later. But I don't see how making separate profile helps here. Instead we can do some runtime checks in "generic" model to apply fixes.

Literal runtime checks are the goal: much of what is not specific to a certain model can and should migrate to nixos-generate-config.

Let's add profiles only for buggy ones.

It's fundamentally unknowable which ones are, or will be buggy. It depends on kernel version: devices do break over time. Case in point, <nixos-hardware/toshiba/swanky> sound setup has changed a few times over recent kernel versions.

It indeed is, only newer thinkpads support it. Though it is a must-have for TLP to work properly. 6 profiles had imported it explicitly. T430, T440p, T460s, X1, X250, X270 supports it, but didn't enable. Was that a bug or a feature? Nobody knows. I can only assume it was a bug. In that case 12 profiles need acpi_call. This now makes it a good candidate to be "default"!

If we never merge this, then potential users of NixOS on X1 can never find out that their TLP settings don't work properly. Or, worse, they can fix stuff themselves and not upstream here.

It's debatable whether this is a good idea (there is a huge lot of ThinkPads in the wild that do not support acpi_call), that said it's just a question of profile inheritance, which is feasible with profile system.

I wanted battery powersaving recommendations since this repo was introduced. So I'd like such "recommendations" to be part of this repo.

I really think that Lenovo default (96-100%) could go into services.tlp, but 75/80% is extremely opinionated and should be set up explicitly with users knowing what they are doing (they are forfeiting 20% of their battery capacity).

services.tlp could accept an attrset with variables instead of types.lines so that overriding is easier. I am willing to help with this work.

Malicious inputs shouldn't be a problem as long as the PRNG is seeded with enough entropy from a secure source. As tpm is not the only source this isn't an issue.

Bad inputs can't hurt the security as long as the PRNG isn't broken. And users can input data to /dev/urandom anyways, so disabling the tpm wouldn't prevent an attack anyways.

A good summary of this is https://www.2uo.de/myths-about-urandom/

(source, sorry it's inflammatory but it's the only one I could find)

👍

boot.blacklistedKernelModules.

👍

By the way, this is a solvable problem, NixOS module system can support exclusions without a major rewrite.

I'd like to know details here.

torvalds/linux@6e592a0#diff-7002202c012eaed630466c5a9d898562L318

👍

It's debatable whether this is a good idea (there is a huge lot of ThinkPads in the wild that do not support acpi_call), that said it's just a question of profile inheritance, which is feasible with profile system.

Also, it is an open question if 6 profiles I've listed above are incomplete. Hardware supports acpi_call but profiles never specify that. Same should go about tpm-rng - what other profiles should include tpm-rng? Why would some include, other don't if hardware supports it? (https://pcsupport.lenovo.com/ua/en/downloads/ds501051 lists some TPM-enabled models, including T460s). Ideally we'd like to distinguish: "is supported", "is not supported" and "is unknown if is supported".

but 75/80% is extremely opinionated and should be set up explicitly with users knowing what they are doing

Ok.

Literal runtime checks are the goal: much of what is not specific to a certain model can and should migrate to nixos-generate-config.

BTW, it may be a good idea to teach nixos-generate-config --show-hardware-config to put device model automatically under hardware.model namespace. A quick check shows that

$ sudo dmidecode -H 0x000E | grep -e "Product Name" -e "Version" -e "Manufacturer"
        Manufacturer: LENOVO
        Product Name: 20H1S00A00
        Version: ThinkPad E470

returns enough information to use then in this repo

{ config, lib, ...}: let
  thinkpadModel = let
    matches = builtins.match "ThinkPad (.*)" config.hardware.model.Version;
  in if matches != null && matches != [] then head matches else null;

  modelHacks = {
    E470 = {
      ...
    };
    ...
  };
  
in {
  config = lib.mkMerge ...convert modelHacks into proper configs...;
}

We can go even further and detect CPU model in nixos-generate-config. Then we don't have to guess whether to enable Intel specifics or not.

maybe another time