Messages in this thread | | | Subject | Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error | From | Armin Wolf <> | Date | Mon, 19 Sep 2022 21:12:00 +0200 |
| |
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
>>> --- >>> 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 >>>
| |