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: <CAB95QASgzUSaLjZuxWauiSvNK7Or=1Kz8htJcjWqU+HrRutvJg@mail.gmail.com>
Date:   Wed, 30 Mar 2022 00:11:15 +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 2/4] hwmon: (asus-ec-sensors) implement locking via the
 ACPI global lock

On Tue, 29 Mar 2022 at 23:23, Guenter Roeck <linux@...ck-us.net> wrote:

> > +/* Moniker for the ACPI global lock (':' is not allowed in ASL identifiers) */
> > +#define ACPI_GLOBAL_LOCK_PSEUDO_PATH ":GLOBAL_LOCK"
> > +
>
> That needs to be documented.

Do you mean a note in the /Documentation/..../...rst or adding details
here? There is an additional bit of information on this identifier
below, in the ec_board_info struct declaration.

> There is some type confusion in the above lock functions. Some return
> ACPI error codes, some return Linux error codes. Please make return
> values consistent.
>
> Also, why use mutex_trylock() instead of mutex_lock() ? This is
> unusual since it will result in errors if more than one user
> tries to access the data (eg multiple processes reading sysfs
> attributes at the same time), and thus warrants a detailed
> explanation.
OK.

> > +     struct lock_data lock_data;
> > +     /* number of board EC sensors */
> > +     u8 nr_sensors;
>
> Ok, I must admit I am more than a bit lost. In patch 1/4
> you removed this variable (and argued that removing it was
> for "deduplication"), only to re-introduce it here.
> Sorry, I don't follow the logic.

Sorry for that. This is my mistake which I tried to warn you about in
my first reply to the email with this patch.

> > +     if (!mutex_path || !strlen(mutex_path)) {
>
> When would mutex_path be NULL ?
When it is set to NULL in the board definition struct ec_board_info.

> > +             if (ACPI_FAILURE(status)) {
> > +                     dev_err(dev,
> > +                             "Failed to get hardware access guard AML mutex"
> > +                             "'%s': error %d",
>
> Please no string splits. And the negative impact can be seen here:
> No space between "mutex" and "'%s'".

Yes, of course.

> >               dev_warn(dev,
> > -                     "Concurrent access to the ACPI EC detected.\nRace condition possible.");
> > +                     "Concurrent access to the ACPI EC detected.\n"
> > +                     "Race condition possible.");
>
> Why this change, and how is it related to this patch ?
Same as above, will be corrected.

Thank you,
Eugene

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ