Re: [PATCH v3 01/11] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 10/13/21 8:48 PM, Rafael J. Wysocki wrote:
> On Wed, Oct 13, 2021 at 8:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 10/13/21 7:29 PM, Rafael J. Wysocki wrote:
>>> On Sun, Oct 10, 2021 at 8:57 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> The clk and regulator frameworks expect clk/regulator consumer-devices
>>>> to have info about the consumed clks/regulators described in the device's
>>>> fw_node.
>>>>
>>>> To work around cases where this info is not present in the firmware tables,
>>>> which is often the case on x86/ACPI devices, both frameworks allow the
>>>> provider-driver to attach info about consumers to the clks/regulators
>>>> when registering these.
>>>>
>>>> This causes problems with the probe ordering wrt drivers for consumers
>>>> of these clks/regulators. Since the lookups are only registered when the
>>>> provider-driver binds, trying to get these clks/regulators before then
>>>> results in a -ENOENT error for clks and a dummy regulator for regulators.
>>>>
>>>> One case where we hit this issue is camera sensors such as e.g. the OV8865
>>>> sensor found on the Microsoft Surface Go. The sensor uses clks, regulators
>>>> and GPIOs provided by a TPS68470 PMIC which is described in an INT3472
>>>> ACPI device. There is special platform code handling this and setting
>>>> platform_data with the necessary consumer info on the MFD cells
>>>> instantiated for the PMIC under: drivers/platform/x86/intel/int3472.
>>>>
>>>> For this to work properly the ov8865 driver must not bind to the I2C-client
>>>> for the OV8865 sensor until after the TPS68470 PMIC gpio, regulator and
>>>> clk MFD cells have all been fully setup.
>>>>
>>>> The OV8865 on the Microsoft Surface Go is just one example, all X86
>>>> devices using the Intel IPU3 camera block found on recent Intel SoCs
>>>> have similar issues where there is an INT3472 HID ACPI-device, which
>>>> describes the clks and regulators, and the driver for this INT3472 device
>>>> must be fully initialized before the sensor driver (any sensor driver)
>>>> binds for things to work properly.
>>>>
>>>> On these devices the ACPI nodes describing the sensors all have a _DEP
>>>> dependency on the matching INT3472 ACPI device (there is one per sensor).
>>>>
>>>> This allows solving the probe-ordering problem by delaying the enumeration
>>>> (instantiation of the I2C-client in the ov8865 example) of ACPI-devices
>>>> which have a _DEP dependency on an INT3472 device.
>>>>
>>>> The new acpi_dev_ready_for_enumeration() helper used for this is also
>>>> exported because for devices, which have the enumeration_by_parent flag
>>>> set, the parent-driver will do its own scan of child ACPI devices and
>>>> it will try to enumerate those during its probe(). Code doing this such
>>>> as e.g. the i2c-core-acpi.c code must call this new helper to ensure
>>>> that it too delays the enumeration until all the _DEP dependencies are
>>>> met on devices which have the new honor_deps flag set.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/acpi/scan.c     | 36 ++++++++++++++++++++++++++++++++++--
>>>>  include/acpi/acpi_bus.h |  5 ++++-
>>>>  2 files changed, 38 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>>> index 5b54c80b9d32..efee6ee91c8f 100644
>>>> --- a/drivers/acpi/scan.c
>>>> +++ b/drivers/acpi/scan.c
>>>> @@ -796,6 +796,12 @@ static const char * const acpi_ignore_dep_ids[] = {
>>>>         NULL
>>>>  };
>>>>
>>>> +/* List of HIDs for which we honor deps of matching ACPI devs, when checking _DEP lists. */
>>>> +static const char * const acpi_honor_dep_ids[] = {
>>>> +       "INT3472", /* Camera sensor PMIC / clk and regulator info */
>>>> +       NULL
>>>> +};
>>>> +
>>>>  static struct acpi_device *acpi_bus_get_parent(acpi_handle handle)
>>>>  {
>>>>         struct acpi_device *device = NULL;
>>>> @@ -1757,8 +1763,12 @@ static void acpi_scan_dep_init(struct acpi_device *adev)
>>>>         struct acpi_dep_data *dep;
>>>>
>>>>         list_for_each_entry(dep, &acpi_dep_list, node) {
>>>> -               if (dep->consumer == adev->handle)
>>>> +               if (dep->consumer == adev->handle) {
>>>> +                       if (dep->honor_dep)
>>>> +                               adev->flags.honor_deps = 1;
>>>
>>> Any concerns about doing
>>>
>>> adev->flags.honor_deps = dep->honor_dep;
>>>
>>> here?
>>
>> The idea is to set adev->flags.honor_deps even if the device has
>> multiple deps and only one of them has the honor_dep flag set.
>>
>> If we just do:
>>
>>         adev->flags.honor_deps = dep->honor_dep;
>>
>> Then adev->flags.honor_deps ends up having the honor_dep
>> flag of the last dependency checked.
> 
> OK, but in that case dep_unmet may be blocking the enumeration of the
> device even if the one in the acpi_honor_dep_ids[] list has probed
> successfully.
> 
> Isn't that a concern?

For the devices where we set the dep->honor_dep flag this is
not a concern (based on the DSDTs which I've seen).

I also don't expect it to be a concern for other cases where we may
set that flag in the future either. This is an opt-in thing, so
I expect that in cases where we opt in to this, we also ensure that
any other _DEPs are also met (by having a Linux driver which calls
acpi_dev_clear_dependencies() for them).

And now a days we also have the acpi_ignore_dep_ids[] list so if
in the future there are some _DEP-s which never get fulfilled/met
on a device where we set the adev->flags.honor_deps flag, then we
can always add the ACPI HIDs for those devices to that list.

>>>> +
>>>>                         adev->dep_unmet++;
>>>> +               }
>>>>         }
>>>>  }
>>>>
>>>> @@ -1962,7 +1972,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
>>>>         for (count = 0, i = 0; i < dep_devices.count; i++) {
>>>>                 struct acpi_device_info *info;
>>>>                 struct acpi_dep_data *dep;
>>>> -               bool skip;
>>>> +               bool skip, honor_dep;
>>>>
>>>>                 status = acpi_get_object_info(dep_devices.handles[i], &info);
>>>>                 if (ACPI_FAILURE(status)) {
>>>> @@ -1971,6 +1981,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
>>>>                 }
>>>>
>>>>                 skip = acpi_info_matches_ids(info, acpi_ignore_dep_ids);
>>>> +              honor_dep = acpi_info_matches_ids(info, acpi_honor_dep_ids);
>>>>                 kfree(info);
>>>>
>>>>                 if (skip)
>>>> @@ -1984,6 +1995,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
>>>>
>>>>                 dep->supplier = dep_devices.handles[i];
>>>>                 dep->consumer = handle;
>>>> +               dep->honor_dep = honor_dep;
>>>>
>>>>                 mutex_lock(&acpi_dep_list_lock);
>>>>                 list_add_tail(&dep->node , &acpi_dep_list);
>>>> @@ -2071,6 +2083,9 @@ static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
>>>>
>>>>  static void acpi_default_enumeration(struct acpi_device *device)
>>>>  {
>>>> +       if (!acpi_dev_ready_for_enumeration(device))
>>>> +               return;
>>>
>>> I'm not sure about this.
>>>
>>> First of all, this adds an acpi_device_is_present() check here which
>>> potentially is a change in behavior and I'm not sure how it is related
>>> to the other changes in this patch (it is not mentioned in the
>>> changelog AFAICS).
>>>
>>> I'm saying "potentially", because if we get here at all,
>>> acpi_device_is_present() has been evaluated already by
>>> acpi_bus_attach().
>>
>> Right the idea was that for this code-path the extra
>> acpi_device_is_present() check is a no-op since the only
>> caller of acpi_default_enumeration() has already done
>> that check before calling acpi_default_enumeration(),
>> where as the is_present check is useful for users outside
>> of the ACPI core code, like e.g. the i2c ACPI enumeration
>> code.
>>
>> Although I see this is also called from
>> acpi_generic_device_attach which comes into play when there
>> is devicetree info embedded inside the ACPI tables.
> 
> That too, but generally speaking this change should at least be
> mentioned in the changelog.
> 
>>> Now, IIUC, the new acpi_dev_ready_for_enumeration() is kind of an
>>> extension of acpi_device_is_present(), so shouldn't it be called by
>>> acpi_bus_attach() instead of the latter rather than from here?
>>
>> That is an interesting proposal. I assume you want this to replace
>> the current acpi_device_is_present() call in acpi_bus_attach()
>> then ?
> 
> That seems consistent to me.
> 
>> For the use-case at hand here that should work fine and it would also
>> make the honor_deps flag work for devices which bind to the actual
>> acpi_device (because we delay the device_attach()) or
>> use an acpi_scan_handler.
>>
>> This would mean though that we can now have acpi_device-s where
>> acpi_device_is_present() returns true, but which are not
>> initialized (do not have device->flags.initialized set)
>> that would be a new acpi_device state which we have not had
>> before. I do not immediately forsee this causing issues,
>> but still...
>>
>> If you want me to replace the current acpi_device_is_present() call
>> in acpi_bus_attach() with the new acpi_dev_ready_for_enumeration()
>> helper, let me know and I'll prepare a new version with this change
>> (and run some tests with that new version).
> 
> I would prefer doing that to making acpi_default_enumeration() special
> with respect to the handling of dependencies.

Ok I will make this change in the next version (ETA sometime next week).

Regards,

Hans