Hi Ilpo,
On Sat, Jul 06, 2024 at 04:02:10PM +0300, Ilpo Järvinen wrote:
> > +
> > + err = unregister_acpi_notifier(&platform_power_source_nb);
> > +
> > + if (err < 0)
> > + pr_err("Failed to remove ACPI power source notify handler\n");
>
> Do we really need this? I don't think deinit paths in general log errors
> (or handle them either).
>
This is something that we discussed with Armin in an earlier revision of
the patch:
On Thu, Jun 20, 2024 at 10:12:21PM +0200, Armin Wolf wrote:
> > On Thu, Jun 27, 2024 at 07:55:26PM +0200, Armin Wolf wrote:
> >>> static void __exit hp_wmi_exit(void)
> >>> {
> >>> + if (is_omen_thermal_profile() || is_victus_thermal_profile())
> >>> + omen_unregister_powersource_event_handler();
> >> You have to check if the event handler was registered successfully before
> >> unregistering it.
> >>
> > Out of curiosity, I did a grep on the kernel drivers source code for
> > register_acpi_notifier/unregister_acpi_notifier and it seems that the
> > common practice is to not check for the return value at all (check out
> > drivers/gpu/drm/radeon/radeon_acpi.c:785 for example).
> >
> > Should I still check for the return value? I also believe there's no
> > proper method to check if a handler is registered or not, so I would
> > believe that I need to keep track of it myself; but since most kernel
> > drivers do not even care about the return value, I am not sure about
> > this.
>
> This seems to me like a very fragile construct, but i believe that error
> checking should be done here regardless.
>
> Maybe you should abort loading of the module when registration fails, so
> hp_wmi_exit() is only called when the notifier was registered successfully.
>
It made sense for me to abort during module loading, so that's what I ended up
doing.
I did a little bit of investigation on unregister_acpi_notifier which
relies on a blocking notifier chain internally. This brings me to
notifier_chain_unregister:
47:static int notifier_chain_unregister(struct notifier_block **nl,
48- struct notifier_block *n)
49-{
50- while ((*nl) != NULL) {
51- if ((*nl) == n) {
52- rcu_assign_pointer(*nl, n->next);
53- trace_notifier_unregister((void *)n->notifier_call);
54- return 0;
55- }
56- nl = &((*nl)->next);
57- }
58- return -ENOENT;
59-}
So it seems that the only error that can be raised from this function is
when the notifier is not found in the chain. This cannot be the case
here, so I understand why most modules do not check for the return
value, at least when unregistering the ACPI notifier.
If Armin is okay with this, I'll remove the error handling and the error
message.
I've taken into account your other comments, and will send a V10 once
everything's good.
Thanks for your time! :]
Alexis
