| Message ID | 20211128184549.9161-1-pauk.denis@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hwmon: (nct6775) Support lock by ACPI mutex | expand |
Hi, Could you please provide a some feedback about such idea? I have bigger list of supported boards that requires ACPI mutex lock, but I prefer to have some feedback before send next version of patch. I have created separate patch[1] with only boards where WMI methods is enough. And if work on patch takes some time/additional patch versions(for sure it will), I prefer to have that patch merged and rebase current patch over resulted list of boards. Thank you. 1. https://patchwork.kernel.org/project/linux-hwmon/list/?series=594167 On Sun, 28 Nov 2021 20:45:45 +0200 Denis Pauk <pauk.denis@gmail.com> wrote: > Lock by a ACPI mutex that is required for support ASUS MAXIMUS VII > HERO motherboard. In other case, all methods are returned zero > instead of real values. Code uses acpi mutex before any IO operations > in case when such acpi mutex is known. > > Patch series adds additional check for chip ID, and if method > returned zero, all calls by acpi_wmi are disabled. > > @Andy Shevchenko: > >> Do you need string_helpers.h after this change? > It was not required in the original patch, as it was included as part > of some other header and I have left includes without changes. > > I have a little bit changed conditionals in "add MAXIMUS VII HERO", > code can change access variable several times: > * By the default, access is set to direct, > * after code has checked that wmi methods can return something useful, > * and as the last step code has checked that mutext is existed and > set access back to direct. > > But as for me, it should be less confusing. > > What do you think? > > --- > > Changes in v2: > - Fix commit message. > - Remove default acpi_board_ANY and use NULL instead. > - Use chip ID everywhere. > - Use an anonymous union for mutexes. > - Use temporary status varibale in acpi calls. > --- > > Denis Pauk (3): > hwmon: (nct6775) Use lock function pointers in nct6775_data > hwmon: (nct6775) Implement custom lock by ACPI mutex > hwmon: (nct6775) add MAXIMUS VII HERO > > drivers/hwmon/nct6775.c | 364 > +++++++++++++++++++++++++++++----------- 1 file changed, 263 > insertions(+), 101 deletions(-) > > > base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf Best regards, Denis.
On 12/16/21 2:22 PM, Denis Pauk wrote: > Hi, > > Could you please provide a some feedback about such idea? > > I have bigger list of supported boards that requires ACPI mutex lock, > but I prefer to have some feedback before send next version of patch. > > I have created separate patch[1] with only boards where WMI methods is > enough. And if work on patch takes some time/additional patch > versions(for sure it will), I prefer to have that patch merged and > rebase current patch over resulted list of boards. > Looking through the code, I am absolutely not happy with it. It makes the driver even more unreadable than it already is, and on top of that makes it vulnerable to problems in the ACPI code. Example: If ACPI fails to unlock the mutex, the driver will end up being non-functional. At some point, we have to face it: ASUS doesn't support Linux, and they make it hard to access chips like this. I think the chip should be accessed through "official" channels only if provided (ie WMI/ACPI), or not at all. Guenter
On Fri, 17 Dec 2021 at 17:23, Guenter Roeck <linux@roeck-us.net> wrote: > At some point, we have to face it: ASUS doesn't support Linux, and they > make it hard to access chips like this. I think the chip should be > accessed through "official" channels only if provided (ie WMI/ACPI), > or not at all. My two cents, if you please. Unfortunately, ASUS doesn't support Windows as well, they only support their own shitty software, and they change the WMI methods (both names and logic). For example, just recently they packed a full hardware monitoring solution in X470 boards in WMI, then removed it in X570 and changed hardware access function names. In order to add support for their next WMI implementation, one needs to thoroughly read the decompiled DSDT code, find functions, learn their logic and test. This is hard to do remotely, without the hardware, obviously. On the other hand it is much easier to find the required mutex name from the DSDT code and access the chip normally. Best regards, Eugene
On Fri, 17 Dec 2021 18:14:19 +0100 Eugene Shalygin <eugene.shalygin@gmail.com> wrote: > On Fri, 17 Dec 2021 at 17:23, Guenter Roeck <linux@roeck-us.net> > wrote: > > > At some point, we have to face it: ASUS doesn't support Linux, and > > they make it hard to access chips like this. I think the chip > > should be accessed through "official" channels only if provided (ie > > WMI/ACPI), or not at all. > > My two cents, if you please. Unfortunately, ASUS doesn't support > Windows as well, they only support their own shitty software, and they > change the WMI methods (both names and logic). For example, just > recently they packed a full hardware monitoring solution in X470 > boards in WMI, then removed it in X570 and changed hardware access > function names. In order to add support for their next WMI > implementation, one needs to thoroughly read the decompiled DSDT code, > find functions, learn their logic and test. This is hard to do > remotely, without the hardware, obviously. On the other hand it is > much easier to find the required mutex name from the DSDT code and > access the chip normally. > > Best regards, > Eugene I will try to continue to support patch as part of https://bugzilla.kernel.org/show_bug.cgi?id=204807. And If we will have some better solution or ideas, I will send updated patch. Thank you! Best regards, Denis.
Lock by a ACPI mutex that is required for support ASUS MAXIMUS VII HERO motherboard. In other case, all methods are returned zero instead of real values. Code uses acpi mutex before any IO operations in case when such acpi mutex is known. Patch series adds additional check for chip ID, and if method returned zero, all calls by acpi_wmi are disabled. @Andy Shevchenko: >> Do you need string_helpers.h after this change? It was not required in the original patch, as it was included as part of some other header and I have left includes without changes. I have a little bit changed conditionals in "add MAXIMUS VII HERO", code can change access variable several times: * By the default, access is set to direct, * after code has checked that wmi methods can return something useful, * and as the last step code has checked that mutext is existed and set access back to direct. But as for me, it should be less confusing. What do you think? --- Changes in v2: - Fix commit message. - Remove default acpi_board_ANY and use NULL instead. - Use chip ID everywhere. - Use an anonymous union for mutexes. - Use temporary status varibale in acpi calls. --- Denis Pauk (3): hwmon: (nct6775) Use lock function pointers in nct6775_data hwmon: (nct6775) Implement custom lock by ACPI mutex hwmon: (nct6775) add MAXIMUS VII HERO drivers/hwmon/nct6775.c | 364 +++++++++++++++++++++++++++++----------- 1 file changed, 263 insertions(+), 101 deletions(-) base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf