Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single
 error
To:     Hans de Goede <hdegoede@redhat.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Mark Gross <markgross@kernel.org>, Len Brown <lenb@kernel.org>,
        Henrique de Moraes Holschuh <hmh@hmh.eng.br>,
        Matan Ziv-Av <matan@svgalib.org>,
        Corentin Chary <corentin.chary@gmail.com>,
        Jeremy Soller <jeremy@system76.com>, productdev@system76.com,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20220912125342.7395-1-W_Armin@gmx.de>
 <20220912125342.7395-2-W_Armin@gmx.de>
 <f8fa6d10-6eb1-7fa7-80eb-ea190d29ba4a@redhat.com>
 <CAJZ5v0jWVMMTjc+KtBRS86f8kYpbPcDCH9JV2ZgeN4f-MSO8rQ@mail.gmail.com>
 <f2af5d01-a2cd-ae96-24c7-d61f5f0d0bc3@gmx.de>
 <f0b17ba6-3d3c-cbc1-ec0d-ec59c73f06f6@gmx.de>
 <471449b3-cced-d75d-e349-6bec950b0bc1@redhat.com>
From:   Armin Wolf <W_Armin@gmx.de>
Message-ID: <f6ac14a8-6403-029e-6179-f8ef0a7d5457@gmx.de>
Date:   Tue, 27 Sep 2022 17:44:18 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <471449b3-cced-d75d-e349-6bec950b0bc1@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Precedence: bulk

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 dr=
ivers
>>> to indicate a unsupported battery.
>>>
>>> Armin Wolf
>>>
>> While checking all instances where the battery hook mechanism is curren=
tly used,
>> i found out that all but a single battery hook return -ENODEV for all e=
rrors 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 o=
n for
>> example -ENOTSUP will result in similar behavior by hooks wanting to av=
oid 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 whic=
h 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 interfac=
es
> 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
>>
>>>>>> ---
>>>>>>  =C2=A0 drivers/acpi/battery.c | 24 +++---------------------
>>>>>>  =C2=A0 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_batter=
y_hook *hook)
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * its attributes.
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 list_for_each_entry(battery, =
&acpi_battery_list, list) {
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 if (hook->add_battery(battery->bat)) {
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /*
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * If a add-ba=
ttery returns non-zero,
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * the registr=
ation of the extension has failed,
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * and we will=
 not add it to the list of loaded
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * hooks.
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("extension f=
ailed to load: %s", hook->name);
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __battery_hook_unre=
gister(hook, 0);
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto end;
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 }
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 hook->add_battery(battery->bat);
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_info("new extension: %s\n"=
, hook->name);
>>>>>> -end:
>>>>>> +
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mutex_unlock(&hook_mutex);
>>>>>>  =C2=A0 }
>>>>>>  =C2=A0 EXPORT_SYMBOL_GPL(battery_hook_register);
>>>>>> @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct ac=
pi_battery *battery)
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * during the battery mo=
dule initialization.
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 list_for_each_entry_safe(hook=
_node, tmp, &battery_hook_list, list) {
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 if (hook_node->add_battery(battery->bat)) {
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /*
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * The notific=
ation of the extensions has failed, to
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * prevent fur=
ther errors we will unload the extension.
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("error in ex=
tension, unloading: %s",
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 hook_node->name);
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __battery_hook_unre=
gister(hook_node, 0);
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 }
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 hook_node->add_battery(battery->bat);
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>>>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mutex_unlock(&hook_mutex);
>>>>>>  =C2=A0 }
>>>>>> --
>>>>>> 2.30.2
>>>>>>