| Message ID | 20210222130735.1313443-2-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce intel_skl_int3472 module | expand |
Hi Andy On 22/02/2021 13:34, Andy Shevchenko wrote: > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote: >> The acpi_walk_dep_device_list() is not as generalisable as its name >> implies, serving only to decrement the dependency count for each >> dependent device of the input. Extend the function to instead accept >> a callback which can be applied to all the dependencies in acpi_dep_list. >> Replace all existing calls to the function with calls to a wrapper, passing >> a callback that applies the same dependency reduction. > The code looks okay to me, if it was the initial idea, feel free to add > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Thank you! >> + */ >> +void acpi_dev_flag_dependency_met(acpi_handle handle) >> +{ > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here? I can do this, but I avoided it because in most of the uses in the kernel currently there's no struct acpi_device, they're just passing ACPI_HANDLE(dev) instead, so I'd need to get the adev with ACPI_COMPANION() in each place. It didn't seem worth it...but happy to do it if you'd prefer it that way?
On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote: > On 22/02/2021 13:34, Andy Shevchenko wrote: > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote: > >> The acpi_walk_dep_device_list() is not as generalisable as its name > >> implies, serving only to decrement the dependency count for each > >> dependent device of the input. Extend the function to instead accept > >> a callback which can be applied to all the dependencies in acpi_dep_list. > >> Replace all existing calls to the function with calls to a wrapper, passing > >> a callback that applies the same dependency reduction. > > The code looks okay to me, if it was the initial idea, feel free to add > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Thank you! > > > >> + */ > >> +void acpi_dev_flag_dependency_met(acpi_handle handle) > >> +{ > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here? > > > I can do this, but I avoided it because in most of the uses in the > kernel currently there's no struct acpi_device, they're just passing > ACPI_HANDLE(dev) instead, so I'd need to get the adev with > ACPI_COMPANION() in each place. It didn't seem worth it...but happy to > do it if you'd prefer it that way? I see, let Rafael decide then. I'm not pushing here.
On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote: > > On 22/02/2021 13:34, Andy Shevchenko wrote: > > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote: > > >> The acpi_walk_dep_device_list() is not as generalisable as its name > > >> implies, serving only to decrement the dependency count for each > > >> dependent device of the input. Extend the function to instead accept > > >> a callback which can be applied to all the dependencies in acpi_dep_list. > > >> Replace all existing calls to the function with calls to a wrapper, passing > > >> a callback that applies the same dependency reduction. > > > The code looks okay to me, if it was the initial idea, feel free to add > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > > > Thank you! > > > > > > >> + */ > > >> +void acpi_dev_flag_dependency_met(acpi_handle handle) > > >> +{ > > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here? > > > > > > I can do this, but I avoided it because in most of the uses in the > > kernel currently there's no struct acpi_device, they're just passing > > ACPI_HANDLE(dev) instead, so I'd need to get the adev with > > ACPI_COMPANION() in each place. It didn't seem worth it... It may not even be possible sometimes, because that function may be called before creating all of the struct acpi_device objects (like in the case of deferred enumeration). > > but happy to > > do it if you'd prefer it that way? > > I see, let Rafael decide then. I'm not pushing here. Well, it's a matter of correctness.
On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote: > On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote: > > > On 22/02/2021 13:34, Andy Shevchenko wrote: > > > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote: > > > >> The acpi_walk_dep_device_list() is not as generalisable as its name > > > >> implies, serving only to decrement the dependency count for each > > > >> dependent device of the input. Extend the function to instead accept > > > >> a callback which can be applied to all the dependencies in acpi_dep_list. > > > >> Replace all existing calls to the function with calls to a wrapper, passing > > > >> a callback that applies the same dependency reduction. > > > > The code looks okay to me, if it was the initial idea, feel free to add > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> ... > > > >> +void acpi_dev_flag_dependency_met(acpi_handle handle) > > > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here? > > > > > > I can do this, but I avoided it because in most of the uses in the > > > kernel currently there's no struct acpi_device, they're just passing > > > ACPI_HANDLE(dev) instead, so I'd need to get the adev with > > > ACPI_COMPANION() in each place. It didn't seem worth it... > > It may not even be possible sometimes, because that function may be > called before creating all of the struct acpi_device objects (like in > the case of deferred enumeration). > > > > but happy to > > > do it if you'd prefer it that way? > > > > I see, let Rafael decide then. I'm not pushing here. > > Well, it's a matter of correctness. Looking at your above comment it is indeed. Thanks for clarification! But should we have acpi_dev_*() namespace for this function if it takes handle? For time being nothing better than following comes to my mind: __acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met() acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met() -- With Best Regards, Andy Shevchenko
On Mon, Mar 8, 2021 at 2:57 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote: > > On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote: > > > > On 22/02/2021 13:34, Andy Shevchenko wrote: > > > > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote: > > > > >> The acpi_walk_dep_device_list() is not as generalisable as its name > > > > >> implies, serving only to decrement the dependency count for each > > > > >> dependent device of the input. Extend the function to instead accept > > > > >> a callback which can be applied to all the dependencies in acpi_dep_list. > > > > >> Replace all existing calls to the function with calls to a wrapper, passing > > > > >> a callback that applies the same dependency reduction. > > > > > The code looks okay to me, if it was the initial idea, feel free to add > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > ... > > > > > >> +void acpi_dev_flag_dependency_met(acpi_handle handle) > > > > > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here? > > > > > > > > I can do this, but I avoided it because in most of the uses in the > > > > kernel currently there's no struct acpi_device, they're just passing > > > > ACPI_HANDLE(dev) instead, so I'd need to get the adev with > > > > ACPI_COMPANION() in each place. It didn't seem worth it... > > > > It may not even be possible sometimes, because that function may be > > called before creating all of the struct acpi_device objects (like in > > the case of deferred enumeration). > > > > > > but happy to > > > > do it if you'd prefer it that way? > > > > > > I see, let Rafael decide then. I'm not pushing here. > > > > Well, it's a matter of correctness. > > Looking at your above comment it is indeed. Thanks for clarification! Well, actually, the struct device for the object passed to this function should be there already, because otherwise it wouldn't make sense to update the list. So my comment above is not really applicable to this particular device and the function could take a struct acpi_device pointer argument. Sorry for the confusion. > But should we have acpi_dev_*() namespace for this function if it takes handle? It takes a device object handle. Anyway, as per the above, it can take a struct acpi_device pointer argument in which case the "acpi_dev_" prefix should be fine. > For time being nothing better than following comes to my mind: > > __acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met() > acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met() The above said, the name is somewhat confusing overall IMV. Something like acpi_dev_clear_dependencies() might be better. So lets make it something like void acpi_dev_clear_dependencies(struct acpi_device *supplier);
On Mon, Mar 8, 2021 at 4:45 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Mar 8, 2021 at 2:57 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote: > > > On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote: > > > > > On 22/02/2021 13:34, Andy Shevchenko wrote: > > > > > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote: > > > > > >> The acpi_walk_dep_device_list() is not as generalisable as its name > > > > > >> implies, serving only to decrement the dependency count for each > > > > > >> dependent device of the input. Extend the function to instead accept > > > > > >> a callback which can be applied to all the dependencies in acpi_dep_list. > > > > > >> Replace all existing calls to the function with calls to a wrapper, passing > > > > > >> a callback that applies the same dependency reduction. > > > > > > The code looks okay to me, if it was the initial idea, feel free to add > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > ... > > > > > > > >> +void acpi_dev_flag_dependency_met(acpi_handle handle) > > > > > > > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here? > > > > > > > > > > I can do this, but I avoided it because in most of the uses in the > > > > > kernel currently there's no struct acpi_device, they're just passing > > > > > ACPI_HANDLE(dev) instead, so I'd need to get the adev with > > > > > ACPI_COMPANION() in each place. It didn't seem worth it... > > > > > > It may not even be possible sometimes, because that function may be > > > called before creating all of the struct acpi_device objects (like in > > > the case of deferred enumeration). > > > > > > > > but happy to > > > > > do it if you'd prefer it that way? > > > > > > > > I see, let Rafael decide then. I'm not pushing here. > > > > > > Well, it's a matter of correctness. > > > > Looking at your above comment it is indeed. Thanks for clarification! > > Well, actually, the struct device for the object passed to this > function should be there already, because otherwise it wouldn't make > sense to update the list. So my comment above is not really > applicable to this particular device and the function could take a > struct acpi_device pointer argument. Sorry for the confusion. > > > But should we have acpi_dev_*() namespace for this function if it takes handle? > > It takes a device object handle. > > Anyway, as per the above, it can take a struct acpi_device pointer > argument in which case the "acpi_dev_" prefix should be fine. > > > For time being nothing better than following comes to my mind: > > > > __acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met() > > acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met() > > The above said, the name is somewhat confusing overall IMV. > > Something like acpi_dev_clear_dependencies() might be better. > > So lets make it something like > > void acpi_dev_clear_dependencies(struct acpi_device *supplier); To be precise, there are two functions in the patch, acpi_dev_flag_dependency_met() which invokes acpi_walk_dep_device_list() and __acpi_dev_flag_dependency_met() invoked by the latter as a callback. Above I was talking about the first one. The callback should still take a struct acpi_dep_data pointer argument and I would call it acpi_scan_clear_dep() or similar.
On Mon, Feb 22, 2021 at 2:07 PM Daniel Scally <djrscally@gmail.com> wrote: > > The acpi_walk_dep_device_list() is not as generalisable as its name > implies, serving only to decrement the dependency count for each > dependent device of the input. Extend the function to instead accept > a callback which can be applied to all the dependencies in acpi_dep_list. > Replace all existing calls to the function with calls to a wrapper, passing > a callback that applies the same dependency reduction. > > Suggested-by: Rafael J. Wysocki <rafael@kernel.org> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v3: > - patch introduced > > drivers/acpi/ec.c | 2 +- > drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +- > drivers/acpi/scan.c | 58 ++++++++++++++++------- > drivers/gpio/gpiolib-acpi.c | 2 +- > drivers/i2c/i2c-core-acpi.c | 2 +- > drivers/platform/surface/surface3_power.c | 2 +- > include/acpi/acpi_bus.h | 7 +++ > include/linux/acpi.h | 4 +- > 8 files changed, 55 insertions(+), 24 deletions(-) > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c > index 13565629ce0a..a258db713bd2 100644 > --- a/drivers/acpi/ec.c > +++ b/drivers/acpi/ec.c > @@ -1627,7 +1627,7 @@ static int acpi_ec_add(struct acpi_device *device) > WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr); > > /* Reprobe devices depending on the EC */ > - acpi_walk_dep_device_list(ec->handle); > + acpi_dev_flag_dependency_met(ec->handle); > > acpi_handle_debug(ec->handle, "enumerated.\n"); > return 0; > diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c > index a5101b07611a..59cca504325e 100644 > --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c > +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c > @@ -117,7 +117,7 @@ static int chtdc_ti_pmic_opregion_probe(struct platform_device *pdev) > return err; > > /* Re-enumerate devices depending on PMIC */ > - acpi_walk_dep_device_list(ACPI_HANDLE(pdev->dev.parent)); > + acpi_dev_flag_dependency_met(ACPI_HANDLE(pdev->dev.parent)); > return 0; > } > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 80b668c80073..c9e4190316ef 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -49,12 +49,6 @@ static DEFINE_MUTEX(acpi_hp_context_lock); > */ > static u64 spcr_uart_addr; > > -struct acpi_dep_data { > - struct list_head node; > - acpi_handle supplier; > - acpi_handle consumer; > -}; > - > void acpi_scan_lock_acquire(void) > { > mutex_lock(&acpi_scan_lock); > @@ -2099,30 +2093,58 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass) > device->handler->hotplug.notify_online(device); > } > > -void acpi_walk_dep_device_list(acpi_handle handle) > +static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep, > + void *data) > { > - struct acpi_dep_data *dep, *tmp; > struct acpi_device *adev; > > + acpi_bus_get_device(dep->consumer, &adev); > + if (!adev) > + return 0; > + > + adev->dep_unmet--; > + if (!adev->dep_unmet) > + acpi_bus_attach(adev, true); > + > + list_del(&dep->node); > + kfree(dep); > + return 0; > +} > + > +void acpi_walk_dep_device_list(acpi_handle handle, > + int (*callback)(struct acpi_dep_data *, void *), > + void *data) > +{ > + struct acpi_dep_data *dep, *tmp; > + int ret; > + > mutex_lock(&acpi_dep_list_lock); > list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) { > if (dep->supplier == handle) { > - acpi_bus_get_device(dep->consumer, &adev); > - if (!adev) > - continue; > - > - adev->dep_unmet--; > - if (!adev->dep_unmet) > - acpi_bus_attach(adev, true); The above code in the mainline has changed recently, so you need to rebase the above and adjust for the change of behavior. > - > - list_del(&dep->node); > - kfree(dep); > + ret = callback(dep, data); > + if (ret) > + break; > } > } > mutex_unlock(&acpi_dep_list_lock); > } > EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list); > > +/** > + * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the device > + * is now active No parens here, please, and make it fit one line. Also the description should be something like "Clear dependencies on the given device." > + * @handle: acpi_handle for the supplier device > + * > + * This function walks through the dependencies list and informs each consumer > + * of @handle that their dependency upon it is now met. Devices with no more > + * unmet dependencies will be attached to the acpi bus. > + */ > +void acpi_dev_flag_dependency_met(acpi_handle handle) > +{ > + acpi_walk_dep_device_list(handle, __acpi_dev_flag_dependency_met, NULL); > +} > +EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met); > + > /** > * acpi_bus_scan - Add ACPI device node objects in a given namespace scope. > * @handle: Root of the namespace scope to scan. > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index e37a57d0a2f0..e4d728fda982 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -1254,7 +1254,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip) > > acpi_gpiochip_request_regions(acpi_gpio); > acpi_gpiochip_scan_gpios(acpi_gpio); > - acpi_walk_dep_device_list(handle); > + acpi_dev_flag_dependency_met(handle); > } > > void acpi_gpiochip_remove(struct gpio_chip *chip) > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 37c510d9347a..38647cf34bde 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -283,7 +283,7 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap) > if (!handle) > return; > > - acpi_walk_dep_device_list(handle); > + acpi_dev_flag_dependency_met(handle); > } > > static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = { > diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c > index cc4f9cba6856..ad895285d3e9 100644 > --- a/drivers/platform/surface/surface3_power.c > +++ b/drivers/platform/surface/surface3_power.c > @@ -478,7 +478,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client) > return -ENOMEM; > } > > - acpi_walk_dep_device_list(handle); > + acpi_dev_flag_dependency_met(handle); > return 0; > } > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 02a716a0af5d..91172af3a04d 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -278,6 +278,12 @@ struct acpi_device_power { > struct acpi_device_power_state states[ACPI_D_STATE_COUNT]; /* Power states (D0-D3Cold) */ > }; > > +struct acpi_dep_data { > + struct list_head node; > + acpi_handle supplier; > + acpi_handle consumer; > +}; > + > /* Performance Management */ > > struct acpi_device_perf_flags { > @@ -683,6 +689,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_flag_dependency_met(acpi_handle handle); > struct acpi_device * > acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); > struct acpi_device * > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 2630c2e953f7..2d5e6e88e8a0 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -655,7 +655,9 @@ extern bool acpi_driver_match_device(struct device *dev, > const struct device_driver *drv); > int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); > int acpi_device_modalias(struct device *, char *, int); > -void acpi_walk_dep_device_list(acpi_handle handle); > +void acpi_walk_dep_device_list(acpi_handle handle, > + int (*callback)(struct acpi_dep_data *, void *), > + void *data); > > struct platform_device *acpi_create_platform_device(struct acpi_device *, > struct property_entry *); > -- > 2.25.1 >
Hi Rafael On 08/03/2021 17:46, Rafael J. Wysocki wrote: >> +void acpi_walk_dep_device_list(acpi_handle handle, >> + int (*callback)(struct acpi_dep_data *, void *), >> + void *data) >> +{ >> + struct acpi_dep_data *dep, *tmp; >> + int ret; >> + >> mutex_lock(&acpi_dep_list_lock); >> list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) { >> if (dep->supplier == handle) { >> - acpi_bus_get_device(dep->consumer, &adev); >> - if (!adev) >> - continue; >> - >> - adev->dep_unmet--; >> - if (!adev->dep_unmet) >> - acpi_bus_attach(adev, true); > The above code in the mainline has changed recently, so you need to > rebase the above and adjust for the change of behavior. Yeah, I'll rebase onto 5.12-rc2 before next submission. > >> - >> - list_del(&dep->node); >> - kfree(dep); >> + ret = callback(dep, data); >> + if (ret) >> + break; >> } >> } >> mutex_unlock(&acpi_dep_list_lock); >> } >> EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list); >> >> +/** >> + * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the device >> + * is now active > No parens here, please, and make it fit one line. > > Also the description should be something like "Clear dependencies on > the given device." OK - no problem
Hi Rafael On 08/03/2021 17:23, Rafael J. Wysocki wrote: > On Mon, Mar 8, 2021 at 4:45 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Mon, Mar 8, 2021 at 2:57 PM Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >>> On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote: >>>> On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko >>>> <andy.shevchenko@gmail.com> wrote: >>>>> On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote: >>>>>> On 22/02/2021 13:34, Andy Shevchenko wrote: >>>>>>> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote: >>>>>>>> The acpi_walk_dep_device_list() is not as generalisable as its name >>>>>>>> implies, serving only to decrement the dependency count for each >>>>>>>> dependent device of the input. Extend the function to instead accept >>>>>>>> a callback which can be applied to all the dependencies in acpi_dep_list. >>>>>>>> Replace all existing calls to the function with calls to a wrapper, passing >>>>>>>> a callback that applies the same dependency reduction. >>>>>>> The code looks okay to me, if it was the initial idea, feel free to add >>>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> ... >>> >>>>>>>> +void acpi_dev_flag_dependency_met(acpi_handle handle) >>>>>>> Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here? >>>>>> I can do this, but I avoided it because in most of the uses in the >>>>>> kernel currently there's no struct acpi_device, they're just passing >>>>>> ACPI_HANDLE(dev) instead, so I'd need to get the adev with >>>>>> ACPI_COMPANION() in each place. It didn't seem worth it... >>>> It may not even be possible sometimes, because that function may be >>>> called before creating all of the struct acpi_device objects (like in >>>> the case of deferred enumeration). >>>> >>>>>> but happy to >>>>>> do it if you'd prefer it that way? >>>>> I see, let Rafael decide then. I'm not pushing here. >>>> Well, it's a matter of correctness. >>> Looking at your above comment it is indeed. Thanks for clarification! >> Well, actually, the struct device for the object passed to this >> function should be there already, because otherwise it wouldn't make >> sense to update the list. So my comment above is not really >> applicable to this particular device and the function could take a >> struct acpi_device pointer argument. Sorry for the confusion. >> >>> But should we have acpi_dev_*() namespace for this function if it takes handle? >> It takes a device object handle. >> >> Anyway, as per the above, it can take a struct acpi_device pointer >> argument in which case the "acpi_dev_" prefix should be fine. OK, so the conclusion there is change the argument to a struct acpi_device pointer and update all the uses. >>> For time being nothing better than following comes to my mind: >>> >>> __acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met() >>> acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met() >> The above said, the name is somewhat confusing overall IMV. >> >> Something like acpi_dev_clear_dependencies() might be better. >> >> So lets make it something like >> >> void acpi_dev_clear_dependencies(struct acpi_device *supplier); > To be precise, there are two functions in the patch, > acpi_dev_flag_dependency_met() which invokes > acpi_walk_dep_device_list() and __acpi_dev_flag_dependency_met() > invoked by the latter as a callback. > > Above I was talking about the first one. > > The callback should still take a struct acpi_dep_data pointer argument > and I would call it acpi_scan_clear_dep() or similar. OK, works for me, I'll make those changes - thanks
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 13565629ce0a..a258db713bd2 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -1627,7 +1627,7 @@ static int acpi_ec_add(struct acpi_device *device) WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr); /* Reprobe devices depending on the EC */ - acpi_walk_dep_device_list(ec->handle); + acpi_dev_flag_dependency_met(ec->handle); acpi_handle_debug(ec->handle, "enumerated.\n"); return 0; diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c index a5101b07611a..59cca504325e 100644 --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c @@ -117,7 +117,7 @@ static int chtdc_ti_pmic_opregion_probe(struct platform_device *pdev) return err; /* Re-enumerate devices depending on PMIC */ - acpi_walk_dep_device_list(ACPI_HANDLE(pdev->dev.parent)); + acpi_dev_flag_dependency_met(ACPI_HANDLE(pdev->dev.parent)); return 0; } diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 80b668c80073..c9e4190316ef 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -49,12 +49,6 @@ static DEFINE_MUTEX(acpi_hp_context_lock); */ static u64 spcr_uart_addr; -struct acpi_dep_data { - struct list_head node; - acpi_handle supplier; - acpi_handle consumer; -}; - void acpi_scan_lock_acquire(void) { mutex_lock(&acpi_scan_lock); @@ -2099,30 +2093,58 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass) device->handler->hotplug.notify_online(device); } -void acpi_walk_dep_device_list(acpi_handle handle) +static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep, + void *data) { - struct acpi_dep_data *dep, *tmp; struct acpi_device *adev; + acpi_bus_get_device(dep->consumer, &adev); + if (!adev) + return 0; + + adev->dep_unmet--; + if (!adev->dep_unmet) + acpi_bus_attach(adev, true); + + list_del(&dep->node); + kfree(dep); + return 0; +} + +void acpi_walk_dep_device_list(acpi_handle handle, + int (*callback)(struct acpi_dep_data *, void *), + void *data) +{ + struct acpi_dep_data *dep, *tmp; + int ret; + mutex_lock(&acpi_dep_list_lock); list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) { if (dep->supplier == handle) { - acpi_bus_get_device(dep->consumer, &adev); - if (!adev) - continue; - - adev->dep_unmet--; - if (!adev->dep_unmet) - acpi_bus_attach(adev, true); - - list_del(&dep->node); - kfree(dep); + ret = callback(dep, data); + if (ret) + break; } } mutex_unlock(&acpi_dep_list_lock); } EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list); +/** + * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the device + * is now active + * @handle: acpi_handle for the supplier device + * + * This function walks through the dependencies list and informs each consumer + * of @handle that their dependency upon it is now met. Devices with no more + * unmet dependencies will be attached to the acpi bus. + */ +void acpi_dev_flag_dependency_met(acpi_handle handle) +{ + acpi_walk_dep_device_list(handle, __acpi_dev_flag_dependency_met, NULL); +} +EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met); + /** * acpi_bus_scan - Add ACPI device node objects in a given namespace scope. * @handle: Root of the namespace scope to scan. diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index e37a57d0a2f0..e4d728fda982 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -1254,7 +1254,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip) acpi_gpiochip_request_regions(acpi_gpio); acpi_gpiochip_scan_gpios(acpi_gpio); - acpi_walk_dep_device_list(handle); + acpi_dev_flag_dependency_met(handle); } void acpi_gpiochip_remove(struct gpio_chip *chip) diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c index 37c510d9347a..38647cf34bde 100644 --- a/drivers/i2c/i2c-core-acpi.c +++ b/drivers/i2c/i2c-core-acpi.c @@ -283,7 +283,7 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap) if (!handle) return; - acpi_walk_dep_device_list(handle); + acpi_dev_flag_dependency_met(handle); } static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = { diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c index cc4f9cba6856..ad895285d3e9 100644 --- a/drivers/platform/surface/surface3_power.c +++ b/drivers/platform/surface/surface3_power.c @@ -478,7 +478,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client) return -ENOMEM; } - acpi_walk_dep_device_list(handle); + acpi_dev_flag_dependency_met(handle); return 0; } diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 02a716a0af5d..91172af3a04d 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -278,6 +278,12 @@ struct acpi_device_power { struct acpi_device_power_state states[ACPI_D_STATE_COUNT]; /* Power states (D0-D3Cold) */ }; +struct acpi_dep_data { + struct list_head node; + acpi_handle supplier; + acpi_handle consumer; +}; + /* Performance Management */ struct acpi_device_perf_flags { @@ -683,6 +689,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_flag_dependency_met(acpi_handle handle); struct acpi_device * acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); struct acpi_device * diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 2630c2e953f7..2d5e6e88e8a0 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -655,7 +655,9 @@ extern bool acpi_driver_match_device(struct device *dev, const struct device_driver *drv); int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); int acpi_device_modalias(struct device *, char *, int); -void acpi_walk_dep_device_list(acpi_handle handle); +void acpi_walk_dep_device_list(acpi_handle handle, + int (*callback)(struct acpi_dep_data *, void *), + void *data); struct platform_device *acpi_create_platform_device(struct acpi_device *, struct property_entry *);
The acpi_walk_dep_device_list() is not as generalisable as its name implies, serving only to decrement the dependency count for each dependent device of the input. Extend the function to instead accept a callback which can be applied to all the dependencies in acpi_dep_list. Replace all existing calls to the function with calls to a wrapper, passing a callback that applies the same dependency reduction. Suggested-by: Rafael J. Wysocki <rafael@kernel.org> Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v3: - patch introduced drivers/acpi/ec.c | 2 +- drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +- drivers/acpi/scan.c | 58 ++++++++++++++++------- drivers/gpio/gpiolib-acpi.c | 2 +- drivers/i2c/i2c-core-acpi.c | 2 +- drivers/platform/surface/surface3_power.c | 2 +- include/acpi/acpi_bus.h | 7 +++ include/linux/acpi.h | 4 +- 8 files changed, 55 insertions(+), 24 deletions(-)