| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VeKosontsmFJSp-fbV9mPiSWJeLUCpx90=RHy1HFfBDnA@mail.gmail.com>
Date: Wed, 24 Nov 2021 18:21:30 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Denis Pauk <pauk.denis@...il.com>
Cc: Eugene Shalygin <eugene.shalygin@...il.com>,
Platform Driver <platform-driver-x86@...r.kernel.org>,
thomas@...ssschuh.net, Olli Asikainen <olli.asikainen@...il.com>,
Guenter Roeck <linux@...ck-us.net>,
Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] hwmon: (nct6775) add MAXIMUS VII HERO.
On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <pauk.denis@...il.com> wrote:
>
> ASUS MAXIMUS VII HERO board has got an nct6775 chip, but by default
> there's no use of it because of resource conflict with WMI method.
>
> This commit adds MAXIMUS VII HERO to the list of boards and provides
> ACPI mutex name that can be used as shared lock with ASUS WMI.
>
> Logic checks that mutex is available. If mutex is not available
> tries to get chip version by ACPI WMI interface.
a chip
...
> +struct acpi_board_info {
> + char *acpi_mutex_name;
Looking below the name of the "name" should be rather "path".
> };
...
> +static const struct dmi_system_id asus_wmi_info_table[] = {
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("MAXIMUS VII HERO", &acpi_board_MAXIMUS_VII_HERO),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B360-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B460-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B550M-A (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME X570-PRO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("Pro WS X570-ACE", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ProArt X570-CREATOR WIFI", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII DARK HERO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII FORMULA", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII HERO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII IMPACT", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-E GAMING",
> + &acpi_board_ROG_STRIX_B550_E_GAMING),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-I GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX X570-F GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z390-E GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z490-I GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PRO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PRO (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS (WI-FI)", &acpi_board_ANY),
So, is it possible to eliminate acpi_board_ANY and use some default in
the code instead?
> + {}
> +};
....
> - if (board_name && board_vendor &&
> - !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
> - err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
> - board_name);
Do you need string_helpers.h after this change?
> - if (err >= 0) {
> - /* if reading chip id via WMI succeeds, use WMI */
> - if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
> - pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> - access = access_asuswmi;
> + if (board_info->acpi_mutex_name) {
> + status = acpi_get_handle(NULL, board_info->acpi_mutex_name,
> + &acpi_wmi_mutex);
One line?
> + if (ACPI_FAILURE(status)) {
> + pr_err("Could not get hardware access guard mutex.\n");
> } else {
> - pr_err("Can't read ChipID by Asus WMI.\n");
> + pr_info("Using Asus WMI mutex: %s\n", board_info->acpi_mutex_name);
> + access = access_direct;
> }
> }
...
> + /* if reading chip id via WMI succeeds, use WMI */
Be consistent with how you spell "ChipID" / "chip id" / etc everywhere
in the code.
...
> + if (access == access_asuswmi &&
> + nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
> + access = access_direct;
> + pr_err("Can't read ChipID by Asus WMI.\n");
> + }
> +
> + if (access == access_asuswmi) {
> + if (tmp)
> + pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> + else
> + access = access_direct;
Why not:
if (access == access_asuswmi) {
access = access_direct;
if (nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp))
pr_err("Can't read ChipID by Asus WMI.\n");
if (tmp) {
pr_info("Using Asus WMI to access %#x chip.\n", tmp);
access = access_...; // do you have this?
}
...
}
?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists