linux-kernel - Re: [PATCH v4 3/9] platform/x86: asus-armoury: move existing tunings to asus-armoury module

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: <b0bf1b66-5f6b-484f-aaaf-ccd10459e0e8@app.fastmail.com>
Date: Sat, 28 Sep 2024 21:52:16 +1200
From: "Luke Jones" <luke@...nes.dev>
To: "Mario Limonciello" <superm1@...nel.org>, linux-kernel@...r.kernel.org
Cc: linux-input@...r.kernel.org, "Benjamin Tissoires" <bentiss@...nel.org>,
 "Jiri Kosina" <jikos@...nel.org>, platform-driver-x86@...r.kernel.org,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 "Hans de Goede" <hdegoede@...hat.com>, corentin.chary@...il.com
Subject: Re: [PATCH v4 3/9] platform/x86: asus-armoury: move existing tunings to
 asus-armoury module

On Sat, 28 Sep 2024, at 2:45 AM, Mario Limonciello wrote:
> On 9/26/2024 18:20, Luke Jones wrote:
>
>>>> + asus_armoury.mini_led_dev_id = 0;
>>>> + if (asus_wmi_is_present(ASUS_WMI_DEVID_MINI_LED_MODE)) {
>>>> + asus_armoury.mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
>>>> + err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
>>>> + &mini_led_mode_attr_group);
>>>> + } else if (asus_wmi_is_present(ASUS_WMI_DEVID_MINI_LED_MODE2)) {
>>>> + asus_armoury.mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2;
>>>> + err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
>>>> + &mini_led_mode_attr_group);
>>>> + }
>>>> + if (err)
>>>> + pr_warn("Failed to create sysfs-group for mini_led\n");
>>>
>>> Shouldn't you fail and clean up here?
>> 
>> Honestly not sure. It's only a failed WMI call, and the group doesn't get created for that fail, the others might be fine. I'll defer to your advice on that.
>
> It comes down to whether failures are expected on some machines or not 
> in practice.
>
> If a machine will fail to create groups, then yeah you should allow 
> skipping.  If it doesn't then I feel you have a much more logical 
> cleanup and support strategy if you unwind on any failure.
>
> Hans or Ilpo might have some thoughts here too.

I see now that I wasn't thinking clearly about this. I was zooming in on the asus_wmi_is_present() and just somehow blinding myself to the fact that, *that* isn't the error source.

Thank you for catching, I will update the code with proper unwind and handling. 

>>> I think you should be using this mutex more.  For example what if an
>>> attribute is being written while the module is unloaded?
>> 
>> Good point. I'll adjust code to suit. However if I do, this will trickle through the other patches I've added your review tag to. Will this be okay?
>> 
>>>
>
> If they change in a material way drop my tag and I can just re-review.

Looks like they haven't :)