lkml.org 
[lkml]   [2022]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error
From
Date
Am 27.09.22 um 16:29 schrieb Hans de Goede:

> Hi,
>
> On 9/19/22 22:35, Armin Wolf wrote:
>> Am 19.09.22 um 21:12 schrieb Armin Wolf:
>>
>>> Am 19.09.22 um 18:27 schrieb Rafael J. Wysocki:
>>>
>>>> On Mon, Sep 19, 2022 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 9/12/22 13:53, Armin Wolf wrote:
>>>>>> Currently, battery hooks are being unloaded if they return
>>>>>> an error when adding a single battery.
>>>>>> This however also causes the removal of successfully added
>>>>>> hooks if they return -ENODEV for a single unsupported
>>>>>> battery.
>>>>>>
>>>>>> Do not unload battery hooks in such cases since the hook
>>>>>> handles any cleanup actions.
>>>>>>
>>>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>>> Maybe instead of removing all error checking, allow -ENODEV
>>>>> and behave as before when the error is not -ENODEV ?
>>>>>
>>>>> Otherwise we should probably make the add / remove callbacks
>>>>> void to indicate that any errors are ignored.
>>>>>
>>>>> Rafael, do you have any opinion on this?
>>>> IMV this is not a completely safe change, because things may simply
>>>> not work in the cases in which an error is returned.
>>>>
>>>> It would be somewhat better to use a special error code to indicate
>>>> "no support" (eg. -ENOTSUPP) and ignore that one only.
>>> I would favor -ENODEV then, since it is already used by quiet a few drivers
>>> to indicate a unsupported battery.
>>>
>>> Armin Wolf
>>>
>> While checking all instances where the battery hook mechanism is currently used,
>> i found out that all but a single battery hook return -ENODEV for all errors they
>> encounter, the exception being the huawei-wmi driver.
> Right, so this means that using -ENODEV to not automatically unload the
> extension on error will result in a behavior change for those drivers,
> with possibly unwanted side-effects.
>
> As such I believe that using -ENOTSUP for the case where the extension
> does not work for 1 battery but should still be used for the other
> batter{y|ies} would be better as this preserves the existing behavior
> for existing drivers.
>
>> I do not know the reason for this, but i fear unloading the extension on for
>> example -ENOTSUP will result in similar behavior by hooks wanting to avoid being
>> unloaded on harmless errors.
> I am not sure what you are trying to say here. The whole idea is
> to add new behavior for -ENOTSUP to allow drivers to opt out of
> getting their extension unregistered when they return this.
>
> Although I wonder why not just have extensions return 0 when
> they don't want to register any sysfs attr and that not considered
> an error. If it is not considered an error the hook can just
> return 0, which would not require any ACPI battery code changes
> at all. So maybe just returning 0 is the easiest (which is
> also often the best) answer here?

I agree, i will send v2 soon.

Armin Wolf

>> However, i agree that when ignoring all errors, battery extensions which provide
>> similar attributes may currently delete each others attributes.
> AFAIK there are no cases where more then 1 extension driver gets loaded,
> since all the extension drivers are tied to a specific vendor's interfaces
> so we won't e.g. see the thinkpad_acpi driver load on the same laptop as
> where toshiba_acpi also loads.
>
> IOW I think you are trying to solve a problem which does not exist here.
>
> Regards,
>
> Hans
>
>
>
>
>> Any idea on how to solve this?
>>
>> Armin Wolf
>>
>>>>>> ---
>>>>>>   drivers/acpi/battery.c | 24 +++---------------------
>>>>>>   1 file changed, 3 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>>>>>> index 306513fec1e1..e59c261c7c59 100644
>>>>>> --- a/drivers/acpi/battery.c
>>>>>> +++ b/drivers/acpi/battery.c
>>>>>> @@ -724,20 +724,10 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>>>>>>         * its attributes.
>>>>>>         */
>>>>>>        list_for_each_entry(battery, &acpi_battery_list, list) {
>>>>>> -             if (hook->add_battery(battery->bat)) {
>>>>>> -                     /*
>>>>>> -                      * If a add-battery returns non-zero,
>>>>>> -                      * the registration of the extension has failed,
>>>>>> -                      * and we will not add it to the list of loaded
>>>>>> -                      * hooks.
>>>>>> -                      */
>>>>>> -                     pr_err("extension failed to load: %s", hook->name);
>>>>>> -                     __battery_hook_unregister(hook, 0);
>>>>>> -                     goto end;
>>>>>> -             }
>>>>>> +             hook->add_battery(battery->bat);
>>>>>>        }
>>>>>>        pr_info("new extension: %s\n", hook->name);
>>>>>> -end:
>>>>>> +
>>>>>>        mutex_unlock(&hook_mutex);
>>>>>>   }
>>>>>>   EXPORT_SYMBOL_GPL(battery_hook_register);
>>>>>> @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>>>>>>         * during the battery module initialization.
>>>>>>         */
>>>>>>        list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
>>>>>> -             if (hook_node->add_battery(battery->bat)) {
>>>>>> -                     /*
>>>>>> -                      * The notification of the extensions has failed, to
>>>>>> -                      * prevent further errors we will unload the extension.
>>>>>> -                      */
>>>>>> -                     pr_err("error in extension, unloading: %s",
>>>>>> -                                     hook_node->name);
>>>>>> -                     __battery_hook_unregister(hook_node, 0);
>>>>>> -             }
>>>>>> +             hook_node->add_battery(battery->bat);
>>>>>>        }
>>>>>>        mutex_unlock(&hook_mutex);
>>>>>>   }
>>>>>> --
>>>>>> 2.30.2
>>>>>>

\
 
 \ /
  Last update: 2022-09-27 17:47    [W:0.074 / U:0.764 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site