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

[Mario Limonciello <superm1@kernel.org>]

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.