Messages in this thread | | | Subject | Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error | From | Armin Wolf <> | Date | Tue, 27 Sep 2022 17:44:18 +0200 |
| |
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 >>>>>>
| |