On Fri, 10 Jan 2025, Joshua Grisham wrote:
> Hi Ilpo, thanks for taking the time! Few clarifications/comments below...
>
> Den fre 10 jan. 2025 kl 12:30 skrev Ilpo Järvinen
> <ilpo.jarvinen@xxxxxxxxxxxxxxx>:
> >
> > > +static int get_performance_mode_profile(struct samsung_galaxybook *galaxybook,
> > > + const u8 performance_mode,
> > > + enum platform_profile_option *profile)
> > > +{
> > > + for (int i = 0; i < PLATFORM_PROFILE_LAST; i++) {
> >
> > unsigned int is preferred for loop variables that never should become
> > negative.
> >
>
> Thanks for the catch! There are a few other places with a for loop
> using int i so I will go ahead and change all of them to unsigned ints
> for the next version unless you say otherwise.
>
> > > + if (num_mapped == 0) {
> > > + dev_dbg(&galaxybook->platform->dev, "failed to map any performance modes\n");
> > > + return 0;
> >
> > Should this return error instead? I assume it's because you want to
> > initialize with part of the features only but...
> >
>
> Yes at this point it is "no harm, no foul": the profile_handler has
> not been set up, platform profile has not been registered, and
> has_performance_mode is false, so I want that it just exits and the
> probe continues to init other features (e.g. devices with SAM0427 have
> kbd backlight controller and firmware attributes but not this specific
> performance_mode implementation, so for them this function will just
> stop anywhere along the way at whatever point it fails and just "not
> doing anything else" but still let them use the other features of the
> driver... all other features that then check against
> has_performance_mode will see that it is false and just skip that
> part). Does this still seem ok or is there any adjustment you would
> like to see for this?
>
> > > + /* if startup performance_mode fails to match a profile, try to set init mode */
> > > + err = get_performance_mode_profile(galaxybook, performance_mode, NULL);
> > > + if (err) {
> > > + if (init_performance_mode == GB_PERFORMANCE_MODE_UNKNOWN) {
> > > + dev_err(&galaxybook->platform->dev, "missing initial performance mode\n");
> > > + return -ENODATA;
> > > + }
> > > + err = performance_mode_acpi_set(galaxybook, init_performance_mode);
> > > + if (err) {
> > > + dev_err(&galaxybook->platform->dev,
> > > + "failed to set initial performance mode 0x%x\n",
> > > + init_performance_mode);
> > > + return err;
> >
> > ...why these two cases then result in failing everything vs. just platform
> > profile feature? Not an end of the world but it feels inconsistent to me.
> >
>
> I am glad you bring this up, as it forces me think through this a few
> more rounds... basically what happens is that the device will always
> come up from a fresh boot with the value of 0x0 as the "current
> performance mode" as response from the ACPI method, even though for
> most devices value 0x0 is the "legacy" optimized value that should not
> be used.
>
> In Windows, the Samsung background apps take care of this by storing
> last-used value from previous session and then re-applying it after
> startup (and the same happens with various userspace services on
> platform profile from what I have seen, actually).
>
> But since the driver does not map 0x0 to any valid profile unless the
> device only has the "legacy" optimized mode, then my function
> get_performance_mode_profile() will return -ENODATA in this initial
> startup state of 0x0. What I noticed is that the first time after this
> that you run platform_profile_cycle() after this, there is a little
> "hiccup" and an error "Failed to get profile for handler .." is
> printed in the kernel log from platform_profile.c (without pr_fmt by
> the way), but then it just works anyway and starts to pick up from the
> first enabled profile and then can continue to cycle.
>
> My bit of code here was to basically try to "force" to set the profile
> to whatever was successfully mapped as "balanced" upon a fresh startup
> so that when platform_profile_cycle() first runs there would not be
> any condition where get_performance_mode_profile() would return
> -ENODATA. Then the userspace tools would take over anyway and restore
> your last session's latest used profile so it would not matter so
> much. In the end, really the aim I guess is to avoid this error
> message in the kernel log, but otherwise everything works even though
> there is an error message printed, but this is maybe a bit overkill?
> And by the way, that, as you say, we should probably not fail the
> entire feature just because of this, but let the error happen anyway
> and let everything work after that.
>
> Possibly more "simple" alternatives I can think of off the top of my head:
>
> 1. Let get_performance_mode_profile() return 0 instead of -ENODATA if
> nothing is matched , this way a non-mapped performance mode would
> always just set platform_profile_cycle() to the start of the cycle I
> guess/would hope? and/or add a special case in
> get_performance_mode_profile() for performance_mode=0 to just return 0
> to get the same effect ? (though what happens if we have not enabled
> PLATFORM_PROFILE_LOW_POWER in the choices? even though the function
> returned 0, will platform_profile see that 0 is not supported, and
> just keep moving on until it gets to the first one that is? If so then
> this will work, but I have not yet tested or fully checked the code to
> ensure that this will actually be the behavior...)
>
> 2. Try to do the logic which I did with this init_performance_mode,
> but in case init_profile_mode is not set, just skip it and let the
> error come from platform_profile_cycle() anyway and it should start to
> work. In this case I think it would be good if the user is maybe
> flagged somehow and create a bug for this, because I would want to be
> able to work with them to see what modes are reported by their device
> and see if the mapping needs to be updated in some way.
>
> 3. Do neither of these, remove everything with init_performance_mode,
> and just let platform_profile_cycle() fail upon startup and print the
> error message and then it should just start working after anyway.
>
> 4. Map 0x0 performance_mode to PLATFORM_PROFILE_CUSTOM in case the
> "legacy" mode with this value is not mapped, then the hotkey would not
> work to cycle the profile at first as the user would be forced to set
> the profile to a value via either a GUI or the sysfs interface before
> they can begin to use the hotkey to cycle the profile. Once they do
> this the very first time, then the userspace tools should kick in
> after this (upon every restart for example) to set the profile to the
> prior session's last used profile and then the hotkey will start to
> work to cycle the profile anyway in that session without any
> intervention from the user at all (so it is the very first time that
> they start their environment up, assuming that the prior value does
> not get cleared somehow due to some combo like the module being
> removed/blacklisted and then they restart, then add it back, etc, or
> some other corner-case situation?)
>
> I do think that something should change, maybe the most
> straight-forward are either 1 or 2 in this list, but not sure if there
> are any opinions or preferences on these or if there are other better
> options I did not think of here?
I think you entirely missed my point, which is that also
galaxybook_probe() will fail if you return an error from this function.
That is, you won't have battery, backlight, etc. when the probe fails.
This is a bit inconsistent with the other case I mentioned above where you
get everything else but platform profiles because 0 is returned.
Also, devm_platform_profile_register() won't be called regardless of
whether you return errno or 0 at this point so there won't be platform
profile handling registered anyway so most of your discussion seems
irrelevant.
> > > +static ssize_t current_value_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct galaxybook_fw_attr *fw_attr =
> > > + container_of(attr, struct galaxybook_fw_attr, current_value);
> > > + struct samsung_galaxybook *galaxybook = fw_attr->galaxybook;
> > > +
> > > + bool value;
> >
> > Remove the extra empty line from within variable declarations.
> >
>
> Yes sorry this was just a miss when so much got moved around due to
> the big changes between v4 and v5; will fix this and the other small
> straight-forward issues for v6 :)
No need to be sorry about things like that, it happens to everyone when
things are moving rapidly.
--
i.
