linux-kernel - Re: [PATCH] platform/x86: thinkpad_acpi: Fix registration of tpacpi platform 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: <D7MSPR52PB4E.N0X1UFVQOODZ@gmail.com>
Date: Fri, 07 Feb 2025 23:56:27 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Mark Pearson" <mpearson-lenovo@...ebb.ca>
Cc: <hdegoede@...hat.com>, <ilpo.jarvinen@...ux.intel.com>,
 <mario.limonciello@....com>, <platform-driver-x86@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Fix registration of tpacpi
 platform driver

Hi Mark,

On Sat Feb 8, 2025 at 4:14 AM -05, Mark Pearson wrote:
> When reviewing and testing the recent platform profile changes I had
> missed that they prevent the tpacpi platform driver from registering.
> This error is seen in the kernel logs, and the various tpacpi entries
> are not created:
> [ 7550.642171] platform thinkpad_acpi: Resources present before probing

This happens because in thinkpad_acpi_module_init(), ibm_init() is
called before platform_driver_register(&tpacpi_pdriver), therefore
devm_platform_profile_register() is called before tpacpi_pdev probes.

As you can verify in [1], in the probing sequence, the driver core
verifies the devres list is empty, which in this case is not because of
the devm_ call.

>
> I believe this is because the platform_profile driver registers the
> device as part of it's initialisation in devm_platform_profile_register,
> and the thinkpad_acpi driver later fails as the resource is already used.
>
> Modified thinkpad_acpi so that it has a separate platform driver for the
> profile handling, leaving the existing tpacpi_pdev to register
> successfully.

While this works, it does not address the problem directly. Also it is
discouraged to create "fake" platform devices [2].

May I suggest moving tpacpi_pdriver registration before ibm_init()
instead, so ibm_init_struct's .init callbacks can use devres?

Thanks for noticing this!

~ Kurt

[1] https://elixir.bootlin.com/linux/v6.13.1/source/drivers/base/dd.c#L626
[2] https://lore.kernel.org/rust-for-linux/2025020620-skedaddle-olympics-1735@gregkh/

>
> Tested on X1 Carbon G12.
>
> Fixes: 31658c916fa6 ("platform/x86: thinkpad_acpi: Use devm_platform_profile_register()")
>
> Signed-off-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 1fcb0f99695a..1dd8f3cc5eda 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -270,6 +270,7 @@ enum tpacpi_hkey_event_t {
>  #define TPACPI_DRVR_NAME TPACPI_FILE
>  #define TPACPI_DRVR_SHORTNAME "tpacpi"
>  #define TPACPI_HWMON_DRVR_NAME TPACPI_NAME "_hwmon"
> +#define TPACPI_PROFILE_DRVR_NAME TPACPI_NAME "_profile"
>  
>  #define TPACPI_NVRAM_KTHREAD_NAME "ktpacpi_nvramd"
>  #define TPACPI_WORKQUEUE_NAME "ktpacpid"
> @@ -962,6 +963,7 @@ static const struct proc_ops dispatch_proc_ops = {
>  
>  static struct platform_device *tpacpi_pdev;
>  static struct platform_device *tpacpi_sensors_pdev;
> +static struct platform_device *tpacpi_profile_pdev;
>  static struct device *tpacpi_hwmon;
>  static struct device *tpacpi_pprof;
>  static struct input_dev *tpacpi_inputdev;
> @@ -10646,7 +10648,8 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>  			"DYTC version %d: thermal mode available\n", dytc_version);
>  
>  	/* Create platform_profile structure and register */
> -	tpacpi_pprof = devm_platform_profile_register(&tpacpi_pdev->dev, "thinkpad-acpi",
> +	tpacpi_pprof = devm_platform_profile_register(&tpacpi_profile_pdev->dev,
> +						      "thinkpad-acpi-profile",
>  						      NULL, &dytc_profile_ops);
>  	/*
>  	 * If for some reason platform_profiles aren't enabled
> @@ -11815,6 +11818,8 @@ static void thinkpad_acpi_module_exit(void)
>  
>  	if (tpacpi_sensors_pdev)
>  		platform_device_unregister(tpacpi_sensors_pdev);
> +	if (tpacpi_profile_pdev)
> +		platform_device_unregister(tpacpi_profile_pdev);
>  	if (tpacpi_pdev)
>  		platform_device_unregister(tpacpi_pdev);
>  	if (proc_dir)
> @@ -11901,6 +11906,17 @@ static int __init thinkpad_acpi_module_init(void)
>  		thinkpad_acpi_module_exit();
>  		return ret;
>  	}
> +
> +	tpacpi_profile_pdev = platform_device_register_simple(TPACPI_PROFILE_DRVR_NAME,
> +							      PLATFORM_DEVID_NONE, NULL, 0);
> +	if (IS_ERR(tpacpi_profile_pdev)) {
> +		ret = PTR_ERR(tpacpi_profile_pdev);
> +		tpacpi_profile_pdev = NULL;
> +		pr_err("unable to register platform profile device\n");
> +		thinkpad_acpi_module_exit();
> +		return ret;
> +	}
> +
>  	tpacpi_sensors_pdev = platform_device_register_simple(
>  						TPACPI_HWMON_DRVR_NAME,
>  						PLATFORM_DEVID_NONE, NULL, 0);