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 :)
