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?
> > +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 :)
> --
> i.
Thanks again!
Joshua
