Am 03.01.25 um 19:52 schrieb Kurt Borja:
On Fri, Jan 03, 2025 at 07:19:51PM +0100, Joshua Grisham wrote:
Hi Kurt, thanks for the comments! Will respond inline below...
Den mån 30 dec. 2024 kl 18:50 skrev Kurt Borja <kuurtb@xxxxxxxxx>:
+ if (err)
+ goto return_with_dbg;
+
+ galaxybook->has_kbd_backlight = true;
+
+ return 0;
+
+return_with_dbg:
+ dev_dbg(&galaxybook->platform->dev,
+ "failed to initialize kbd_backlight, error %d\n", err);
+ return 0;
Return `err` here.
I actually intentionally want to return 0 here -- the feature is "not
enabled" but other features of the driver can be (so probe should not
fail and unload the module). Not all devices that have these ACPI IDs
will have keyboard backlight (or various other features that are
supported by this module), but do have other features, so those
features that exist on the specific device should "work" ideally while
others are not made available. This logic matches the behavior from
before but just slightly refactored now to clean it up a bit. Per some
other comments from Armin I will change a bit of this so the debug
messages will be more clear at "point of use" so hopefully it will be
even more clear; does this seem ok or should there also be a comment
or clear text in the debug message that it will continue without
failing the probe?
I thought this might have been the case, but you do propagate errors
from this method to the probe, even though it always returns 0, so it
seems that you wanted to return err instead.
To me it would be better to make this method void like
galaxybook_profile_init() or galaxybook_battery_threshold_init(). But
I'd like to hear Armin's opinion.
I am OK with returning 0 in case some errors are expected and should not cause a probe error.
However errors coming from other subsystems (e.g. error during subsystem registration) should usually
be propagated so that the driver either registers all supported interfaces or none.
Thanks,
Armin Wolf
+ int mapped_profiles;
[...]
+ /* if current mode value mapped to a supported platform_profile_option, set it up */
+ if (mode_profile != IGNORE_PERFORMANCE_MODE_MAPPING) {
+ mapped_profiles++;
mapped_profiles is uninitialized!!
Thank you! A total miss on my part .. and feels like just random
chance that I have not had an issue so far (it seems like it has
always grabbed fresh memory / a value that was already 0) but I will
fix this :)
Thankfully, I think there are kernel configs to auto-initialize stack
variables to 0. That may be why you didn't encounter problems.
+ err = galaxybook_i8042_filter_install(galaxybook);
+ if (err)
+ return dev_err_probe(&galaxybook->platform->dev, err,
+ "failed to initialize i8042_filter\n");
+
+ return 0;
+}
+
+static void galaxybook_remove(struct platform_device *pdev)
+{
+ if (galaxybook_ptr)
+ galaxybook_ptr = NULL;
Please someone correct me if I'm wrong.
Device resources get released after calling the .remove callback,
therefore there is a small window in which the i8042 filter is *still*
installed after this point, which means you could dereference a NULL
pointer.
I suggest not using devres for the i8042 filter.
I believe you are correct, and I checked some of the driver core code
and was able to pinpoint the exact sequence to confirm. This was also
mentioned by Armin in a comment. My intention is that I will actually
fold everything to do with this global pointer into the i8042 init /
remove functions since it is the only thing that uses it, so hopefully
all will work out ok. Also my intention further is if Armin's changes
to add a context pointer to the i8042 filter hook get accepted and
merged then I will move to that and remove this global pointer
entirely :)
Yes, I'm also waiting for it to get merged. I want to implement a filter
in alienware-wmi.
Thanks again for looking into this, and please feel free to say if
there is anything else you find or something I responded with here
that does not sound good!
Sure :)
~ Kurt
Joshua
