linux-kernel - Re: [PATCH v3 1/1] asus-wmi: Add support for platform

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: <CAHp75VcCzjb7TKZ84iVjJr27+nCcA10n38nwCAGATucfAAMkKA@mail.gmail.com>
Date:   Sat, 14 Aug 2021 12:40:39 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     "Luke D. Jones" <luke@...nes.dev>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Bastien Nocera <hadess@...ess.net>,
        Platform Driver <platform-driver-x86@...r.kernel.org>
Subject: Re: [PATCH v3 1/1] asus-wmi: Add support for platform_profile

On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones <luke@...nes.dev> wrote:
>
> Add initial support for platform_profile where the support is
> based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>
> Because throttle_thermal_policy is used by platform_profile and is
> writeable separately to platform_profile any userspace changes to
> throttle_thermal_policy need to notify platform_profile.
>
> In future throttle_thermal_policy sysfs should be removed so that
> only one method controls the laptop power profile.

Some comments below.

...

> +       /*
> +        * Ensure that platform_profile updates userspace with the change to ensure
> +        * that platform_profile and throttle_thermal_policy_mode are in sync

Missed period here and in other multi-line comments.

> +        */

...

> +       /* All possible toggles like throttle_thermal_policy here */
> +       if (asus->throttle_thermal_policy_available) {
> +               tp = asus->throttle_thermal_policy_mode;
> +       } else {
> +               return -1;
> +       }
> +
> +       if (tp < 0)
> +               return tp;

This will be better in a form

    if (!..._available)
        return -ENODATA; // what -1 means?

    tp = ...;
    if (tp < 0)
        return tp;

...

> +       /* All possible toggles like throttle_thermal_policy here */
> +       if (!asus->throttle_thermal_policy_available) {
> +               return -1;

See above.

> +       }

...

> +       if (asus->throttle_thermal_policy_available) {
> +               pr_info("Using throttle_thermal_policy for platform_profile support\n");

Why pr_*()?

> +       } else {
> +               /*
> +                * Not an error if a component platform_profile relies on is unavailable
> +                * so early return, skipping the setup of platform_profile.
> +               */
> +               return 0;

Do it other way around,

/*
 * Comment
 */
if (!...)
  return 0;

> +       }


-- 
With Best Regards,
Andy Shevchenko