Input: soc_button_array - fix MS Surface Pro 11 probe failure
Subsystems (3)
PATCH v2 1/1 Input: soc_button_array - fix MS Surface Pro 11 probe failure
On the MS Surface Pro 11 soc_button_array probing races with the GPIO driver probing. If soc_button_array wins the race then gpiod_get() returns EPROBE_DEFER, which should normally take care of retrying later, but the soc_button_array code deliberately ignores EPROBE_DEFER causing it to fail its probe() which causes the volume and power buttons to now work.
The ignoring of EPROBE_DEFER is there to deal with a problem specific to older Bay Trail (BYT) and Cherry Trail (CHT) tablets which often use this driver. Modify the error handling to only ignore EPROBE_DEFER on BYT and CHT platforms and propagate EPROBE_DEFER normally on other platforms.
---
Changes in v2:
- Also check for irq == -EPROBE_DEFER (Shashiko)
- Drop #ifdef X86-ified soc_intel_is_byt_or_cht() helper, linux/platform_data/x86/soc.h already contains non x86 stubs (Shashiko)
--- drivers/input/misc/soc_button_array.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/input/misc/soc_button_array.c +10 -2
@@ -16,6 +16,7 @@#include <linux/gpio/consumer.h> #include <linux/gpio_keys.h> #include <linux/gpio.h> +#include <linux/platform_data/x86/soc.h> #include <linux/platform_device.h> static bool use_low_level_irq;
@@ -191,8 +192,9 @@ soc_button_device_create(struct platform_device *pdev, error =soc_button_lookup_gpio (&pdev->dev, info->acpi_index, &gpio, &irq);if (error || irq <0 ) {/* -* Skip GPIO if not present. Note we deliberately -* ignore -EPROBE_DEFER errors here. On some devices +* Propagate -EPROBE_DEFER, skip button on other errors. +* +* -EPROBE_DEFER is ignored on Bay & Cherry Trail. Here * Intel is using so called virtual GPIOs which are not * GPIOs at all but some way for AML code to check some * random status bits without need a custom opregion.
@@ -201,6 +203,12 @@ soc_button_device_create(struct platform_device *pdev,* we do not have a driver for these so they will never * show up, therefore we ignore -EPROBE_DEFER. */ +if ((error == -EPROBE_DEFER || irq == -EPROBE_DEFER ) && + !(soc_intel_is_byt () ||soc_intel_is_cht ())) { + error = -EPROBE_DEFER ; + goto err_free_mem; + } +continue ; }
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The local variable irq is evaluated without being initialized when soc_button_lookup_gpio() returns an error other than -EPROBE_DEFER (e.g., -ENOENT).
Pre-existing issues:
- [High] Missing bounds check on btns_desc->package.count before accessing the first element of the ACPI package. --
commit 71e6e56f2553b9410815c77e8b20ed9b2b9a2d37
Author: Hans de Goede <[email protected]>
Input: soc_button_array - fix MS Surface Pro 11 probe failure
This commit alters the error handling in soc_button_device_create() to propagate EPROBE_DEFER on platforms other than older Bay Trail and Cherry Trail tablets. This allows the MS Surface Pro 11 to correctly retry probing when racing with the GPIO driver.
Quoted 4 lines
drivers/input/misc/soc_button_array.c +0
-0
[ ... ]
Quoted 13 lines
drivers/input/misc/soc_button_array.c +3
-2@@ -191,8 +192,9 @@ soc_button_device_create(struct platform_device *pdev, error =soc_button_lookup_gpio (&pdev->dev, info->acpi_index, &gpio, &irq);if (error || irq <0 ) {/* -* Skip GPIO if not present. Note we deliberately -* ignore -EPROBE_DEFER errors here. On some devices +* Propagate -EPROBE_DEFER, skip button on other errors. +* +* -EPROBE_DEFER is ignored on Bay & Cherry Trail. Here * Intel is using so called virtual GPIOs which are not * GPIOs at all but some way for AML code to check some * random status bits without need a custom opregion.
drivers/input/misc/soc_button_array.c soc_button_device_create(struct platform_device *pdev, · line 206
Quoted 4 lines
drivers/input/misc/soc_button_array.c +1
-0@@ -201,6 +203,12 @@ soc_button_device_create(struct platform_device *pdev,* we do not have a driver for these so they will never * show up, therefore we ignore -EPROBE_DEFER. */ + if ((error == -EPROBE_DEFER || irq == -EPROBE_DEFER) &&
[Severity: High]
Can this evaluate the uninitialized local variable irq?
When soc_button_lookup_gpio() encounters an error other than -EPROBE_DEFER, it returns early without setting irq_ret:
soc_button_lookup_gpio()
desc = gpiod_get_index(dev, NULL, acpi_index, GPIOD_ASIS);
if (IS_ERR(desc))
return PTR_ERR(desc);If error is -ENOENT, the short-circuiting of the || operator means the condition evaluates irq == -EPROBE_DEFER while irq is uninitialized stack memory.
This will trigger KMSAN warnings, and if the uninitialized memory happens to contain -517 (-EPROBE_DEFER), it would falsely abort the entire probe instead of just skipping the missing button.
drivers/input/misc/soc_button_array.c soc_button_device_create(struct platform_device *pdev, · line 213
Quoted 7 lines
drivers/input/misc/soc_button_array.c +5
-0+ !(soc_intel_is_byt() || soc_intel_is_cht())) { + error = -EPROBE_DEFER ; + goto err_free_mem; + } +continue ; }
This is a pre-existing issue, but does soc_button_get_button_info() lack a bounds check on the ACPI package element count?
soc_button_get_button_info()
/* The first package describes the collection */
el0 = &btns_desc->package.elements[0];
if (el0->type == ACPI_TYPE_PACKAGE &&If the system provides a malformed ACPI table with an empty package where btns_desc->package.count is 0, this will access out-of-bounds memory or dereference a NULL pointer, potentially causing a panic during boot.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1