linux-kernel - Re: [PATCH 3/3] platform/x86: dell-pc: Transition to faux device

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: <1e8a6fe0-518d-4eac-9895-51179ca23f36@redhat.com>
Date: Wed, 23 Apr 2025 15:44:56 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Kurt Borja <kuurtb@...il.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Lyndon Sanche <lsanche@...deno.ca>,
 Mario Limonciello <mario.limonciello@....com>,
 platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] platform/x86: dell-pc: Transition to faux device

Hi Ilpo,

On 23-Apr-25 3:27 PM, Ilpo Järvinen wrote:
> On Fri, 11 Apr 2025, Kurt Borja wrote:
> 
>> Use a faux device parent for registering the platform_profile instead of
>> a "fake" platform device.
>>
>> The faux bus is a minimalistic, single driver bus designed for this
>> purpose.
> 
> Hi Kurt, Hans & Greg,
> 
> I'm not sure about this change. So dell-pc not a platform device but
> a "fake".

Arguably the dell-pc driver does not need a struct device at all,
since it just exports /sys/firmware/acpi/platform_profile sysfs
interface by using the relevant Dell SMBIOS interfaces for this.

As such maybe we should just completely get rid of the whole
struct device here?

If we do decide to keep the struct device, then since the struct device
seems to just be there to tie the lifetime of the platform_profile
handler to, I guess that calling it a faux device is fair.

> I'm not saying this is wrong, but feel I'm a bit just lost where the 
> dividing line is.

In this case it seems to be clear that this is a faux device,
but I do agree that sometimes the line can be a bit blurry.

Regards,

Hans




> Is it just because this driver only happens to call
> dell_send_request(), etc., not contains that low-level access code within? 
> Or is that dell-smbios "fake" too?
> 
>> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>> ---
>>  drivers/platform/x86/dell/dell-pc.c | 46 +++++++++++--------------------------
>>  1 file changed, 13 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
>> index 794924913be0c6f13ed4aed8b01ffd21f1d34dea..48cc7511905a62d2828e3a7b593b3d2dae893e34 100644
>> --- a/drivers/platform/x86/dell/dell-pc.c
>> +++ b/drivers/platform/x86/dell/dell-pc.c
>> @@ -13,18 +13,18 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/bitops.h>
>>  #include <linux/bits.h>
>> +#include <linux/device/faux.h>
>>  #include <linux/dmi.h>
>>  #include <linux/err.h>
>>  #include <linux/init.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_profile.h>
>> -#include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  
>>  #include "dell-smbios.h"
>>  
>> -static struct platform_device *platform_device;
>> +static struct faux_device *dell_pc_fdev;
>>  static int supported_modes;
>>  
>>  static const struct dmi_system_id dell_device_table[] __initconst = {
>> @@ -246,7 +246,7 @@ static const struct platform_profile_ops dell_pc_platform_profile_ops = {
>>  	.profile_set = thermal_platform_profile_set,
>>  };
>>  
>> -static int thermal_init(void)
>> +static int dell_pc_faux_probe(struct faux_device *fdev)
>>  {
>>  	struct device *ppdev;
>>  	int ret;
>> @@ -258,51 +258,31 @@ static int thermal_init(void)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	platform_device = platform_device_register_simple("dell-pc", PLATFORM_DEVID_NONE, NULL, 0);
>> -	if (IS_ERR(platform_device))
>> -		return PTR_ERR(platform_device);
>> +	ppdev = devm_platform_profile_register(&fdev->dev, "dell-pc", NULL,
>> +					       &dell_pc_platform_profile_ops);
>>  
>> -	ppdev = devm_platform_profile_register(&platform_device->dev, "dell-pc",
>> -					       NULL, &dell_pc_platform_profile_ops);
>> -	if (IS_ERR(ppdev)) {
>> -		ret = PTR_ERR(ppdev);
>> -		goto cleanup_platform_device;
>> -	}
>> -
>> -	return 0;
>> -
>> -cleanup_platform_device:
>> -	platform_device_unregister(platform_device);
>> -
>> -	return ret;
>> +	return PTR_ERR_OR_ZERO(ppdev);
>>  }
>>  
>> -static void thermal_cleanup(void)
>> -{
>> -	platform_device_unregister(platform_device);
>> -}
>> +static const struct faux_device_ops dell_pc_faux_ops = {
>> +	.probe = dell_pc_faux_probe,
>> +};
>>  
>>  static int __init dell_init(void)
>>  {
>> -	int ret;
>> -
>>  	if (!dmi_check_system(dell_device_table))
>>  		return -ENODEV;
>>  
>> -	ret = thermal_init();
>> -	if (ret)
>> -		goto fail_thermal;
>> +	dell_pc_fdev = faux_device_create("dell-pc", NULL, &dell_pc_faux_ops);
>> +	if (!dell_pc_fdev)
>> +		return -ENODEV;
>>  
>>  	return 0;
>> -
>> -fail_thermal:
>> -	thermal_cleanup();
>> -	return ret;
>>  }
>>  
>>  static void __exit dell_exit(void)
>>  {
>> -	thermal_cleanup();
>> +	faux_device_destroy(dell_pc_fdev);
>>  }
>>  
>>  module_init(dell_init);
>>
>>
>