linux-kernel - Re: [PATCH 1/3] hwmon: (asus-ec-sensors) add driver for ASUS EC

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]

Message-ID: <CAB95QARFT8V-kMTtdRJHPAhXFk2BrF=5jxY2+CT0DvMrn6vKOg@mail.gmail.com>
Date:   Fri, 17 Dec 2021 01:07:41 +0100
From:   Eugene Shalygin <eugene.shalygin@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Denis Pauk <pauk.denis@...il.com>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-hwmon@...r.kernel.org
Subject: Re: [PATCH 1/3] hwmon: (asus-ec-sensors) add driver for ASUS EC

On Thu, 16 Dec 2021 at 22:28, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> > to the EC, in the same way as the WMI DSDT code does.
>
> How do you know that this way there is no race with any of ACPI code?

Because this mutex is exactly what the ACPI code uses to avoid races.

> _LOCK_DELAY_MS and drop useless comment
>
> I think I gave the very same comments before. Maybe you can check the
> reviews of another driver?

I understand your frustration, sorry. In all those similar reviews I
must have missed some emails. I'll fix what I can.

> > +static const struct ec_sensor_info known_ec_sensors[] = {
> > +       EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a), /* SENSOR_TEMP_CHIPSET */
> > +       EC_SENSOR("CPU", hwmon_temp, 1, 0x00, 0x3b), /* SENSOR_TEMP_CPU */
> > +       EC_SENSOR("Motherboard", hwmon_temp, 1, 0x00, 0x3c), /* SENSOR_TEMP_MB */
> > +       EC_SENSOR("T_Sensor", hwmon_temp, 1, 0x00, 0x3d), /* SENSOR_TEMP_T_SENSOR */
> > +       EC_SENSOR("VRM", hwmon_temp, 1, 0x00, 0x3e), /* SENSOR_TEMP_VRM */
> > +       EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xb0), /* SENSOR_FAN_CPU_OPT */
> > +       EC_SENSOR("VRM HS", hwmon_fan, 2, 0x00, 0xb2), /* SENSOR_FAN_VRM_HS */
> > +       EC_SENSOR("Chipset", hwmon_fan, 2, 0x00, 0xb4), /* SENSOR_FAN_CHIPSET */
> > +       EC_SENSOR("Water_Flow", hwmon_fan, 2, 0x00, 0xbc), /* SENSOR_FAN_WATER_FLOW */
> > +       EC_SENSOR("CPU", hwmon_curr, 1, 0x00, 0xf4), /* SENSOR_CURR_CPU */
> > +       EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00), /* SENSOR_TEMP_WATER_IN */
> > +       EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x01), /* SENSOR_TEMP_WATER_OUT */
>
> Instead of comments, use form of
>
>   [FOO] = BAR(...),

Was unable do that because the SENSOR_ enum is a flag enum. But given
this suggestion and the one about bit foreach loop, I will convert the
enum to bitmap.

> What's wrong with post-decrement, and I think I already commented on this.
> So, I stopped here until you go and enforce all comments given against
> previous incarnation of this driver.

I missed these ones, sorry.

> > +       for (i = 1; i < SENSOR_END; i <<= 1) {
> > +               if ((i & ec->board->sensors) == 0
> > +                       continue;
>
> Interesting way of NIH for_each_set_bit().

Will convert to bitmap.

> > +       acpi_status status = acpi_get_handle(
> > +               NULL, (acpi_string)state->board->acpi_mutex_path, &res);
>
> It looks awful (indentation), Have you run checkpatch?

Yes, but some warnings remained.

Thanks,
Eugene