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

[Armin Wolf <W_Armin@gmx.de>]

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@gmail.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.

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