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

added debian-related instructions and fixed some arch-only lines #556

Closed
wants to merge 29 commits into from

Conversation

POMATu
Copy link

@POMATu POMATu commented Jul 1, 2024

after my success with making suspend work on debian bookworm same way it works on arch I have added debian-related instructions and fixed some of the confusing arch-only lines
I think its important to include my .config for compiling current kernel on debian because i think this is the main reason why suspend works for me but doesnt works on any of the current kernels from repos
you might remove it later when there will be suspend-enabled packages for debian-type distros in the repos

the kernel i reference is:
Linux macbookpro 6.9.7 #1 SMP PREEMPT_DYNAMIC Sun Jun 30 09:07:34 EDT 2024 x86_64 GNU/Linux

POMATu and others added 4 commits July 1, 2024 15:33
fixed real and actual list of packages that needed to compile on debian bookworm (and probably ubuntu too)
added specific steps for debian-type distros which differs from arch
many changes regarding debian bookworm suspend and touchbar
Copy link
Member

@AdityaGarg8 AdityaGarg8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing of the scripts is not proper. Your lint will fail if I approve this.

docs/guides/kernel.md Outdated Show resolved Hide resolved
docs/guides/postinstall.md Outdated Show resolved Hide resolved
docs/guides/postinstall.md Outdated Show resolved Hide resolved
POMATu and others added 14 commits July 2, 2024 12:07
no need for sudo here also made it similar to arch command
reverted table back
tee and spacing
another ident
udev rule and fixed typo
kernel blacklist didnt work for me so networkmanager fix
removed kernel config
added service touchbar-poke
@POMATu
Copy link
Author

POMATu commented Jul 2, 2024

fixed all from request and added some more useful stuff

actual patching instructions
docs/guides/kernel.md Outdated Show resolved Hide resolved
docs/guides/postinstall.md Outdated Show resolved Hide resolved
```
and fix the pathes in service script if they differ for your system

4. If you having problems with touchbar being dead after restoring state from suspend then you might as well try the following version of the script:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of writing the almost same thing again, add instructions how to modify the previous service file

To check, run: `zcat /proc/config.gz | grep "CONFIG_MODULE_FORCE_UNLOAD"`
This seems to be working with `CONFIG_MODULE_FORCE_UNLOAD=y` in the kernel config when kernel is compiled with [kernel compilation instructions](https://wiki.t2linux.org/guides/kernel/).

To check, run:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again bad spacing. See how notes are written. https://squidfunk.github.io/mkdocs-material/reference/admonitions/#usage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't fixed the spacing yet.


Without this config option you wont be able to unload the required modules, they will be busy.

6. If touchbar occasionally does not work on boot but works after suspend+restore then you can place this workaround somewhere late after boot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it be better to just keep this and remove the extra suspend.service you made here https://github.com/t2linux/wiki/pull/556/files#diff-a9bb6c3fdd453b5d31bf4ce51c2122abccd3d3f1d21d0c2c78f5e783a7bc1b87R212

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this hasn't been addressed yet

removed
git checkout 0ad2b3913f5484ba8e86b6965f5d88903464261d
removed networkmanager config
rewrote service instructions
fix spacing
removed touchbar-poke and simplified
@POMATu
Copy link
Author

POMATu commented Jul 4, 2024

ok i removed touchbar-poke.service since it failed for me at least once and simplified the footer of the file. Maybe worth including instructions how to bind that script to power button to solve any kind of occasional kernel module reloading issues: whether its restoring from suspend or tiny-dfr being dead.

@sharpenedblade
Copy link
Contributor

ok i removed touchbar-poke.service since it failed for me at least once and simplified the footer of the file. Maybe worth including instructions how to bind that script to power button to solve any kind of occasional kernel module reloading issues: whether its restoring from suspend or tiny-dfr being dead.

tiny-dfr can be fixed in less hacky ways. Its better to get tiny-dfr working by itself, the code is pretty simple so its doable.

docs/guides/kernel.md Outdated Show resolved Hide resolved
docs/guides/postinstall.md Outdated Show resolved Hide resolved
docs/guides/postinstall.md Outdated Show resolved Hide resolved

!!! note
This seems to be working only on Arch with `CONFIG_MODULE_FORCE_UNLOAD=y` in the kernel config.
To check, run: `zcat /proc/config.gz | grep "CONFIG_MODULE_FORCE_UNLOAD"`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blank line here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not resolved

To check, run: `zcat /proc/config.gz | grep "CONFIG_MODULE_FORCE_UNLOAD"`
This seems to be working with `CONFIG_MODULE_FORCE_UNLOAD=y` in the kernel config when kernel is compiled with [kernel compilation instructions](https://wiki.t2linux.org/guides/kernel/).

To check, run:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't fixed the spacing yet.


Without this config option you wont be able to unload the required modules, they will be busy.

6. If touchbar occasionally does not work on boot but works after suspend+restore then you can place this workaround somewhere late after boot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this hasn't been addressed yet


Without this config option you wont be able to unload the required modules, they will be busy.

5. If touchbar occasionally does not work on boot but works after suspend+restore then you can place this workaround somewhere late after boot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why brcmfmac is touchbar? Both have no relation to each other.

```bash
modprobe -r hid_appletb_kbd
modprobe -r brcmfmac_wcc
modprobe -r brcmfmac
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why apple-bce is being removed?

modprobe brcmfmac
modprobe brcmfmac_wcc

touchbar --restart
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

touchbar --restart is an Ubuntu only command. Make it disable and enable tiny-dfr.service instead.

@@ -142,6 +142,7 @@ blacklist cdc_mbim" >> /etc/modprobe.d/blacklist.conf'

Please note that this internal ethernet interface is required for various services including touchid that there currently is no Linux support for. In the future, if any of these services are supported, you'll need to undo this.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra line is unnecessary


!!! note
This seems to be working only on Arch with `CONFIG_MODULE_FORCE_UNLOAD=y` in the kernel config.
To check, run: `zcat /proc/config.gz | grep "CONFIG_MODULE_FORCE_UNLOAD"`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not resolved

@@ -39,10 +38,24 @@ done
### Setting kernel configuration

!!! Info "Using config from lower kernel versions"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why extra line here? Remove this.

@AdityaGarg8
Copy link
Member

Extra line as per https://squidfunk.github.io/mkdocs-material/reference/admonitions/ after !!!note is not used in the wiki

@POMATu
Copy link
Author

POMATu commented Jul 11, 2024

apparently working with people is not my thing, i am afraid i got bored long time ago and already missed the point what I wanted to contribute with this pull request
I got my suspend working on my debian I am grateful for that, maybe I can explain to someone in #debian discord if they ask, but contributing thru exam-type moderation i am not interested, closing

@POMATu POMATu closed this Jul 11, 2024
@AdityaGarg8
Copy link
Member

apparently working with people is not my thing, i am afraid i got bored long time ago and already missed the point what I wanted to contribute with this pull request I got my suspend working on my debian I am grateful for that, maybe I can explain to someone in #debian discord if they ask, but contributing thru exam-type moderation i am not interested, closing

No worries. Thanks for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants