linux-kernel - Re: [PATCH v4] platform/x86: samsung-galaxybook: Add samsung-galaxybook driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <v4ch4vicbofhr2sawc6synxzf552lxukr73f2qtothdedvoafh@or3ghcu3zqcm>
Date: Fri, 3 Jan 2025 13:52:03 -0500
From: Kurt Borja <kuurtb@...il.com>
To: Joshua Grisham <josh@...huagrisham.com>
Cc: ilpo.jarvinen@...ux.intel.com, hdegoede@...hat.com, W_Armin@....de, 
	platform-driver-x86@...r.kernel.org, corbet@....net, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] platform/x86: samsung-galaxybook: Add
 samsung-galaxybook driver

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@...il.com>:
> >
> > > +     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.

> 
> > > +     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