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

On Wed, 15 Jul 2026, Andy Shevchenko wrote:
> Instead just define with LIST_HEAD() macro.
>
>         LIST_HEAD(res_list);
>         int ret;
> [...]
> Check for errors when it's required. I believe you fixed it at some point
> and now it's again old version. Please, reduce full rely on AI, it's not
> helpful sometimes.

My bad, I checked and found the correct snippet in v10:

        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;

I will post v12 with this and your other comments addressed.

Thanks,
Hardik

On Thu, 16 Jul 2026 at 10:59, Hardik Prakash
<[email protected]> wrote:
>
> On Wed, 15 Jul 2026, Bartosz Golaszewski wrote:
> > Please include the entire changelog in every new iteration. I believe I gave
> > my Ack for this and now it's gone. I have no idea why because there's neither
> > a complete changelog nor links to previous versions.
>
> Sorry about that, I will include the full changelog history from now on.
>
> Here's a catch-up: since your v6, v7 Ack, the patch went through the v8
> regression (NULL ptr deref + probe deferral loop), a v9 revert+fix
> split, and now v11's restructure around gpio_device_find_by_fwnode()
> per Andy's suggestion. The core approach (generic GpioInt dependency
> check) is the same one you acked at v6.
>
> However, given the amount that's changed since v6, I wonder if you
> might want to take another look before re-acking.
>
> Thanks,
> Hardik
>
> On Wed, 15 Jul 2026 at 19:47, Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Wed, Jul 15, 2026 at 05:17:01PM +0530, Hardik Prakash wrote:
> > > I2C controllers may have child devices with GpioInt resources that
> > > depend on GPIO controllers being 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:
> > >
> > >   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
> > >
> > > Add a dependency check that walks ACPI child devices and defers probe
> > > until any referenced GPIO controller is bound.
> >
> > ...
> >
> > > v10 -> v11:
> > > - Replaced custom gpio_controller_ref list with gpio_device_find_by_fwnode(),
> > >   as suggested by Andy, dropping the linked list and dedup logic (~60 lines)
> > > - Moved resource-skip explanation from commit message into a code comment
> > > - Fixed device_is_bound() to check gpio_device_to_device(gdev)->parent
> > >   rather than the gpio_device's own internal class device, which never
> > >   has a driver bound to it
> >
> > Thanks for an update, looks much better!
> >
> > My comments below. I think v12 will be final if you address everything as
> > suggested and answer to the Bart's request.
> >
> > ...
> >
> > > +static int check_gpioint_resource(struct acpi_resource *ares, void *data)
> > > +{
> >
> > > +     struct gpio_device *gdev __free(gpio_device_put) = NULL;
> >
> > This style is discouraged. Please, move this below.
> >
> > > +     struct acpi_resource_gpio *agpio;
> > > +     struct acpi_device *gpio_adev;
> > > +     struct device *gpio_dev;
> > > +     acpi_handle handle;
> > > +
> > > +     if (!acpi_gpio_get_irq_resource(ares, &agpio))
> > > +             return 1; /* not a GpioInt resource, skip */
> > > +
> > > +     if (!agpio->resource_source.string_length)
> > > +             return 1; /* no named controller, skip */
> >
> > > +     if (ACPI_FAILURE(acpi_get_handle(NULL, agpio->resource_source.string_ptr, &handle)))
> > > +             return 1;
> >
> > Can be rewritten as
> >
> >         acpi_status status;
> >         ...
> >
> >         status = acpi_get_handle(NULL, agpio->resource_source.string_ptr, &handle);
> >         if (ACPI_FAILURE(status))
> >                 return 1;
> >
> > > +     gpio_adev = acpi_fetch_acpi_dev(handle);
> > > +     if (!gpio_adev)
> > > +             return 1;
> > > +
> > > +     gdev = gpio_device_find_by_fwnode(acpi_fwnode_handle(gpio_adev));
> >
> >         struct gpio_device *gdev __free(gpio_device_put) =
> >                 gpio_device_find_by_fwnode(acpi_fwnode_handle(gpio_adev));
> >
> > > +     if (!gdev)
> > > +             return -EPROBE_DEFER; /* controller not registered yet: abort walk */
> > > +
> > > +     gpio_dev = gpio_device_to_device(gdev)->parent;
> >
> > > +     scoped_guard(device, gpio_dev) {
> >
> > Can we use simple guard()() here?
> >
> > > +             if (!device_is_bound(gpio_dev))
> > > +                     return -EPROBE_DEFER; /* controller not bound yet: abort walk */
> > > +     }
> > > +
> > > +     return 1; /* bound, skip adding to resource list, continue walk */
> > > +}
> >
> > ...
> >
> > > +static int check_child_gpioint(struct acpi_device *adev, void *data)
> > > +{
> > > +     struct list_head res_list;
> > > +     int ret;
> > > +
> > > +     INIT_LIST_HEAD(&res_list);
> >
> > Instead just define with LIST_HEAD() macro.
> >
> >         LIST_HEAD(res_list);
> >         int ret;
> >
> > > +     ret = acpi_dev_get_resources(adev, &res_list, check_gpioint_resource, NULL);
> >
> > > +     acpi_dev_free_resource_list(&res_list);
> > > +
> > > +     /*
> > > +      * ret is a nonnegative resource count on success, which must not
> > > +      * be mistaken for a nonzero "stop iteration" signal by
> > > +      * acpi_dev_for_each_child(); only forward genuine errors.
> > > +      */
> > > +     return ret < 0 ? ret : 0;
> >
> > Check for errors when it's required. I believe you fixed it at some point
> > and now it's again old version. Please, reduce full rely on AI, it's not
> > helpful sometimes.
> >
> >         ret = acpi_dev_get_resources(adev, &res_list, check_gpioint_resource, NULL);
> >         if (ret < 0)
> >                 return ret;
> >         ...
> >         return 0;
> >
> > > +}
> >
> > ...
> >
> > > +static int i2c_dw_check_gpio_dependencies(struct device *dev)
> > > +{
> > > +     struct acpi_device *adev = ACPI_COMPANION(dev);
> >
> > This style is discouraged as it brings an unneeded burden on a maintenance.
> >
> > > +     if (!adev)
> > > +             return 0;
> >
> >         struct acpi_device *adev;
> >
> >         adev = ACPI_COMPANION(dev);
> >         if (!adev)
> >                 return 0;
> >
> > > +     return acpi_dev_for_each_child(adev, check_child_gpioint, NULL);
> > > +}
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >