[PATCH v9 0/2] i2c: designware: fix WACF2200 touchscreen probe ordering

[PATCH v9 0/2] i2c: designware: fix WACF2200 touchscreen probe ordering

[Hardik Prakash]

Patch 1 reverts the broken v8 patch 2/2 which caused boot regressions
(NULL pointer dereference and probe deferral loop leading to CPU
starvation). Patch 2 is a corrected resubmission addressing all issues.

Changes from v8:
 - Use acpi_gpio_get_irq_resource() instead of open-coding GPIO resource
   type checks, eliminating duplication with gpiolib-acpi (Andy Shevchenko)
 - Remove gpio_dep_ctx wrapper struct, pass list_head * directly (Andy)
 - Add const to gpio_controller_ref.path (Andy)
 - Add NULL check for resource_source.string_ptr to fix crash on hardware
   where GPIO resources have no named controller (Nathan Chancellor,
   Chaitanya Kumar Borah)
 - Use acpi_dev_get_resources() return value properly (Andy)
 - Fix all error paths to call free_gpio_controller_list() (Andy)
 - Change guard to #if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
 - Remove erroneous put_device() calls -- acpi_get_first_physical_node()
   returns a borrowed pointer with no refcount increment
 - Use LIST_HEAD() macro, split adev declaration and assignment (Andy)
 - scoped_guard single statement without braces (Andy)
 - Remove misused Reported-by/Closes tags (Andy)

Tested on Lenovo Yoga 7 14AGP11 (83TD), Fedora 44, kernel 7.1.0-rc5+.
Touch and stylus fully functional.

Kernel bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=221494

Hardik Prakash (2):
  Revert "i2c: designware: defer probe if child GpioInt controllers are
    not bound"
  i2c: designware: defer probe if child GpioInt controllers are not bound

 drivers/i2c/busses/i2c-designware-platdrv.c | 133 ++++++++++++++++++++
 1 file changed, 133 insertions(+)

base-commit: ef76a3a28c79b628890431aa344af633e892035b

[PATCH v9 1/2] Revert "i2c: designware: defer probe if child GpioInt controllers are not bound"

[Hardik Prakash]

This reverts commit ef76a3a28c79b628890431aa344af633e892035b.

The patch causes boot regressions on multiple machines. A NULL pointer
dereference occurs when agpio->resource_source.string_ptr is NULL (i.e.
when string_length is 0), and a probe deferral loop causes CPU starvation
leading to kernel panic on Intel CI machines.

The patch needs a proper rewrite addressing these issues before resubmission.

Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
Signed-off-by: Hardik Prakash <hardikprakash.official@gmail.com>
Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Tested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://lore.kernel.org/linux-i2c/90656be5-eca0-4a09-9b19-0c6e85f1d455@intel.com/
Closes: https://lore.kernel.org/20260602185339.GA404948@ax162/
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 156 --------------------
 1 file changed, 156 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 1c01b0460385..3351c4a9ef11 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -8,8 +8,6 @@
  * Copyright (C) 2007 MontaVista Software Inc.
  * Copyright (C) 2009 Provigent Ltd.
  */
-
-#include <linux/acpi.h>
 #include <linux/clk-provider.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -132,152 +130,6 @@ static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
 	return 0;
 }
 
-#ifdef CONFIG_ACPI
-struct gpio_dep_ctx {
-	struct list_head gpio_controllers;
-	int ret;
-};
-
-struct gpio_controller_ref {
-	struct list_head node;
-	char *path;
-};
-
-static int check_gpioint_resource(struct acpi_resource *ares, void *data)
-{
-	struct gpio_dep_ctx *ctx = data;
-	struct acpi_resource_gpio *agpio;
-	struct gpio_controller_ref *ref, *tmp;
-	bool found = false;
-
-	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
-		return 1;
-
-	agpio = &ares->data.gpio;
-	if (agpio->connection_type != ACPI_RESOURCE_GPIO_TYPE_INT)
-		return 1;
-
-	/* Check if we've already tracked this GPIO controller */
-	list_for_each_entry(tmp, &ctx->gpio_controllers, node) {
-		if (!strcmp(tmp->path, agpio->resource_source.string_ptr)) {
-			found = true;
-			break;
-		}
-	}
-
-	if (!found) {
-		ref = kzalloc(sizeof(*ref), GFP_KERNEL);
-		if (!ref) {
-			ctx->ret = -ENOMEM;
-			return 0;
-		}
-
-		ref->path = kstrdup(agpio->resource_source.string_ptr, GFP_KERNEL);
-		if (!ref->path) {
-			kfree(ref);
-			ctx->ret = -ENOMEM;
-			return 0;
-		}
-
-		list_add_tail(&ref->node, &ctx->gpio_controllers);
-	}
-
-	return 1;
-}
-
-static int check_child_gpioint(struct acpi_device *adev, void *data)
-{
-	struct gpio_dep_ctx *ctx = data;
-	struct list_head res_list;
-
-	INIT_LIST_HEAD(&res_list);
-
-	acpi_dev_get_resources(adev, &res_list, check_gpioint_resource, ctx);
-	acpi_dev_free_resource_list(&res_list);
-
-	if (ctx->ret < 0)
-		return ctx->ret;
-
-	return 0;
-}
-
-static int i2c_dw_check_gpio_dependencies(struct device *dev)
-{
-	struct acpi_device *adev = ACPI_COMPANION(dev);
-	struct gpio_dep_ctx ctx = { .ret = 0 };
-	struct gpio_controller_ref *ref, *tmp;
-	int ret = 0;
-
-	if (!adev)
-		return 0;
-
-	INIT_LIST_HEAD(&ctx.gpio_controllers);
-
-	/* Walk all child devices and collect GpioInt controller references */
-	ret = acpi_dev_for_each_child(adev, check_child_gpioint, &ctx);
-	if (ret < 0 || ctx.ret < 0) {
-		ret = ctx.ret ?: ret;
-		goto cleanup;
-	}
-
-	/* For each GPIO controller, check if its parent device is bound */
-	list_for_each_entry(ref, &ctx.gpio_controllers, node) {
-		acpi_handle handle;
-		acpi_status status;
-		struct acpi_device *gpio_adev;
-		struct device *gpio_dev;
-		bool bound;
-
-		status = acpi_get_handle(NULL, ref->path, &handle);
-		if (ACPI_FAILURE(status))
-			continue;
-
-		gpio_adev = acpi_fetch_acpi_dev(handle);
-		if (!gpio_adev)
-			continue;
-
-		gpio_dev = acpi_get_first_physical_node(gpio_adev);
-		acpi_dev_put(gpio_adev);
-
-		if (!gpio_dev) {
-			ret = -EPROBE_DEFER;
-			goto cleanup;
-		}
-
-		/*
-		 * Check if the GPIO controller's device is bound. If not,
-		 * defer probe to ensure GPIO initialization (including IRQ
-		 * setup and quirks) is complete before we enumerate I2C
-		 * child devices.
-		 */
-		scoped_guard(device, gpio_dev) {
-			bound = device_is_bound(gpio_dev);
-		}
-		if (!bound) {
-			put_device(gpio_dev);
-			ret = -EPROBE_DEFER;
-			goto cleanup;
-		}
-
-		put_device(gpio_dev);
-	}
-
-cleanup:
-	list_for_each_entry_safe(ref, tmp, &ctx.gpio_controllers, node) {
-		list_del(&ref->node);
-		kfree(ref->path);
-		kfree(ref);
-	}
-
-	return ret;
-}
-#else
-static int i2c_dw_check_gpio_dependencies(struct device *dev)
-{
-	return 0;
-}
-#endif /* CONFIG_ACPI */
-
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
@@ -286,14 +138,6 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	struct dw_i2c_dev *dev;
 	int irq, ret;
 
-	/*
-	 * Check if any child devices have GpioInt resources, and if so,
-	 * defer probe until those GPIO controllers are fully bound.
-	 */
-	ret = i2c_dw_check_gpio_dependencies(device);
-	if (ret)
-		return ret;
-
 	irq = platform_get_irq_optional(pdev, 0);
 	if (irq == -ENXIO)
 		flags |= ACCESS_POLLING;
-- 
2.54.0

[PATCH v9 2/2] i2c: designware: defer probe if child GpioInt controllers are not bound

[Hardik Prakash]

I2C controllers may have child devices with GpioInt resources that
depend on GPIO controllers to be fully initialized. If the I2C
controller probes and enumerates children before the referenced GPIO
controller has completed probe, GPIO interrupts may not be properly
configured, leading to device failures.

On Lenovo Yoga 7 14AGP11, the WACF2200 touchscreen (child of
AMDI0010:02) has a GpioInt resource pointing to GPIO 157 on the
pinctrl-amd controller (AMDI0030:00). When i2c-designware probes
AMDI0010:02 before pinctrl-amd finishes initializing, I2C transactions
fail with lost arbitration errors.

Add a generic dependency check in i2c-designware that walks ACPI child
devices, identifies any GpioInt resources, resolves the referenced GPIO
controllers, and defers probe if those controllers are not yet bound.
Uses acpi_gpio_get_irq_resource() to avoid duplicating GPIO resource
parsing logic from gpiolib-acpi. Skips resources with no resource
source string (string_length == 0 or string_ptr == NULL) to avoid
crashes on hardware where GPIO resources have no named controller.

The probe ordering race was confirmed via dynamic debug tracing:

  0.285952  amd_gpio_probe: registering gpiochip  <- GPIO chip visible
  0.287121  amd_gpio_probe: requesting parent IRQ <- probe still running
  0.301454  AMDI0010:02 dw_i2c_plat_probe: start  <- races here
  2.348157  lost arbitration

Fixes: 3812a9e84265 ("pinctrl-amd: enable IRQ for WACF2200 touchscreen on Lenovo Yoga 7 14AGP11")
Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Hardik Prakash <hardikprakash.official@gmail.com>
Assisted-by: Claude:claude-sonnet-4-6
Assisted-by: GPT:gpt-5.4-mini
Link: https://bugzilla.kernel.org/show_bug.cgi?id=221494
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 133 ++++++++++++++++++++
 1 file changed, 133 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 3351c4a9ef11..51172fffa2b8 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -8,6 +8,8 @@
  * Copyright (C) 2007 MontaVista Software Inc.
  * Copyright (C) 2009 Provigent Ltd.
  */
+
+#include <linux/acpi.h>
 #include <linux/clk-provider.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -130,6 +132,133 @@ static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
 	return 0;
 }
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
+struct gpio_controller_ref {
+	struct list_head node;
+	const char *path;
+};
+
+static void free_gpio_controller_list(struct list_head *gpio_controllers)
+{
+	struct gpio_controller_ref *ref, *tmp;
+
+	list_for_each_entry_safe(ref, tmp, gpio_controllers, node) {
+		list_del(&ref->node);
+		kfree(ref->path);
+		kfree(ref);
+	}
+}
+
+static int check_gpioint_resource(struct acpi_resource *ares, void *data)
+{
+	struct list_head *gpio_controllers = data;
+	struct acpi_resource_gpio *agpio;
+	struct gpio_controller_ref *ref;
+
+	if (!acpi_gpio_get_irq_resource(ares, &agpio))
+		return 1;
+
+	if (!agpio->resource_source.string_length ||
+	    !agpio->resource_source.string_ptr)
+		return 1;
+
+	/* Skip if we've already tracked this GPIO controller */
+	list_for_each_entry(ref, gpio_controllers, node) {
+		if (!strcmp(ref->path, agpio->resource_source.string_ptr))
+			return 1;
+	}
+
+	ref = kzalloc(sizeof(*ref), GFP_KERNEL);
+	if (!ref)
+		return -ENOMEM;
+
+	ref->path = kstrdup(agpio->resource_source.string_ptr, GFP_KERNEL);
+	if (!ref->path) {
+		kfree(ref);
+		return -ENOMEM;
+	}
+
+	list_add_tail(&ref->node, gpio_controllers);
+	return 1;
+}
+
+static int check_child_gpioint(struct acpi_device *adev, void *data)
+{
+	struct list_head res_list;
+	int ret;
+
+	INIT_LIST_HEAD(&res_list);
+	ret = acpi_dev_get_resources(adev, &res_list,
+				     check_gpioint_resource, data);
+	acpi_dev_free_resource_list(&res_list);
+	return ret < 0 ? ret : 0;
+}
+
+static int i2c_dw_check_gpio_dependencies(struct device *dev)
+{
+	struct acpi_device *adev;
+	LIST_HEAD(gpio_controllers);
+	struct gpio_controller_ref *ref;
+	int ret = 0;
+
+	adev = ACPI_COMPANION(dev);
+	if (!adev)
+		return 0;
+
+	/* Walk all child devices and collect GpioInt controller references */
+	ret = acpi_dev_for_each_child(adev, check_child_gpioint,
+				      &gpio_controllers);
+	if (ret < 0)
+		goto cleanup;
+
+	/* For each GPIO controller, check if its platform device is bound */
+	list_for_each_entry(ref, &gpio_controllers, node) {
+		acpi_handle handle;
+		acpi_status status;
+		struct acpi_device *gpio_adev;
+		struct device *gpio_dev;
+		bool bound;
+
+		status = acpi_get_handle(NULL, ref->path, &handle);
+		if (ACPI_FAILURE(status))
+			continue;
+
+		gpio_adev = acpi_fetch_acpi_dev(handle);
+		if (!gpio_adev)
+			continue;
+
+		gpio_dev = acpi_get_first_physical_node(gpio_adev);
+		acpi_dev_put(gpio_adev);
+		if (!gpio_dev) {
+			ret = -EPROBE_DEFER;
+			goto cleanup;
+		}
+
+		/*
+		 * Defer probe until the GPIO controller is fully bound,
+		 * ensuring its IRQ setup is complete before we enumerate
+		 * I2C child devices.
+		 */
+		scoped_guard(device, gpio_dev)
+			bound = device_is_bound(gpio_dev);
+
+		if (!bound) {
+			ret = -EPROBE_DEFER;
+			goto cleanup;
+		}
+	}
+
+cleanup:
+	free_gpio_controller_list(&gpio_controllers);
+	return ret;
+}
+#else
+static int i2c_dw_check_gpio_dependencies(struct device *dev)
+{
+	return 0;
+}
+#endif /* CONFIG_ACPI && CONFIG_GPIOLIB */
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
@@ -138,6 +267,10 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	struct dw_i2c_dev *dev;
 	int irq, ret;
 
+	ret = i2c_dw_check_gpio_dependencies(device);
+	if (ret)
+		return ret;
+
 	irq = platform_get_irq_optional(pdev, 0);
 	if (irq == -ENXIO)
 		flags |= ACCESS_POLLING;
-- 
2.54.0

Re: [PATCH v9 0/2] i2c: designware: fix WACF2200 touchscreen probe ordering

[Andy Shevchenko]

On Wed, Jun 17, 2026 at 12:29:20PM +0530, Hardik Prakash wrote:
> Patch 1 reverts the broken v8 patch 2/2 which caused boot regressions
> (NULL pointer dereference and probe deferral loop leading to CPU
> starvation). Patch 2 is a corrected resubmission addressing all issues.

When preparing patches, use existing tools. This series was half received by me
as the second patch has incomplete Cc list. You may use `b4` tool (check in
your Linux distro) or my "smart" script [1] which I use on a daily basis.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 0/2] i2c: designware: fix WACF2200 touchscreen probe ordering

[Andy Shevchenko]

On Wed, Jun 17, 2026 at 11:03:44AM +0300, Andy Shevchenko wrote:
> On Wed, Jun 17, 2026 at 12:29:20PM +0530, Hardik Prakash wrote:
> > Patch 1 reverts the broken v8 patch 2/2 which caused boot regressions
> > (NULL pointer dereference and probe deferral loop leading to CPU
> > starvation). Patch 2 is a corrected resubmission addressing all issues.
> 
> When preparing patches, use existing tools. This series was half received by me
> as the second patch has incomplete Cc list. You may use `b4` tool (check in
> your Linux distro) or my "smart" script [1] which I use on a daily basis.
> 
> [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

Ah, eventually got it. It was a glitch on my side, sorry for the noise.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 2/2] i2c: designware: defer probe if child GpioInt controllers are not bound

[Andy Shevchenko]

On Wed, Jun 17, 2026 at 12:29:22PM +0530, Hardik Prakash wrote:
> I2C controllers may have child devices with GpioInt resources that
> depend on GPIO controllers to be fully initialized. If the I2C
> controller probes and enumerates children before the referenced GPIO
> controller has completed probe, GPIO interrupts may not be properly
> configured, leading to device failures.
> 
> On Lenovo Yoga 7 14AGP11, the WACF2200 touchscreen (child of
> AMDI0010:02) has a GpioInt resource pointing to GPIO 157 on the
> pinctrl-amd controller (AMDI0030:00). When i2c-designware probes
> AMDI0010:02 before pinctrl-amd finishes initializing, I2C transactions
> fail with lost arbitration errors.
> 
> Add a generic dependency check in i2c-designware that walks ACPI child
> devices, identifies any GpioInt resources, resolves the referenced GPIO
> controllers, and defers probe if those controllers are not yet bound.
> Uses acpi_gpio_get_irq_resource() to avoid duplicating GPIO resource
> parsing logic from gpiolib-acpi. Skips resources with no resource
> source string (string_length == 0 or string_ptr == NULL) to avoid
> crashes on hardware where GPIO resources have no named controller.
> 
> The probe ordering race was confirmed via dynamic debug tracing:
> 
>   0.285952  amd_gpio_probe: registering gpiochip  <- GPIO chip visible
>   0.287121  amd_gpio_probe: requesting parent IRQ <- probe still running
>   0.301454  AMDI0010:02 dw_i2c_plat_probe: start  <- races here
>   2.348157  lost arbitration

...

> +static int check_gpioint_resource(struct acpi_resource *ares, void *data)
> +{
> +	struct list_head *gpio_controllers = data;
> +	struct acpi_resource_gpio *agpio;
> +	struct gpio_controller_ref *ref;
> +
> +	if (!acpi_gpio_get_irq_resource(ares, &agpio))
> +		return 1;

> +	if (!agpio->resource_source.string_length ||
> +	    !agpio->resource_source.string_ptr)
> +		return 1;

I'm wondering if we simply can move to strncmp() instead of this check

> +	/* Skip if we've already tracked this GPIO controller */
> +	list_for_each_entry(ref, gpio_controllers, node) {
> +		if (!strcmp(ref->path, agpio->resource_source.string_ptr))

		if (!strncmp(ref->path, agpio->resource_source.string_ptr))


> +			return 1;
> +	}
> +
> +	ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> +	if (!ref)
> +		return -ENOMEM;
> +
> +	ref->path = kstrdup(agpio->resource_source.string_ptr, GFP_KERNEL);
> +	if (!ref->path) {
> +		kfree(ref);
> +		return -ENOMEM;
> +	}
> +
> +	list_add_tail(&ref->node, gpio_controllers);
> +	return 1;
> +}
> +
> +static int check_child_gpioint(struct acpi_device *adev, void *data)
> +{
> +	struct list_head res_list;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&res_list);

> +	ret = acpi_dev_get_resources(adev, &res_list,
> +				     check_gpioint_resource, data);

Make it a single line.

> +	acpi_dev_free_resource_list(&res_list);

It's not critical double free (it will try to free an empty list) on error.

> +	return ret < 0 ? ret : 0;

	ret = acpi_dev_get_resources(adev, &res_list, check_gpioint_resource, data);
	if (ret < 0)
		return ret;

	acpi_dev_free_resource_list(&res_list);
	return 0;

> +}
> +
> +static int i2c_dw_check_gpio_dependencies(struct device *dev)
> +{
> +	struct acpi_device *adev;
> +	LIST_HEAD(gpio_controllers);
> +	struct gpio_controller_ref *ref;

Reversed xmas tree order.

> +	int ret = 0;

Useless assignment.

> +	adev = ACPI_COMPANION(dev);
> +	if (!adev)
> +		return 0;
> +
> +	/* Walk all child devices and collect GpioInt controller references */

> +	ret = acpi_dev_for_each_child(adev, check_child_gpioint,
> +				      &gpio_controllers);

Make it a single line.

> +	if (ret < 0)
> +		goto cleanup;
> +
> +	/* For each GPIO controller, check if its platform device is bound */
> +	list_for_each_entry(ref, &gpio_controllers, node) {
> +		acpi_handle handle;
> +		acpi_status status;
> +		struct acpi_device *gpio_adev;
> +		struct device *gpio_dev;

Reversed xmas tree order.

> +		bool bound;
> +
> +		status = acpi_get_handle(NULL, ref->path, &handle);
> +		if (ACPI_FAILURE(status))
> +			continue;
> +
> +		gpio_adev = acpi_fetch_acpi_dev(handle);
> +		if (!gpio_adev)
> +			continue;

> +		gpio_dev = acpi_get_first_physical_node(gpio_adev);
> +		acpi_dev_put(gpio_adev);
> +		if (!gpio_dev) {
> +			ret = -EPROBE_DEFER;
> +			goto cleanup;
> +		}

> +		/*
> +		 * Defer probe until the GPIO controller is fully bound,
> +		 * ensuring its IRQ setup is complete before we enumerate
> +		 * I2C child devices.
> +		 */
> +		scoped_guard(device, gpio_dev)
> +			bound = device_is_bound(gpio_dev);

> +		if (!bound) {

Some of the compilers might complain the use of uninitialised variable (they
might not parse properly scoped_guard() case).

> +			ret = -EPROBE_DEFER;
> +			goto cleanup;
> +		}

To make it sure and deduplicate above the whole stuff can be written as

		gpio_dev = acpi_get_first_physical_node(gpio_adev);
		acpi_dev_put(gpio_adev);
		if (gpio_dev) {
			guard(device)(gpio_dev);

			bound = device_is_bound(gpio_dev);
		} else {
			bound = false;
		}
		/*
		 * Defer probe until the GPIO controller is fully bound,
		 * ensuring its IRQ setup is complete before we enumerate
		 * I2C child devices.
		 */
		if (!bound) {
			ret = -EPROBE_DEFER;
			goto cleanup;
		}

> +	}
> +
> +cleanup:
> +	free_gpio_controller_list(&gpio_controllers);
> +	return ret;
> +}
> +#else
> +static int i2c_dw_check_gpio_dependencies(struct device *dev)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_ACPI && CONFIG_GPIOLIB */

I'm not sure if it's good to have all this quirk here or simply start
a i2c-designware-quirks.c. Theoretically the PCI counterpart might,
but I think quite unlikely, want to have something similar in the future.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 2/2] i2c: designware: defer probe if child GpioInt controllers are not bound

[Hardik Prakash]

On Wed, Jun 17, 2026 at 14:57, Andy Shevchenko wrote:
> > +     if (!agpio->resource_source.string_length ||
> > +         !agpio->resource_source.string_ptr)
> > +             return 1;
>
> I'm wondering if we simply can move to strncmp() instead of this check
>
> > +     list_for_each_entry(ref, gpio_controllers, node) {
> > +             if (!strcmp(ref->path, agpio->resource_source.string_ptr))
>
>                 if (!strncmp(ref->path, agpio->resource_source.string_ptr))

Could you clarify? strncmp() with n=string_length would protect the
dedup check against a NULL or unterminated string_ptr, but we still
need string_ptr to be non-NULL before passing it to kstrdup(). Should
we keep a NULL/zero-length guard before kstrdup() and only replace the
strcmp() in the dedup loop with strncmp()?

Thanks,
Hardik

On Wed, 17 Jun 2026 at 14:57, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Jun 17, 2026 at 12:29:22PM +0530, Hardik Prakash wrote:
> > I2C controllers may have child devices with GpioInt resources that
> > depend on GPIO controllers to be fully initialized. If the I2C
> > controller probes and enumerates children before the referenced GPIO
> > controller has completed probe, GPIO interrupts may not be properly
> > configured, leading to device failures.
> >
> > On Lenovo Yoga 7 14AGP11, the WACF2200 touchscreen (child of
> > AMDI0010:02) has a GpioInt resource pointing to GPIO 157 on the
> > pinctrl-amd controller (AMDI0030:00). When i2c-designware probes
> > AMDI0010:02 before pinctrl-amd finishes initializing, I2C transactions
> > fail with lost arbitration errors.
> >
> > Add a generic dependency check in i2c-designware that walks ACPI child
> > devices, identifies any GpioInt resources, resolves the referenced GPIO
> > controllers, and defers probe if those controllers are not yet bound.
> > Uses acpi_gpio_get_irq_resource() to avoid duplicating GPIO resource
> > parsing logic from gpiolib-acpi. Skips resources with no resource
> > source string (string_length == 0 or string_ptr == NULL) to avoid
> > crashes on hardware where GPIO resources have no named controller.
> >
> > The probe ordering race was confirmed via dynamic debug tracing:
> >
> >   0.285952  amd_gpio_probe: registering gpiochip  <- GPIO chip visible
> >   0.287121  amd_gpio_probe: requesting parent IRQ <- probe still running
> >   0.301454  AMDI0010:02 dw_i2c_plat_probe: start  <- races here
> >   2.348157  lost arbitration
>
> ...
>
> > +static int check_gpioint_resource(struct acpi_resource *ares, void *data)
> > +{
> > +     struct list_head *gpio_controllers = data;
> > +     struct acpi_resource_gpio *agpio;
> > +     struct gpio_controller_ref *ref;
> > +
> > +     if (!acpi_gpio_get_irq_resource(ares, &agpio))
> > +             return 1;
>
> > +     if (!agpio->resource_source.string_length ||
> > +         !agpio->resource_source.string_ptr)
> > +             return 1;
>
> I'm wondering if we simply can move to strncmp() instead of this check
>
> > +     /* Skip if we've already tracked this GPIO controller */
> > +     list_for_each_entry(ref, gpio_controllers, node) {
> > +             if (!strcmp(ref->path, agpio->resource_source.string_ptr))
>
>                 if (!strncmp(ref->path, agpio->resource_source.string_ptr))
>
>
> > +                     return 1;
> > +     }
> > +
> > +     ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> > +     if (!ref)
> > +             return -ENOMEM;
> > +
> > +     ref->path = kstrdup(agpio->resource_source.string_ptr, GFP_KERNEL);
> > +     if (!ref->path) {
> > +             kfree(ref);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     list_add_tail(&ref->node, gpio_controllers);
> > +     return 1;
> > +}
> > +
> > +static int check_child_gpioint(struct acpi_device *adev, void *data)
> > +{
> > +     struct list_head res_list;
> > +     int ret;
> > +
> > +     INIT_LIST_HEAD(&res_list);
>
> > +     ret = acpi_dev_get_resources(adev, &res_list,
> > +                                  check_gpioint_resource, data);
>
> Make it a single line.
>
> > +     acpi_dev_free_resource_list(&res_list);
>
> It's not critical double free (it will try to free an empty list) on error.
>
> > +     return ret < 0 ? ret : 0;
>
>         ret = acpi_dev_get_resources(adev, &res_list, check_gpioint_resource, data);
>         if (ret < 0)
>                 return ret;
>
>         acpi_dev_free_resource_list(&res_list);
>         return 0;
>
> > +}
> > +
> > +static int i2c_dw_check_gpio_dependencies(struct device *dev)
> > +{
> > +     struct acpi_device *adev;
> > +     LIST_HEAD(gpio_controllers);
> > +     struct gpio_controller_ref *ref;
>
> Reversed xmas tree order.
>
> > +     int ret = 0;
>
> Useless assignment.
>
> > +     adev = ACPI_COMPANION(dev);
> > +     if (!adev)
> > +             return 0;
> > +
> > +     /* Walk all child devices and collect GpioInt controller references */
>
> > +     ret = acpi_dev_for_each_child(adev, check_child_gpioint,
> > +                                   &gpio_controllers);
>
> Make it a single line.
>
> > +     if (ret < 0)
> > +             goto cleanup;
> > +
> > +     /* For each GPIO controller, check if its platform device is bound */
> > +     list_for_each_entry(ref, &gpio_controllers, node) {
> > +             acpi_handle handle;
> > +             acpi_status status;
> > +             struct acpi_device *gpio_adev;
> > +             struct device *gpio_dev;
>
> Reversed xmas tree order.
>
> > +             bool bound;
> > +
> > +             status = acpi_get_handle(NULL, ref->path, &handle);
> > +             if (ACPI_FAILURE(status))
> > +                     continue;
> > +
> > +             gpio_adev = acpi_fetch_acpi_dev(handle);
> > +             if (!gpio_adev)
> > +                     continue;
>
> > +             gpio_dev = acpi_get_first_physical_node(gpio_adev);
> > +             acpi_dev_put(gpio_adev);
> > +             if (!gpio_dev) {
> > +                     ret = -EPROBE_DEFER;
> > +                     goto cleanup;
> > +             }
>
> > +             /*
> > +              * Defer probe until the GPIO controller is fully bound,
> > +              * ensuring its IRQ setup is complete before we enumerate
> > +              * I2C child devices.
> > +              */
> > +             scoped_guard(device, gpio_dev)
> > +                     bound = device_is_bound(gpio_dev);
>
> > +             if (!bound) {
>
> Some of the compilers might complain the use of uninitialised variable (they
> might not parse properly scoped_guard() case).
>
> > +                     ret = -EPROBE_DEFER;
> > +                     goto cleanup;
> > +             }
>
> To make it sure and deduplicate above the whole stuff can be written as
>
>                 gpio_dev = acpi_get_first_physical_node(gpio_adev);
>                 acpi_dev_put(gpio_adev);
>                 if (gpio_dev) {
>                         guard(device)(gpio_dev);
>
>                         bound = device_is_bound(gpio_dev);
>                 } else {
>                         bound = false;
>                 }
>                 /*
>                  * Defer probe until the GPIO controller is fully bound,
>                  * ensuring its IRQ setup is complete before we enumerate
>                  * I2C child devices.
>                  */
>                 if (!bound) {
>                         ret = -EPROBE_DEFER;
>                         goto cleanup;
>                 }
>
> > +     }
> > +
> > +cleanup:
> > +     free_gpio_controller_list(&gpio_controllers);
> > +     return ret;
> > +}
> > +#else
> > +static int i2c_dw_check_gpio_dependencies(struct device *dev)
> > +{
> > +     return 0;
> > +}
> > +#endif /* CONFIG_ACPI && CONFIG_GPIOLIB */
>
> I'm not sure if it's good to have all this quirk here or simply start
> a i2c-designware-quirks.c. Theoretically the PCI counterpart might,
> but I think quite unlikely, want to have something similar in the future.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v9 2/2] i2c: designware: defer probe if child GpioInt controllers are not bound

[Andy Shevchenko]

On Thu, Jun 18, 2026 at 12:52:46AM +0530, Hardik Prakash wrote:
> On Wed, Jun 17, 2026 at 14:57, Andy Shevchenko wrote:

> > > +     if (!agpio->resource_source.string_length ||
> > > +         !agpio->resource_source.string_ptr)
> > > +             return 1;
> >
> > I'm wondering if we simply can move to strncmp() instead of this check
> >
> > > +     list_for_each_entry(ref, gpio_controllers, node) {
> > > +             if (!strcmp(ref->path, agpio->resource_source.string_ptr))
> >
> >                 if (!strncmp(ref->path, agpio->resource_source.string_ptr))
> 
> Could you clarify? strncmp() with n=string_length would protect the
> dedup check against a NULL or unterminated string_ptr, but we still
> need string_ptr to be non-NULL before passing it to kstrdup(). Should
> we keep a NULL/zero-length guard before kstrdup() and only replace the
> strcmp() in the dedup loop with strncmp()?

Ah, okay, you are talking about the first iteration when the list is empty and
we have to add it to the list.

So the question is, do we expect the resource_source not to be set at this point?
In other words is there any valid AML that interpreter decodes to the empty
resource_source? If so, can we ever have the following condition to be true?

	string_length != 0 && string_ptr == NULL

P.S. Do not top-post! Reply under the piece in question. Also remove
the context you are not replying to.

> On Wed, 17 Jun 2026 at 14:57, Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Jun 17, 2026 at 12:29:22PM +0530, Hardik Prakash wrote:

...

> > > +static int check_gpioint_resource(struct acpi_resource *ares, void *data)
> > > +{
> > > +     struct list_head *gpio_controllers = data;
> > > +     struct acpi_resource_gpio *agpio;
> > > +     struct gpio_controller_ref *ref;
> > > +
> > > +     if (!acpi_gpio_get_irq_resource(ares, &agpio))
> > > +             return 1;
> >
> > > +     if (!agpio->resource_source.string_length ||
> > > +         !agpio->resource_source.string_ptr)
> > > +             return 1;
> >
> > I'm wondering if we simply can move to strncmp() instead of this check
> >
> > > +     /* Skip if we've already tracked this GPIO controller */
> > > +     list_for_each_entry(ref, gpio_controllers, node) {
> > > +             if (!strcmp(ref->path, agpio->resource_source.string_ptr))
> >
> >                 if (!strncmp(ref->path, agpio->resource_source.string_ptr))
> >
> >
> > > +                     return 1;
> > > +     }
> > > +
> > > +     ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> > > +     if (!ref)
> > > +             return -ENOMEM;
> > > +
> > > +     ref->path = kstrdup(agpio->resource_source.string_ptr, GFP_KERNEL);
> > > +     if (!ref->path) {
> > > +             kfree(ref);
> > > +             return -ENOMEM;
> > > +     }
> > > +
> > > +     list_add_tail(&ref->node, gpio_controllers);
> > > +     return 1;
> > > +}

You haven't replied to the rest, I assume you agree with all the suggestions?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 2/2] i2c: designware: defer probe if child GpioInt controllers are not bound

[Hardik Prakash]

On Thu, Jun 18, 2026 at 12:25, Andy Shevchenko wrote:
> So the question is, do we expect the resource_source not to be set at
> this point? In other words is there any valid AML that interpreter
> decodes to the empty resource_source? If so, can we ever have the
> following condition to be true?
>
>         string_length != 0 && string_ptr == NULL

I checked the ACPICA parser in drivers/acpi/acpica/rsutils.c. The
acpi_rs_get_resource_source() helper always assigns string_ptr before
string_length when the resource source is present. When it is absent,
both are cleared together (string_length = 0, string_ptr = NULL). So
string_length != 0 && string_ptr == NULL cannot be produced by normal
AML parsing.

Therefore checking string_length alone is sufficient:

        if (!agpio->resource_source.string_length)
                return 1;

I'll simplify to this in v10.

> You haven't replied to the rest, I assume you agree with all the
> suggestions?

Yes, I agree with all the remaining suggestions and will address them
in v10:
 - acpi_dev_get_resources() on single line, free only on success path
 - Reversed xmas tree ordering for variable declarations
 - Remove useless int ret = 0 assignment
 - acpi_dev_for_each_child() on single line
 - Use guard(device)(gpio_dev) pattern to avoid uninitialized variable
   warning and deduplicate the !gpio_dev check

Thanks,
Hardik

On Thu, 18 Jun 2026 at 12:25, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Jun 18, 2026 at 12:52:46AM +0530, Hardik Prakash wrote:
> > On Wed, Jun 17, 2026 at 14:57, Andy Shevchenko wrote:
>
> > > > +     if (!agpio->resource_source.string_length ||
> > > > +         !agpio->resource_source.string_ptr)
> > > > +             return 1;
> > >
> > > I'm wondering if we simply can move to strncmp() instead of this check
> > >
> > > > +     list_for_each_entry(ref, gpio_controllers, node) {
> > > > +             if (!strcmp(ref->path, agpio->resource_source.string_ptr))
> > >
> > >                 if (!strncmp(ref->path, agpio->resource_source.string_ptr))
> >
> > Could you clarify? strncmp() with n=string_length would protect the
> > dedup check against a NULL or unterminated string_ptr, but we still
> > need string_ptr to be non-NULL before passing it to kstrdup(). Should
> > we keep a NULL/zero-length guard before kstrdup() and only replace the
> > strcmp() in the dedup loop with strncmp()?
>
> Ah, okay, you are talking about the first iteration when the list is empty and
> we have to add it to the list.
>
> So the question is, do we expect the resource_source not to be set at this point?
> In other words is there any valid AML that interpreter decodes to the empty
> resource_source? If so, can we ever have the following condition to be true?
>
>         string_length != 0 && string_ptr == NULL
>
> P.S. Do not top-post! Reply under the piece in question. Also remove
> the context you are not replying to.
>
> > On Wed, 17 Jun 2026 at 14:57, Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Wed, Jun 17, 2026 at 12:29:22PM +0530, Hardik Prakash wrote:
>
> ...
>
> > > > +static int check_gpioint_resource(struct acpi_resource *ares, void *data)
> > > > +{
> > > > +     struct list_head *gpio_controllers = data;
> > > > +     struct acpi_resource_gpio *agpio;
> > > > +     struct gpio_controller_ref *ref;
> > > > +
> > > > +     if (!acpi_gpio_get_irq_resource(ares, &agpio))
> > > > +             return 1;
> > >
> > > > +     if (!agpio->resource_source.string_length ||
> > > > +         !agpio->resource_source.string_ptr)
> > > > +             return 1;
> > >
> > > I'm wondering if we simply can move to strncmp() instead of this check
> > >
> > > > +     /* Skip if we've already tracked this GPIO controller */
> > > > +     list_for_each_entry(ref, gpio_controllers, node) {
> > > > +             if (!strcmp(ref->path, agpio->resource_source.string_ptr))
> > >
> > >                 if (!strncmp(ref->path, agpio->resource_source.string_ptr))
> > >
> > >
> > > > +                     return 1;
> > > > +     }
> > > > +
> > > > +     ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> > > > +     if (!ref)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     ref->path = kstrdup(agpio->resource_source.string_ptr, GFP_KERNEL);
> > > > +     if (!ref->path) {
> > > > +             kfree(ref);
> > > > +             return -ENOMEM;
> > > > +     }
> > > > +
> > > > +     list_add_tail(&ref->node, gpio_controllers);
> > > > +     return 1;
> > > > +}
>
> You haven't replied to the rest, I assume you agree with all the suggestions?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>