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: <919a3891-4024-4b67-81ae-93f3c87ee37b@kernel.org>
Date: Fri, 27 Sep 2024 09:45:35 -0500
From: Mario Limonciello <superm1@...nel.org>
To: Luke Jones <luke@...nes.dev>, 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 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 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.