Received-SPF: pass (google.com: domain of linux-kernel+bounces-193108-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <371d4109-a3bb-4c3b-802f-4ec27a945c99@amd.com>
Date: Tue, 28 May 2024 16:16:41 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting
To: Luke Jones <luke@ljones.dev>, Hans de Goede <hdegoede@redhat.com>,
 =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc: corentin.chary@gmail.com, platform-driver-x86@vger.kernel.org,
 linux-kernel@vger.kernel.org, richard@hughsie.com
References: <20240528013626.14066-1-luke@ljones.dev>
 <20240528013626.14066-9-luke@ljones.dev>
 <6f4bc109-00d0-47b0-a581-b96a6152545c@amd.com>
 <4d6b9171-7248-4937-87de-7e921ed8e507@app.fastmail.com>
 <5f4799b1-0606-46a9-a347-5a03738db341@amd.com>
 <ac597520-fb5b-40e5-ae1f-de825450d2db@app.fastmail.com>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <ac597520-fb5b-40e5-ae1f-de825450d2db@app.fastmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Ny9nZkdYZEFqMEtIaExtS2FFeW5lcUtJN09FZEVpTUZzSkRrRVRPNjc5VzVK?=
 =?utf-8?B?THlValI3LzZmQ2Z5Wm5YZHFBQVdOK0s4Z2hlQ1VDWVB0Sld5aWUwZ3NMRUFB?=
 =?utf-8?B?VnBvaTZiZUxWbUh6S0Rpbk5MbThXYlJWUHVhRXRPL3dhNU85OGtmSnl1MzBr?=
 =?utf-8?B?N2xFd0hoQTJ5UFBnV2laa1J0WWszdXovbytNREd3Tk4weml2NXg5a1M0M3Ny?=
 =?utf-8?B?S2FJeXI2cVhXMGZmejBDbDdORXFzNEM5TUZSYzdVYlZMcE40YzZIOGlsay95?=
 =?utf-8?B?S290WDJaejVuYjhCL282WjBUWUxvM0RBeFhzcWRxa0drZkVzZGFXYUhPMjRy?=
 =?utf-8?B?YTZXRjFjRlFiUzRRSWt1TzREaXV5MXRXNHRpRzJhbHV3ZDlSQStiQXphQVc1?=
 =?utf-8?B?aFNQczlTK3VkYlBxR1NaL1RpbmVrNmU0d0NGSnpBNldrVlhLQ0lFRml5SUxi?=
 =?utf-8?B?OTYvNllUUFFxRk14VDJYNThlaFRlQkx5K245TlU3OVNML1hhWjlwdmpXRUNE?=
 =?utf-8?B?VkltYTFiVm1ZUnllMFlxZk1OZnFqMXREOXFLOUNHTENSWFQ0ZldwcG9uRDlZ?=
 =?utf-8?B?OHNYWTIzRC9sKzFyWkFyZGlJWkl4eXVya243WUI1VXVVUkF6bFQ3VktsVWoy?=
 =?utf-8?B?M0hIUStRaERRbmFXL2tEbnIvMmhqYUtOVUpVT3kyUHRXM2VlMmZrMG5WYXJX?=
 =?utf-8?B?UnRGT3I5a0xUUnhIV2FjNmFVNTRTTGVFN28ySXdJU2xEQzVWemVxL1d0ZGl6?=
 =?utf-8?B?VDc5b3NSRk04QXZUUnhidEVpQWJNRVM3d0pGUzNkZ0hrUzlxSU5CMTdka0V4?=
 =?utf-8?B?SGFrYkh3TlZhYStLa29FMmdKQnl0eUg4RDBrMHJEcmhGTEdZMGhlcmpXS2Jy?=
 =?utf-8?B?SjBwd1lIOFlsNUhlS3p3RithVkQ1YmJ5eENjQ09jUUp1ZGR3ZXhkeVdoQjR6?=
 =?utf-8?B?RkZkWnoyOTMrRHVaYUxpUHFIZVF6TDBlallMTlVLL3p2OE53SXB6TmRCZmpn?=
 =?utf-8?B?NGxyalI2QnpBUlhuYkNkR0hCU2czK3psaFN6MVBNaUZzSVpIb1BIdFg5MFRh?=
 =?utf-8?B?bjM1eHlyOWNDRStDeXV4YU9URThFeFRNcjdPalZ3SzhSbytaaFRRUU9PZlhp?=
 =?utf-8?B?N2RFWHhRUVNDcjlvSVg1b2VNQUVzZU9wNndGUGJWYlhNc2d0SG9CQnRka09F?=
 =?utf-8?B?WElvQ1BxZ1NraFpDYXZOeHlqV1RLT2hRMmVqZG9iYzRrcWppdWRxNEJ1RFlK?=
 =?utf-8?B?R0RyS0M4UVVPTWMvbVpjTW9GZ2J4TWo3NEViZmNQVFp1ODhHN0NGWXgzUTIv?=
 =?utf-8?B?aXZaTUtnL3JqTDFHUUtuOGtSMDVvZXRMQkQ0Sjh6NnZtR1plUGhGTWZRaFRy?=
 =?utf-8?B?Z0o1Qkl3bmI5d1RPT0xIYnA5NEdKYVE0MGIzWW4rRGRheXBUOHJ0REdKRjMz?=
 =?utf-8?B?SU9vMENzNU1zc2JGdTJUVWExV0lQYm91cnRZMEdvbVROMldCOWVnRGFMY1FK?=
 =?utf-8?B?dXRHQkUyY3dpN3k4dWVEaU1JVG5qclprc04yUUROSTVzNTJqWVJ4cHA4TzQv?=
 =?utf-8?B?cGRLM3BYRkcxWHJSQVFoMEFpWjlzZmhOTktmMUYreXREU3M4TDVidkQwL1dj?=
 =?utf-8?B?azV6S1p0cUpzeU1yYVRPTmY4bC83SnE0blpUUzJpYTBoMTUrbStkUkZwVUZQ?=
 =?utf-8?B?cHJHWFVBYWFYZldZQ1hDUEFrbnFBbjBkNU9Pd2RsSkppbXVOcWdaMTFod2RY?=
 =?utf-8?B?WW5JUjVPSm5NSE50WjlLOFB2TEtFVGZuWFZjMWRRUTZmaG5QSGIxR21jcnZz?=
 =?utf-8?B?SUtPc3pySDIyMVJlN0FWZk9sL1hXa2J1TE5rbkwzbjl4YjV6eHQ2RGgvWEJm?=
 =?utf-8?B?Rm1aL3kyQitEeVpXV0JIRm5tZW5mQVhXUFh6Wk40dmFWYjY1RFR1MVhkT0NL?=
 =?utf-8?B?Z3lZbFhiMVJHaTQzZ3JIbHhQcWFIT2hha2Q3TlJ3djFyWjdwZFhXU3B3WTJU?=
 =?utf-8?B?OHgyc3pzenF1R3p0K2J0U21HR3lFR3ppcWRNazcySkU4dHNRVUxocjlzS0hU?=
 =?utf-8?B?YmNsUzNxTnpVZU5Gd0lDVm1MeHBQRk9lRzNTRlpFRlBlL2VEZWpoZHo0SmNp?=
 =?utf-8?Q?a73RxHWPCH90MUDQ0OfTBDz4K?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a304265-c425-40a5-a739-08dc7f5b79c6
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 May 2024 21:16:44.2881
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: z0yHbGg5zRv8QKafmwnq0op1awaIUpzWV5kvTV8z2RxUbq8GM8xs/V+ajY3vG0/lK4dn5doEEIRwT73U5OhJgQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB7727

>> For any "new" attributes it's better to put them in that API than code
>> duplication of the BIOS attributes API as well as a random sysfs file
>> API that you can never discard.
> 
> Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also.
> 
> If I'm understanding the docs correctly, for example this apu_mem attr would then become:
> - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type
> - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value
> - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value
> - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name
> - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values
> - ..etc
> 
> That's absolutely much better than what I've been doing and I wish I'd known about it sooner.

Yup!  And even better "fwupdmgr get-bios-setting" and "fwupdmgr 
set-bios-setting" [1] will work with bash completion too.

If you want to build a GUI around it, it could use the fwupd dbus 
interface for getting those settings and manipulating them.  The dbus 
interface policy is protected by PK.

> 
> So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps).
> 
> What I don't want is a split between platform and firmware_attributes.
> 

My personal thought is add everything to the new API and then hide the 
"old" interface behind some boolean Kconfig option that defaults to 
disabled.  If someone enables it then when it's "used" (IE read from / 
written to) then do something like a pr_warn_once() that it's a 
deprecated interface and it will go away.  Wait until next LTS kernel, 
and then rip it out after that.

Need Hans and/or Ilpo agree to make with that though before you spend 
too much effort on it though.

[1] https://fwupd.github.io/libfwupdplugin/bios-settings.html