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: <0720575c-88c3-4f88-caa7-4707139702a1@metux.net>
Date:   Thu, 22 Oct 2020 11:22:32 +0200
From:   "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
To:     Ed Wildgoose <lists@...dgooses.com>, linux-kernel@...r.kernel.org
Cc:     fe@....tdt.de, hdegoede@...hat.com,
        "Enrico Weigelt, metux IT consult" <info@...ux.net>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 1/2] x86: Conditional init of pcengines leds/keys gpios

On 21.10.20 23:41, Ed Wildgoose wrote:

Hi,

> The pcengines bios/firmware includes ACPI tables (since 4.10.0.1) which
> will cause the kernel to automatically create led + gpio_key devices for
> the platform. This means that the platform setup now creates duplicates
> of all these led/key devices.
> 
> Driver conditionally initialises leds/keys only for older bios.

still breaks existing userlands as device names change - LEDs as well as
keys, and keycodes also change.

IOW: userland needs to be depending on specific BIOS versions.


--mtx


> 
> Signed-off-by: Ed Wildgoose <lists@...dgooses.com>
> ---
>  drivers/platform/x86/pcengines-apuv2.c | 115 +++++++++++++++++++------
>  1 file changed, 90 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
> index c37349f97..45f7a89de 100644
> --- a/drivers/platform/x86/pcengines-apuv2.c
> +++ b/drivers/platform/x86/pcengines-apuv2.c
> @@ -128,6 +128,18 @@ static struct gpiod_lookup_table gpios_key_table = {
>  
>  /* Board setup */
>  
> +struct apu_driver_data {
> +	const struct amd_fch_gpio_pdata *board_data;
> +	const struct gpio_keys_platform_data *apu_keys_pdata;
> +	const struct gpio_led_platform_data *apu_leds_pdata;
> +};
> +
> +static struct apu_driver_data apu2_driver_data = {
> +	.board_data = &board_apu2,
> +	.apu_keys_pdata = &apu2_keys_pdata,
> +	.apu_leds_pdata = &apu2_leds_pdata
> +};
> +
>  /* Note: matching works on string prefix, so "apu2" must come before "apu" */
>  static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  
> @@ -138,7 +150,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "APU2")
>  		},
> -		.driver_data	= (void *)&board_apu2,
> +		.driver_data	= (void *)&apu2_driver_data,
>  	},
>  	/* APU2 w/ legacy BIOS >= 4.0.8 */
>  	{
> @@ -147,7 +159,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "apu2")
>  		},
> -		.driver_data	= (void *)&board_apu2,
> +		.driver_data	= (void *)&apu2_driver_data,
>  	},
>  	/* APU2 w/ mainline BIOS */
>  	{
> @@ -156,7 +168,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu2")
>  		},
> -		.driver_data	= (void *)&board_apu2,
> +		.driver_data	= (void *)&apu2_driver_data,
>  	},
>  
>  	/* APU3 w/ legacy BIOS < 4.0.8 */
> @@ -166,7 +178,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "APU3")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU3 w/ legacy BIOS >= 4.0.8 */
>  	{
> @@ -175,7 +187,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "apu3")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU3 w/ mainline BIOS */
>  	{
> @@ -184,7 +196,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu3")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU4 w/ legacy BIOS < 4.0.8 */
>  	{
> @@ -193,7 +205,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "APU4")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU4 w/ legacy BIOS >= 4.0.8 */
>  	{
> @@ -202,7 +214,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "apu4")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU4 w/ mainline BIOS */
>  	{
> @@ -211,11 +223,42 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu4")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	{}
>  };
>  
> +int cmp_version(const char *base, const char *test)
> +{
> +	int i;
> +	int oct_base[4], oct_test[4];
> +	int matched_base, matched;
> +
> +	matched_base = sscanf(base, "v%4d.%4d.%4d.%4d",
> +					&oct_base[0], &oct_base[1],
> +					&oct_base[2], &oct_base[3]);
> +	matched = sscanf(test, "v%4d.%4d.%4d.%4d",
> +					&oct_test[0], &oct_test[1],
> +					&oct_test[2], &oct_test[3]);
> +
> +	/* opinionated: match as lower if we can't parse test version */
> +	if (matched == 0)
> +		return -1;
> +
> +	if (matched_base < matched)
> +		matched = matched_base;
> +
> +	for (i = 0; i < matched; i++) {
> +		if (oct_test[i] > oct_base[i])
> +			return 1;
> +		else if (oct_test[i] < oct_base[i])
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +
>  static struct platform_device *apu_gpio_pdev;
>  static struct platform_device *apu_leds_pdev;
>  static struct platform_device *apu_keys_pdev;
> @@ -244,41 +287,63 @@ static struct platform_device * __init apu_create_pdev(
>  static int __init apu_board_init(void)
>  {
>  	const struct dmi_system_id *id;
> +	const struct apu_driver_data *driver_data;
> +	char const *bios_ver;
> +
> +	apu_leds_pdev = apu_keys_pdev = NULL;
>  
>  	id = dmi_first_match(apu_gpio_dmi_table);
>  	if (!id) {
>  		pr_err("failed to detect APU board via DMI\n");
>  		return -ENODEV;
> +	} else {
> +		pr_info("Detected APU board: %s - BIOS: %s\n",
> +		dmi_get_system_info(DMI_BOARD_NAME),
> +		dmi_get_system_info(DMI_BIOS_VERSION));
>  	}
>  
> -	gpiod_add_lookup_table(&gpios_led_table);
> -	gpiod_add_lookup_table(&gpios_key_table);
> +	driver_data = id->driver_data;
> +	bios_ver = dmi_get_system_info(DMI_BIOS_VERSION);
>  
>  	apu_gpio_pdev = apu_create_pdev(
>  		AMD_FCH_GPIO_DRIVER_NAME,
> -		id->driver_data,
> +		driver_data->board_data,
>  		sizeof(struct amd_fch_gpio_pdata));
>  
> -	apu_leds_pdev = apu_create_pdev(
> -		"leds-gpio",
> -		&apu2_leds_pdata,
> -		sizeof(apu2_leds_pdata));
> -
> -	apu_keys_pdev = apu_create_pdev(
> -		"gpio-keys-polled",
> -		&apu2_keys_pdata,
> -		sizeof(apu2_keys_pdata));
> +	if (cmp_version("v4.10", bios_ver) < 0) {
> +		/* Newer bios configure LEDs/keys via ACPI */
> +
> +		if (driver_data->apu_keys_pdata) {
> +			gpiod_add_lookup_table(&gpios_key_table);
> +			apu_keys_pdev = apu_create_pdev(
> +				"gpio-keys-polled",
> +				driver_data->apu_keys_pdata,
> +				sizeof(struct gpio_keys_platform_data));
> +		}
> +
> +		if (driver_data->apu_leds_pdata) {
> +			gpiod_add_lookup_table(&gpios_led_table);
> +			apu_leds_pdev = apu_create_pdev(
> +				"leds-gpio",
> +				driver_data->apu_leds_pdata,
> +				sizeof(struct gpio_led_platform_data));
> +		}
> +	}
>  
>  	return 0;
>  }
>  
>  static void __exit apu_board_exit(void)
>  {
> -	gpiod_remove_lookup_table(&gpios_led_table);
> -	gpiod_remove_lookup_table(&gpios_key_table);
> +	if (apu_keys_pdev) {
> +		gpiod_remove_lookup_table(&gpios_key_table);
> +		platform_device_unregister(apu_keys_pdev);
> +	}
> +	if (apu_leds_pdev) {
> +		gpiod_remove_lookup_table(&gpios_led_table);
> +		platform_device_unregister(apu_leds_pdev);
> +	}
>  
> -	platform_device_unregister(apu_keys_pdev);
> -	platform_device_unregister(apu_leds_pdev);
>  	platform_device_unregister(apu_gpio_pdev);
>  }
>  
> 

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@...ux.net -- +49-151-27565287

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ