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

Add support for alternate applesmc module device path. #236

Merged
merged 1 commit into from
Apr 2, 2023

Conversation

robabram
Copy link

@robabram robabram commented Sep 27, 2021

In newer versions of Linux and/or Apple devices, the applesmc module uses a different "/sys/devices" path to map the fan controller to. This seems to be due to changes in how ACPI devices are mapped to the "/sys/devices" directory.

changes

If the original hardcoded "applesmc" device path is not found, dynamically look for the device path to the "applesmc" device root by searching sub-directories of "/sys/bus/acpi/drivers/applesmc". Once the device path has been found, setup the fan device path root.

testing

Tested on Macbook Pro 2019 (16,1), Fedora 33, Kernel: 5.13.12-200.mbp15.fc33.x86_64

@satmandu
Copy link

Could we get this merged? This fixes mbpfan on my MBP11,3 with kernel 5.17-rc5.

@satmandu
Copy link

@gaul The debian/ubuntu packages are broken on newer kernels w/o this fix.

@@ -52,6 +53,7 @@

#define CORETEMP_PATH "/sys/devices/platform/coretemp.0"
#define APPLESMC_PATH "/sys/devices/platform/applesmc.768"
#define ALT_APPLESMC_PATH "/sys/bus/acpi/drivers/applesmc"
Copy link
Member

Choose a reason for hiding this comment

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

Is this the new, preferred path, or just some weird quirk of Debian/Ubuntu?

Choose a reason for hiding this comment

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

On the MacBookPro11,3, /sys/devices/platform/applesmc.768 is still being used, and /sys/bus/acpi/drivers/applesmc is empty, but since that is a mid-2014 machine, things may be different for newer macs.

(I will say that the stock mbpfan package available in ubuntu 22.04 was not working to set the fan speed on my MacBookPro11,3 though, whereas building with this patch does set the fan speeds according to my settings in the mbpfan.conf file.)

Choose a reason for hiding this comment

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

(so maybe one of the other changes... or doing a rebuild for 22.04 made the package work.)

Copy link
Member

Choose a reason for hiding this comment

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

I tested my MacBook Air 2014 with 5.16.8-200.fc35.x86_64 and /sys/devices/platform/applesmc.768 has the files that mbpfan needs. Does kernel 5.17 change this path? Can you point me to the Linux commit that changes this? The original PR talks about kernel 5.13 -- what am I missing here?

@satmandu
Copy link

This is what I see on my ubuntu system:

sudo /usr/sbin/mbpfan -t
Starting the tests..
It is normal for them to take a bit to finish.
Error: max_fan_speed value is not 6200
Tests run: 5
sudo /usr/sbin/mbpfan -v
mbpfan[9658]: mbpfan 2.2.0 starting up
mbpfan 2.2.0 starting up

And using the binary from this PR:

sudo ./mbpfan-tests -t
applesmc device path: /sys/devices/platform/applesmc.768
Starting the tests..
It is normal for them to take a bit to finish.
Using new sensor path for kernel >= 3.15.0 or some CentOS versions with kernel 3.10.0
Found hwmon path at /sys/devices/platform/coretemp.0/hwmon/hwmon3/temp
Found 5 sensors
Found 2 fans
Using new sensor path for kernel >= 3.15.0 or some CentOS versions with kernel 3.10.0
Found hwmon path at /sys/devices/platform/coretemp.0/hwmon/hwmon3/temp
Found 5 sensors
Couldn't open configfile, using defaults
Error: max_fan_speed value is not 2600
Tests run: 5
sudo ./mbpfan -v
applesmc device path: /sys/devices/platform/applesmc.768
mbpfan[10328]: mbpfan 2.2.0 starting up
mbpfan 2.2.0 starting up

@satmandu
Copy link

satadru@mbp113 ~$ cat /sys/devices/platform/applesmc.768/fan*_min
2160
2000
satadru@mbp113 ~$ cat /sys/devices/platform/applesmc.768/fan*_max
6156
5700

my /etc/mbpfan.conf has this:

# temperature units in celcius
low_temp = 50			# if temperature is below this, fans will run at minimum speed
high_temp = 55			# if temperature is above this, fan speed will gradually increase
max_temp = 60			# if temperature is above this, fans will run at maximum speed
polling_interval = 1	# default is 1 seconds

The fan does seem to be increasing with increased CPU temps though using the packaged mbpfan though...

@gaul
Copy link
Member

gaul commented Feb 26, 2022

Please do not use the tests to inform yourself about any successful operation of mbpfan. These were written long ago for a single device and do not work reliably for others. These were removed in 52d8973 almost 2 years ago to reduce user confusion. I am not clear how you are even running them on this PR as you suggest.

I am happy to merge PRs that address real issues. But I cannot merge what I don't understand. I don't understand the symptom you are trying to report here but it appears that mbpfan successfully found the applesmc path in both instances.

@agustiner
Copy link

@robabram I like this pull request, but just to check, is "5.13.12-200.mbp15.fc33.x86_64" a stock kernel and applesmc module or a modified one? If stock, the path "/sys/devices/platform/applesmc.768" no longer exists?

@Redecorating
Copy link

/sys/bus/acpi/drivers/applesmc/APP0001:00/ is the directory for the applesmc sysfs when using a kernel with patches added for supporting the MMIO based SMC on newer macs with the T2 chip. This isn't upstream, and probably won't get upstreamed as we plan to use macsmc when upstreaming smc support.

Currently we get people on T2 macs to use this fork https://github.com/networkException/mbpfan, but if you can support both paths then we can just tell people to use this repo instead of the fork, which makes it much simpler.

@gaul gaul merged commit 1941f45 into linux-on-mac:master Apr 2, 2023
@gaul
Copy link
Member

gaul commented Apr 2, 2023

Thank you for your contribution @robabram and for your explanation @Redecorating! 2.4.0 includes this commit.

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.

5 participants