linux-kernel - Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board

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:   Tue, 29 Mar 2022 21:22:20 +0200
From:   Eugene Shalygin <eugene.shalygin@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     darcagn@...tonmail.com, Jean Delvare <jdelvare@...e.com>,
        linux-hwmon@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info
 struct for board data

On Tue, 29 Mar 2022 at 15:44, Guenter Roeck <linux@...ck-us.net> wrote:
> >
> >   struct ec_sensors_data {
> > -     unsigned long board_sensors;
> > +     struct ec_board_info board_info;
>
> Please explain why this needs to be the entire structure and not
> just a pointer to it.

I marked the board_info array as __initconst assuming that this large
array will be unloaded from memory after the init phase, while we keep
only a single element. Is that assumption incorrect?

> > +static int sensor_count(const struct ec_board_info *board)
> > +{
> > +     return hweight_long(board->sensors);
> > +}
>
> This function is called several times. Does it really make sense, or is it
> necessary, to re-calculate the number of sensors over and over again
> instead of keeping it in ec->nr_sensors as before ? What are the benefits ?
> Unless there is a good explanation I see that as unrelated and unnecessary
> change.

This had something to do with data deduplication. However, I need the
count value only for looping over the sensor array, thus I can as well
add an invalid element to the end of the array. I rushed to submit
this driver to replace the wmi one, and it still has an artifact for
the WMI code I'd like to get rid of eventually, which is the read
buffer and the registers array. This will remove all the nr_ variables
and two dynamically allocated arrays. I will understand, of course, if
you ask to submit that refactoring separately.

> > -MODULE_DEVICE_TABLE(dmi, asus_ec_dmi_table);
> >   module_platform_driver_probe(asus_ec_sensors_platform_driver, asus_ec_probe);
> > +MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
>
> Why is MODULE_DEVICE_TABLE moved ?
Accidentally, probably. Thank you, will be corrected.

Thanks,
Eugene