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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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