Message-ID: <CAB95QAQ+u4DmF0e9Zvy5hDV0mFQDEULtr-newtz5_6y=Bzp+ww@mail.gmail.com>
Date:   Thu, 7 Oct 2021 20:11:33 +0200
From:   Eugene Shalygin <eugene.shalygin@...il.com>
To:     Denis Pauk <pauk.denis@...il.com>
Cc:     andy.shevchenko@...il.com, Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2 2/3] hwmon: (asus_wmi_ec_sensors) Support B550 Asus WMI.

Denis and All,

regarding the asus-wmi-ec-sensors driver: it uses a WMI method to read
EC registers, and this method is slow (requires almost a full second
for a single call). Maybe I'm doing something wrong, but my impression
is that the WMI calls themselves are that slow. I will try to
reimplement this driver using direct EC operations and the global ACPI
lock with a hope to make it read sensors quicker. If that works out,
perhaps the nct6775 may go the same way, as it suffers too from the
slow WMI calls. I know next to nothing about the ACPI system and learn
from the beginning, so I'm not sure about the result. I know the naive
reading from the ACPI EC registers leads to problems (fans get stuck,
etc.), and if someone with knowledge can assure me that the idea with
the ACPI global lock (as far as I understand it is even implemented in
the ec kernel driver already) is correct, I would even request to stop
accepting the EC WMI sensors driver, as it is so slow (albeit dead
simple and small).

Best regards,
Eugen

On Thu, 7 Oct 2021 at 19:55, Eugene Shalygin <eugene.shalygin@...il.com> wrote:
>
> Hi Denis,
>
> yes, the GH repo contains the fix and a few code cleanups, which I
> would like to propose for mainlining too. Also, please find below a
> draft of the documentation:
>
> Kernel driver asus-wmi-ec-sensors
> =================================
>
> Authors:
>         <eugene.shalygin@...il.com>
>
> Description:
> ------------
> ASUS mainboards publish hardware monitoring information via Super I/O
> chip and the ACPI embedded controller (EC) registers. Some of the sensors
> are only available via the EC.
>
> ASUS WMI interface provides a method (BREC) to read data from EC registers,
> which is utilized by this driver to publish those sensor readings to the
> HWMON system. The driver is aware of and reads the following sensors:
>
> 1. Chipset (PCH) temperature
> 2. CPU package temperature
> 3. Motherboard temperature
> 4. Readings from the T_Sensor header
> 5. VRM temperature
> 6. CPU_Opt fan RPM
> 7. Chipset fan RPM
> 8. Readings from the "Water flow meter" header (RPM)
> 9. Readings from the "Water In" and "Water Out" temperature headers
> 10. CPU current
>
> Best regards,
> Eugene
>
> On Thu, 7 Oct 2021 at 17:46, Denis Pauk <pauk.denis@...il.com> wrote:
> >
> > Hi Eugene,
> >
> > On Thu, 7 Oct 2021 01:32:14 +0200
> > Eugene Shalygin <eugene.shalygin@...il.com> wrote:
> >
> > > On Thu, 7 Oct 2021 at 00:25, Denis Pauk <pauk.denis@...il.com> wrote:
> > > >
> > >
> > > > Supported motherboards:
> > > > * ROG CROSSHAIR VIII HERO
> > > > * ROG CROSSHAIR VIII DARK HERO
> > > > * ROG CROSSHAIR VIII FORMULA
> > > > * ROG STRIX X570-E GAMING
> > > > * ROG STRIX B550-E GAMING
> > >
> > > Pro WS X570-ACE is missing from this list.
> > Thanks, I will check.
> > >
> > > > + * EC provided:
> > > provides
> > Thanks, I will check.
> > >
> > > > + * Chipset temperature,
> > > > + * CPU temperature,
> > > > + * Motherboard temperature,
> > > > + * T_Sensor temperature,
> > > > + * VRM  temperature,
> > > > + * Water In temperature,
> > > > + * Water Out temperature,
> > > > + * CPU Optional Fan,
> > > Hereinafter "CPU Optional Fan RPM"?
> > >
> > Thanks, I will check.
> > > > +static const enum known_ec_sensor
> > > > known_board_sensors[BOARD_MAX][SENSOR_MAX + 1] = {
> > > > +       [BOARD_PW_X570_A] = {
> > > > +               SENSOR_TEMP_CHIPSET, SENSOR_TEMP_CPU,
> > > > SENSOR_TEMP_MB, SENSOR_TEMP_VRM,
> > > > +               SENSOR_FAN_CHIPSET,
> > >
> > > I missed SENSOR_CURR_CPU for a few boards, and unfortunately the
> > > mistake made it here too. Sorry for that.
> > >
> > Do you have such fix in your repository?
> > > > +/**
> > > > + * struct asus_wmi_ec_info - sensor info.
> > > > + * @sensors: list of sensors.
> > > > + * @read_arg: UTF-16 string to pass to BRxx() WMI function.
> > > > + * @read_buffer: WMI function output.
> > >
> > > This seems to be a bit misleading to me in a sense that the variable
> > > holds decoded output (array of numbers as opposed to array of
> > > characters in the WMI output buffer.
> > >
> > > > +struct asus_wmi_data {
> > > > +       int ec_board;
> > > > +};
> > >
> > > A leftover?
> > >
> > Its platform data and used to share board_id with probe.
> >
> > > > +static void asus_wmi_ec_decode_reply_buffer(const u8 *inp, u8 *out)
> > > > +{
> > > > +       unsigned int len = ACPI_MIN(ASUS_WMI_MAX_BUF_LEN, inp[0] /
> > > > 4);
> > > > +       char buffer[ASUS_WMI_MAX_BUF_LEN * 2];
> > > > +       const char *pos = buffer;
> > > > +       const u8 *data = inp + 2;
> > > > +       unsigned int i;
> > > > +
> > > > +       utf16s_to_utf8s((wchar_t *)data, len * 2,
> > > > UTF16_LITTLE_ENDIAN, buffer, len * 2);
> > > Errr... Why is it here? You need the same loop afterwards, just with a
> > > smaller stride.
> > I have tried to apply Andy's idea. And it looks it does not
> > provide benefits. Andy, what do you think? Maybe I understand it in
> > wrong way.
> > > > +
> > > > +       for (i = 0; i < len; i++, pos += 2)
> > > > +               out[i] = (hex_to_bin(pos[0]) << 4) +
> > > > hex_to_bin(pos[1]); +}
> > > > +
> > > > +static void asus_wmi_ec_encode_registers(u16 *registers, u8 len,
> > > > char *out) +{
> > > > +       char buffer[ASUS_WMI_MAX_BUF_LEN * 2];
> > > > +       char *pos = buffer;
> > > > +       unsigned int i;
> > > > +       u8 byte;
> > > > +
> > > > +       *out++ = len * 8;
> > > > +       *out++ = 0;
> > > > +
> > > > +       for (i = 0; i < len; i++) {
> > > > +               byte = registers[i] >> 8;
> > > > +               *pos = hex_asc_hi(byte);
> > > > +               pos++;
> > > > +               *pos = hex_asc_lo(byte);
> > > > +               pos++;
> > > > +               byte = registers[i];
> > > > +               *pos = hex_asc_hi(byte);
> > > > +               pos++;
> > > > +               *pos = hex_asc_lo(byte);
> > > > +               pos++;
> > > > +       }
> > > > +
> > > > +       utf8s_to_utf16s(buffer, len * 4, UTF16_LITTLE_ENDIAN,
> > > > (wchar_t *)out, len * 4);
> > > Same here. Just for the sake of calling utf8s_to_utf16s() you need the
> > > same loop plus an additional buffer. I don't get it.
> > >
> > I have tried to apply Andy's idea. And it looks it does not
> > provide benefits. Andy, what do you think? Maybe I understand it in
> > wrong way.
> > > > +}
> > > > +
> > > > +static void asus_wmi_ec_make_block_read_query(struct
> > > > asus_wmi_ec_info *ec) +{
> > > > +       u16 registers[ASUS_WMI_BLOCK_READ_REGISTERS_MAX];
> > > > +       const struct ec_sensor_info *si;
> > > > +       long i, j, register_idx = 0;
> > > long? maybe a simple unsigned or int?
> > >
> > Looks as it was in original patch, I will look.
> > > > +
> > > > +static int asus_wmi_ec_update_ec_sensors(struct asus_wmi_ec_info
> > > > *ec) +{
> > > > +       const struct ec_sensor_info *si;
> > > > +       struct ec_sensor *s;
> > > > +
> > > > +       u32 value;
> > > This variable is now redundant.
> > >
> > Thank you, I will look.
> >
> > > > +               if (si->addr.size == 1)
> > > Maybe switch(si->addr.size)?
> > >
> > Thank you, I will check.
> > > > +                       value = ec->read_buffer[read_reg_ct];
> > > > +               else if (si->addr.size == 2)
> > > > +                       value =
> > > > get_unaligned_le16(&ec->read_buffer[read_reg_ct]);
> > > > +               else if (si->addr.size == 4)
> > > > +                       value =
> > > > get_unaligned_le32(&ec->read_buffer[read_reg_ct]); +
> > > > +               read_reg_ct += si->addr.size;
> > > > +               s->cached_value = value;
> > > > +       }
> > > > +       return 0;
> > > > +}
> > >
> > >
> > > > +       mutex_lock(&sensor_data->lock);
> > > The mutex locking/unlocking should be moved inside the
> > > update_ec_sensors(), I guess.
> > >
> > > I re-read your answer to my question as to why don't you add module
> > > aliases to the driver, and I have to admit I don't really understand
> > > it. Could you, please, elaborate?
> > >
> > It looked complicated to support two kind of WMI interfaces with UUID.
> > As we split big support module to two separate - I will look to such
> > change also.
> >
> > > Thank you,
> > > Eugene
> >
> > Best regards,
> >      Denis.

Powered by blists - more mailing lists