linux-kernel - Re: [PATCH v6 3/3] hwmon: (nct6775) Support access via Asus WMI.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Sep 2021 11:34:50 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Denis Pauk <pauk.denis@...il.com>
Cc:     Bernhard Seibold <mail@...nhard-seibold.de>,
        Pär Ekholm <pehlm@...holm.org>,
        to.eivind@...il.com, "Artem S . Tashkinov" <aros@....com>,
        Vittorio Roberto Alfieri <me@...toor.com>,
        Sahan Fernando <sahan.h.fernando@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 3/3] hwmon: (nct6775) Support access via Asus WMI.

On Wed, Sep 15, 2021 at 11:45 PM Denis Pauk <pauk.denis@...il.com> wrote:
>
> Support accessing the NCT677x via Asus WMI functions.
>
> On mainboards that support this way of accessing the chip, the driver will
> usually not work without this option since in these mainboards, ACPI will
> mark the I/O port as used.
>
> Code uses ACPI firmware interface to commucate with sensors with ASUS

communicate

> motherboards:
> * PRIME B460-PLUS,
> * ROG CROSSHAIR VIII IMPACT,
> * ROG STRIX B550-E GAMING,
> * ROG STRIX B550-F GAMING,
> * ROG STRIX B550-F GAMING (WI-FI),
> * ROG STRIX Z490-I GAMING,
> * TUF GAMING B550M-PLUS,
> * TUF GAMING B550M-PLUS (WI-FI),
> * TUF GAMING B550-PLUS,
> * TUF GAMING X570-PLUS,
> * TUF GAMING X570-PRO (WI-FI).

...

> +static int asuswmi_evaluate_method(u32 method_id, u8 bank, u8 reg, u8 val, u32 *retval)
> +{

> +#if IS_ENABLED(CONFIG_ACPI_WMI)

The idea behind IS_ENABLED() macro is that it may be used in C
conditionals, like

if (IS_ENABLED(...))

> +       u32 args = bank | (reg << 8) | (val << 16);
> +       struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> +       struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +       acpi_status status;
> +       union acpi_object *obj;
> +       u32 tmp = 0;

> +       obj = output.pointer;
> +       if (obj && obj->type == ACPI_TYPE_INTEGER)
> +               tmp = obj->integer.value;
> +
> +       if (retval)
> +               *retval = tmp;
> +
> +       kfree(obj);

> +       if (tmp == ASUSWMI_UNSUPPORTED_METHOD)

This is uninitialized tmp in case when no obj, or obj is of the wrong type.

> +               return -ENODEV;
> +       return 0;
> +#else
> +       return -EOPNOTSUPP;
> +#endif
> +}

...

> +static u16 nct6775_wmi_read_value(struct nct6775_data *data, u16 reg)
> +{
> +       int res, word_sized = is_word_sized(data, reg);
> +       u8 tmp;
> +
> +       nct6775_wmi_set_bank(data, reg);
> +
> +       nct6775_asuswmi_read(data->bank, reg, &tmp);

> +       res = (tmp & 0xff);

Too many parentheses.
tnp is u8, ' & 0xff' is redundant.

> +       if (word_sized) {
> +               nct6775_asuswmi_read(data->bank, (reg & 0xff) + 1, &tmp);
> +               res = (res << 8) + (tmp & 0xff);

Ditto.

> +       }
> +       return res;
> +}
> +
> +static int nct6775_wmi_write_value(struct nct6775_data *data, u16 reg, u16 value)
> +{
> +       int word_sized = is_word_sized(data, reg);
> +
> +       nct6775_wmi_set_bank(data, reg);
> +
> +       if (word_sized) {

> +               nct6775_asuswmi_write(data->bank, (reg & 0xff),

Too many parentheses

> +                                     (value >> 8) & 0xff);

' & 0xff' part is redundant.

> +               nct6775_asuswmi_write(data->bank, (reg & 0xff) + 1,
> +                                     value & 0xff);
> +       } else {
> +               nct6775_asuswmi_write(data->bank, (reg & 0xff), value);
> +       }
> +
> +       return 0;
> +}

...

> +               err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
> +                                  board_name);

> +               if (err != -EINVAL) {

I believe I commented on this in the way as

if (err >= 0)

The rationale behind is that you shouldn't really care what kind of
error codes the API may return.

> +                       /* 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;
> +                       } else {
> +                               pr_err("Can't read ChipID by Asus WMI.\n");
> +                       }
> +               }

-- 
With Best Regards,
Andy Shevchenko