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? > > + 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 :) > > + 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 :) 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! Joshua