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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d9c46324-ff88-4ee3-aa9b-a187a2caf22f@t-8ch.de>
Date: Tue, 7 Jan 2025 18:15:07 +0100
From: Thomas Weißschuh <thomas@...ch.de>
To: Joshua Grisham <josh@...huagrisham.com>
Cc: Armin Wolf <W_Armin@....de>, 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

On 2025-01-07 16:09:51+0100, Joshua Grisham wrote:
> 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.
> 
> 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.

Please take a look at my recent series[0].

It provides an API similar to what you propose.
Currently it does not provide an even higher level API with predefined
structs for enumeration attributes etc. That is intentional for now as
it is also meant to handle custom attribute types.
Nothing would prevent a creation of such a convenience API on top of my
series, however.

We'd need some coordination about landing the different patches,
but that shouldn't really be a problem.

> <snip>

Thomas

[0] https://lore.kernel.org/lkml/20250107-pdx86-firmware-attributes-v1-0-9d75c04a3b52@weissschuh.net/