Message-ID: <9f257ca1-946d-416f-8741-247592b3866c@gmx.de>
Date: Tue, 7 Jan 2025 19:56:37 +0100
From: Armin Wolf <W_Armin@....de>
To: Joshua Grisham <josh@...huagrisham.com>
Cc: ilpo.jarvinen@...ux.intel.com, hdegoede@...hat.com,
 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

Am 07.01.25 um 16:09 schrieb Joshua Grisham:

> Hi again Armin! I think I am finally with you on most of this, I think
> jet lag and general craziness made me a little extra dense for a week
> or two :)
>
> Den lör 4 jan. 2025 kl 07:28 skrev Armin Wolf <W_Armin@....de>:
>> The reason for the firmware-attribute class original was that driver could export BIOS settings
>> to userspace applications, together with some metadata (min/max values, etc).
>>
>> Because of this the exact meaning of each firmware attribute is usually only known to the user
>> changing those firmware attributes.
>>
>> Your driver is a bit special in that it knows the meaning of each firmware attribute. However
>> you still have to follow the firmware-attribute class ABI since userspace applications do not
>> know this.
>>
> Yes ok, as said, I am with you all now on this I think :)
>
> As a prototype for v5 I have created a new struct for each "firmware
> attribute" that helps me keep everything linked together with all of
> the different sub attributes for each different "fw attribute"
> including allowing a link back to my samsung_galaxybook instance
> without using the global pointer. At the end of the day, if I wanted
> to avoid using a global pointer, I needed a way to grab the private
> data based on either the kobj or the attr parameters to the show/store
> method of these individual sub attributes within each "firmware
> attribute", so what I have done is added the kobj_attribute as a
> struct member and then manually init+filled this kobj_attributes
> during probe, so I can now grab the parent struct instance using
> container_of() within the show/store functions which then gets me to
> my pointer. I thought about using the kset or something else for this
> but it seemed like kobj_attribute supported being a struct member
> better and gave the least amount of headaches from what I could tell.

Sounds reasonable to me.

> After trying to fight my way through this problem, I have an idea of
> what a better "dream scenario" would for me as a user/consumer of the
> firmware attributes interface -- namely that there is some kind of way
> to register and unregister by "type" (e.g. "I want a new enumeration
> fw attr; here is its parent, its name, and all of the functions for
> show/store of the required attributes, plus a data pointer that I can
> pack together with my attribute/somehow reach within the show/store
> functions"). I have handled a bit of this myself now in the working v5
> of samsung-galaxybook (just a minimal version of what it requires) but
> as said it currently relies on creating the kobj_attributes (at least
> those where I need the pointer) as struct members that I can later use
> with container_of() instead of creating static ones using the various
> __ATTR.. macros.
>
> Please feel free to say if any of this sounds totally (or partially?)
> off, otherwise I will try to test a bit more, clean up, and work
> through any checkpatch exceptions and get this sent as a v5.

I think your current plan sounds good. Thomas already submitted a patch series which
provides a more abstract API for registering firmware attributes.

>>>>> +static void galaxybook_fw_attr_class_remove(void *data)
>>>>> +{
>>>>> +     device_destroy(fw_attr_class, MKDEV(0, 0));
>>>> Please use device_unregister() instead since multiple devices might share the same devt of MKDEV(0, 0).
>>>> This would also allow you to remove the global variable "fw_attr_class".
>>>>
>>> Here I am a bit confused on exactly how this would/should look; all
>>> existing usages of fw_attr_class I can see use exactly this same
>>> pattern: device_create() and then device_destroy() with MKDEV(0, 0).
>>> Taking a look at the latest proposed changes from Thomas and it stil
>>> seems the intention is the same, just that it is slightly simplified
>>> and use pointer to the firmware_attributes_class directly instead of
>>> fetching it using fw_attributes_class_get(). Or is there a better way
>>> to do this (including usage of device_unregister() and/or something
>>> different with the devt) that will help solve some other problem(s)?
>> This is the code of device_destroy():
>>
>> void device_destroy(const struct class *class, dev_t devt)
>> {
>>          struct device *dev;
>>
>>          dev = class_find_device_by_devt(class, devt);
>>          if (dev) {
>>                  put_device(dev);
>>                  device_unregister(dev);
>>          }
>> }
>>
>> if multiple devices of a given class are using the same devt (like MKDEV(0, 0)) then
>> class_find_device_by_devt() might pick the wrong device.
>>
>> The fact that the other drivers are using this function is actually an error. The only
>> reason why this error was not noticed until now seems to be that currently only a single
>> driver using the firmware-attribute class is typically active at the same time.
>>
> Yes again sorry for being dense -- now with a little sleep and time to
> marinate this makes total sense, and it is a lot easier to just use
> device_unregister() like you say. This will be included in v5.

I partly blame the comment of device_destroy(), at first glance it looks like the
natural complement of device_create(), even if its not.

I will see if i can create a patch series to fix this.

>>> Also there are several other platform drivers that implement a very
>>> similar device attribute as ones that I have added here as a firmware
>>> attribute (namely I am thinking of "USB Charging" which exists in
>>> several other pdx86 drivers but a few other devices should/would
>>> probably support this kind of "Power on Lid Open" attribute as well);
>>> in the event that maintainers of those drivers should and eventually
>>> do migrate over to use the same or similar firmware attribute for this
>>> same kind of setting, should it include all of these attributes in the
>>> standard "enumeration" type attribute group or is it possible / would
>>> it make sense to have some sort of boolean-based fw attr type that is
>>> a bit more simple and a bit more self-explanatory?
>> Introducing a new "boolean" type would indeed be nice. This would allow userspace application to use a simple
>> on/off slider instead of a dropdown menu when displaying such firmware attributes.
>>
>> In this case you could drop the "possible_values" attribute.
>>
>> What is the opinion of the pdx86 maintainers on this proposal?
>>
> Now that I have finally taken a better understanding of this, I see
> your point. Yes, nice with a boolean that could give a slider in a GUI
> or similar, but does not really change a whole lot in the driver
> implementation. I will go with enumeration type for now as mentioned
> and it can always be changed later if this new type comes.

Ok.

>
>>> I am quite certain that the code can be cleaned up and/or refactored a
>>> bit, but I would hope that the result of the logic should stay the
>>> same (per what I described above); having said all of that, does it
>>> still make sense to try and simplify this somehow and/or any tips or
>>> recommendation how to achieve the desired result in a better way?
>> I am OK with you preferring the non-legacy modes over the legacy ones. However trying to limit yourself
>> to the profiles currently supported by gnome (AFAIK uses platform-profiles-daemon) is not a good idea.
>>
>> I would like to see a more static mapping be implemented:
>>
>> PERFORMANCE_MODE_ULTRA -> performance
>> PERFORMANCE_MODE_PERFORMANCE -> balanced-performance (can also be legacy if non-legacy is not available)
>> PERFORMANCE_MODE_OPTIMIZED -> balanced (can also be legacy is non-legacy is not available)
>> PERFORMANCE_MODE_QUIET -> quiet
>> PERFORMANCE_MODE_SILENT -> low-power
>>
>> In this case the platform-profiles-daemon would have a similar job as the Samsung service, which is to
>> determine a suitable mapping for the supported modes to {performance, balanced, powersave}.
>>
>> Looking at the code of the daemon it seems something similar is already being done, but only for the profiles
>> "quiet" and "low-power" (one of which is getting mapped to the "powersave" mode).
>>
>> I am confident that the daemon could be extended be a bit more intelligent when it comes to determine the
>> mapping of the other modes.
>>
> I understand the thought here but my only problem and what sort of
> "itches" at me is that most of these devices are not "Ultra" models
> and they will never have an "Ultra" mode. For the non-Ultra models,
> "Performance mode" *is* "Performance mode" (meaning, it is the mode
> which prioritizes performance over anything else) so for me it feels
> best if these non-Ultra models (again majority of these devices) can
> have the Performance mode that they should have. And you can maybe
> argue that "Ultra" is in fact its own mode entirely -- when you use
> this mode on these devices, they really scream (the fans, mostly, that
> is) and they get super hot haha :)

Is this non-ultra performance mode any different than the ultra performance mode
in terms of performance gains, fan speed, etc?

> Other than this Ultra vs Performance question, I do agree with you and
> think it makes sense. My first thought if we want to actually
> "simplify" this in this way is if there could actually exist a
> platform profile called "ultra" then it would be just a perfect 1:1
> mapping (other than taking legacy modes into account).
>
> This "perfect fit" for samsung-galaxybook would be to create a new
> platform profile called something like PLATFORM_PROFILE_ULTRA, but
> that seems like a bit of a tall order... Would it make more sense to
> implement this "ultra" mode using the new PLATFORM_PROFILE_CUSTOM and
> then map them like this?
>
> PERFORMANCE_MODE_ULTRA -> custom (named "ultra" if that is possible?)
> PERFORMANCE_MODE_PERFORMANCE (or PERFORMANCE_MODE_PERFORMANCE_LEGACY)
> -> performance
> PERFORMANCE_MODE_OPTIMIZED (or PERFORMANCE_MODE_OPTIMIZED_LEGACY) -> balanced
> PERFORMANCE_MODE_QUIET -> quiet
> PERFORMANCE_MODE_SILENT -> low-power
>
> Thought admittedly I am not 100% familiar with how
> PLATFORM_PROFILE_CUSTOM is implemented to work; I have a vague memory
> that I read somewhere that this was roughly the intention? But I am
> not sure if it is actually implemented to work this way. But if it
> will in fact work "out of the box" including with
> platform_profile_cycle() for the hotkey then it seems like the
> cleanest and easiest approach.

PLATFORM_PROFILE_CUSTOM is meant to signal that the platform is not in a well-defined
profile state, usually due to manual tuning. So please do not use it for ULTRA.

>
> If this is possible, then my best guess for the logic for this mapping
> in samsung-galaxybook could be changed to loop the "supported modes"
> forwards instead of backwards, and just let the "legacy" modes be
> written first (as they seem to always come first in the list), and
> then in case the non-legacy mode exists later in the array, it will
> just replace the already-mapped legacy value with the new non-legacy
> value, and thus skip any kind of condition-based checking/mapping
> entirely. Is that sort of more like what you had in mind?

Can you be sure that legacy performance modes are always placed before non-legacy
performance modes?

If no then i suggest that you iterate over all supported modes and if you encounter
a legacy performance mode you check if the associated platform profile slot was already
taken by a non-legacy performance mode. If that is the case you ignore that legacy performance
mode.

If you are sure that the order is always the same then you can of course simplify this by
iterating forward. I will leave it to you to choose which one to implement, as you seem
to have more knowledge about the underlying hardware than me.

Thanks,
Armin Wolf

>> Thanks,
>> Armin Wolf
>>
> Thanks again!
>
> Joshua
>
>> [...]

Powered by blists - more mailing lists