linux-kernel - Re: [PATCH v4 1/1] platform/x86: asus-wmi: add support for vivobook fan profiles

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c3be1b7f-8a4c-42d8-9a5d-6a341c5beb33@redhat.com>
Date: Thu, 1 Aug 2024 16:01:12 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Luke Jones <luke@...nes.dev>, Mohamed Ghanmi <mohamed.ghanmi@...com.tn>
Cc: corentin.chary@...il.com, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, platform-driver-x86@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/1] platform/x86: asus-wmi: add support for vivobook
 fan profiles

Hi,

On 7/31/24 11:06 PM, Luke Jones wrote:

...

>>>>> If Hans and Ilpo don't have any requirements then:
>>>>>
>>>>> Signed-off-by: Luke D. Jones <luke@...nes.dev>
> 
> I have no issues with this patch and it needs to be merged before I submit a stream of work. My signed-off tag is above.

Reading back the comments about the function names I believe that
the asus_wmi_platform_profile_mode_[to|from]_vivo() names are fine.

And the rest of the patch looks good to me to:

Reviewed-by: Hans de Goede <hdegoede@...hat.com>

I have merged this into my review-hans branch now with one small change,
both asus_wmi_platform_profile_mode_[to|from]_vivo() ended in:

       return mode;

}

I have dropped the empty line after the return mode; statement
(in both functions) while merging this.

I've also dropped a spurious whitespace change (extra empty line)
added to asus_wmi_platform_profile_get().

Luke, please base your coming work on top of pdx86/review-hans.

Regards,

Hans

p.s.

One thing which I noticed while reviewing is this:

static int fan_curve_get_factory_default(struct asus_wmi *asus, u32 fan_dev)
{
        struct fan_curve_data *curves;
        u8 buf[FAN_CURVE_BUF_LEN];
        int err, fan_idx;
        u8 mode = 0;

        if (asus->throttle_thermal_policy_dev)
                mode = asus->throttle_thermal_policy_mode;
        /* DEVID_<C/G>PU_FAN_CURVE is switched for OVERBOOST vs SILENT */
        if (mode == 2)
                mode = 1;
        else if (mode == 1)
                mode = 2;

        err = asus_wmi_evaluate_method_buf(asus->dsts_id, fan_dev, mode, buf,
                                           FAN_CURVE_BUF_LEN);

Since asus->throttle_thermal_policy_mode stores the raw mode, which has
silent/overboost swapped for vivo notebooks I wonder if we should still
do the mode 1/2 swapping here when on a vivo notebook ?

I have a feeling the swapping here should not be done when one a vivo
notebook but I'm not sure.

If the swapping here should be skipped on Vivobooks please submit
a separate follow-up patch for that.

Regards,

Hans