| Message ID | 20211010185707.195883-2-hdegoede@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data | expand |
Hi, On Sun, Oct 10, 2021 at 08:56:57PM +0200, Hans de Goede wrote: > +/* 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 */ Is there some reason why we can't do this for all devices with _DEP? That way we don't need to maintain lists like this. Otherwise looks good.
Hi, On 10/11/21 8:19 AM, Mika Westerberg wrote: > Hi, > > On Sun, Oct 10, 2021 at 08:56:57PM +0200, Hans de Goede wrote: >> +/* 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 */ > > Is there some reason why we can't do this for all devices with _DEP? > That way we don't need to maintain lists like this. Up until now the ACPI core deliberate mostly ignores _DEP-s because the _DEP method may point to pretty much any random ACPI object and Linux does not necessarily have a driver for all ACPI objects the driver points too, which would lead to the devices never getting instantiated. In hindsight this might not have been the best solution (1), but if we now start honoring _DEP-s for all devices all of a sudden then this will almost certainly lead to a whole bunch of regressions. Note that in this case the HID which triggers this is for the device being depended upon and for all camera sensors used with the IPU3 and IPU4 Intel camera blocks this is the INT3472 device. By triggering on this HID (rather then on the sensor HIDs) I expect that we will not need to update this list all that often. Regards, Hans 1) I believe that Windows does pay more reference to the _DEP-s and we've had some other related issues lately.
On Mon, Oct 11, 2021 at 09:11:05AM +0200, Hans de Goede wrote: > Hi, > > On 10/11/21 8:19 AM, Mika Westerberg wrote: > > Hi, > > > > On Sun, Oct 10, 2021 at 08:56:57PM +0200, Hans de Goede wrote: > >> +/* 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 */ > > > > Is there some reason why we can't do this for all devices with _DEP? > > That way we don't need to maintain lists like this. > > Up until now the ACPI core deliberate mostly ignores _DEP-s because the > _DEP method may point to pretty much any random ACPI object and Linux does > not necessarily have a driver for all ACPI objects the driver points too, > which would lead to the devices never getting instantiated. > > In hindsight this might not have been the best solution (1), but if we > now start honoring _DEP-s for all devices all of a sudden then this > will almost certainly lead to a whole bunch of regressions. > > Note that in this case the HID which triggers this is for the device > being depended upon and for all camera sensors used with the IPU3 and > IPU4 Intel camera blocks this is the INT3472 device. By triggering on > this HID (rather then on the sensor HIDs) I expect that we will not > need to update this list all that often. I see and agree. Thanks for the explanation! No objections from my side then :)
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? > + > 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(). 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? > + > /* > * Do not enumerate devices with enumeration_by_parent flag set as > * they will be enumerated by their respective parents. > @@ -2313,6 +2328,23 @@ void acpi_dev_clear_dependencies(struct acpi_device *supplier) > } > EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies); > > +/** > + * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration > + * @device: Pointer to the &struct acpi_device to check > + * > + * Check if the device is present and has no unmet dependencies. > + * > + * Return true if the device is ready for enumeratino. Otherwise, return false. > + */ > +bool acpi_dev_ready_for_enumeration(const struct acpi_device *device) > +{ > + if (device->flags.honor_deps && device->dep_unmet) > + return false; > + > + return acpi_device_is_present(device); > +} > +EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration); > + > /** > * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier > * @supplier: Pointer to the dependee device > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 13d93371790e..2da53b7b4965 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -202,7 +202,8 @@ struct acpi_device_flags { > u32 coherent_dma:1; > u32 cca_seen:1; > u32 enumeration_by_parent:1; > - u32 reserved:19; > + u32 honor_deps:1; > + u32 reserved:18; > }; > > /* File System */ > @@ -284,6 +285,7 @@ struct acpi_dep_data { > struct list_head node; > acpi_handle supplier; > acpi_handle consumer; > + bool honor_dep; > }; > > /* Performance Management */ > @@ -693,6 +695,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) > bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2); > > void acpi_dev_clear_dependencies(struct acpi_device *supplier); > +bool acpi_dev_ready_for_enumeration(const struct acpi_device *device); > struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier); > struct acpi_device * > acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); > --
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. > >> + >> 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. > 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 ? 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). Regards, Hans > >> + >> /* >> * Do not enumerate devices with enumeration_by_parent flag set as >> * they will be enumerated by their respective parents. >> @@ -2313,6 +2328,23 @@ void acpi_dev_clear_dependencies(struct acpi_device *supplier) >> } >> EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies); >> >> +/** >> + * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration >> + * @device: Pointer to the &struct acpi_device to check >> + * >> + * Check if the device is present and has no unmet dependencies. >> + * >> + * Return true if the device is ready for enumeratino. Otherwise, return false. >> + */ >> +bool acpi_dev_ready_for_enumeration(const struct acpi_device *device) >> +{ >> + if (device->flags.honor_deps && device->dep_unmet) >> + return false; >> + >> + return acpi_device_is_present(device); >> +} >> +EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration); >> + >> /** >> * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier >> * @supplier: Pointer to the dependee device >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h >> index 13d93371790e..2da53b7b4965 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -202,7 +202,8 @@ struct acpi_device_flags { >> u32 coherent_dma:1; >> u32 cca_seen:1; >> u32 enumeration_by_parent:1; >> - u32 reserved:19; >> + u32 honor_deps:1; >> + u32 reserved:18; >> }; >> >> /* File System */ >> @@ -284,6 +285,7 @@ struct acpi_dep_data { >> struct list_head node; >> acpi_handle supplier; >> acpi_handle consumer; >> + bool honor_dep; >> }; >> >> /* Performance Management */ >> @@ -693,6 +695,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) >> bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2); >> >> void acpi_dev_clear_dependencies(struct acpi_device *supplier); >> +bool acpi_dev_ready_for_enumeration(const struct acpi_device *device); >> struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier); >> struct acpi_device * >> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); >> -- >
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? > > > >> + > >> 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.
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
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; + 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; + /* * Do not enumerate devices with enumeration_by_parent flag set as * they will be enumerated by their respective parents. @@ -2313,6 +2328,23 @@ void acpi_dev_clear_dependencies(struct acpi_device *supplier) } EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies); +/** + * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration + * @device: Pointer to the &struct acpi_device to check + * + * Check if the device is present and has no unmet dependencies. + * + * Return true if the device is ready for enumeratino. Otherwise, return false. + */ +bool acpi_dev_ready_for_enumeration(const struct acpi_device *device) +{ + if (device->flags.honor_deps && device->dep_unmet) + return false; + + return acpi_device_is_present(device); +} +EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration); + /** * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier * @supplier: Pointer to the dependee device diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 13d93371790e..2da53b7b4965 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -202,7 +202,8 @@ struct acpi_device_flags { u32 coherent_dma:1; u32 cca_seen:1; u32 enumeration_by_parent:1; - u32 reserved:19; + u32 honor_deps:1; + u32 reserved:18; }; /* File System */ @@ -284,6 +285,7 @@ struct acpi_dep_data { struct list_head node; acpi_handle supplier; acpi_handle consumer; + bool honor_dep; }; /* Performance Management */ @@ -693,6 +695,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2); void acpi_dev_clear_dependencies(struct acpi_device *supplier); +bool acpi_dev_ready_for_enumeration(const struct acpi_device *device); struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier); struct acpi_device * acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
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(-)