Hi Ilpo,
Den fre 10 jan. 2025 kl 17:34 skrev Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx>:
>
> 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.
>
I did get the point I think and was why I said both "I do think that
something should change" and specifically when I said "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." I can imagine that with my wall of words it is easy
that the message could get lost in there -- that is on me :)
But regardless, yes, it should not return here, but I was more
thinking about how to better/more cleanly handle the situation so that
the logic is less "hacky", if that makes sense :)
> --
> i.
Thanks again!
Joshua
