Hi All,
Confusingly enough, the ACPI subsystem stores the information on the given ACPI
device's children in two places: as the list of children in struct acpi_device
and (as a result of device registration) in the list of children in the embedded
struct device.
These two lists agree with each other most of the time, but not always (like in
error paths in some cases), and the list of children in struct acpi_device is
not generally safe to use without locking. In principle, it should always be
walked under acpi_device_lock, but in practice holding acpi_scan_lock is
sufficient for that too. However, its users may not know whether or not
they operate under acpi_scan_lock and at least in some cases it is not accessed
in a safe way (note that ACPI devices may go away as a result of hot-remove,
unlike OF nodes).
For this reason, it is better to consolidate the code that needs to walk the
children of an ACPI device which is the purpose of this patch series.
Overall, it switches over all of the users of the list of children in struct
acpi_device to using helpers based on the driver core's mechanics and finally
drops that list, but some extra cleanups are done on the way.
Please refer to the patch changelogs for details.
Thanks!
From: Rafael J. Wysocki <[email protected]>
Drop the children and node list heads that have no more users
from struct acpi_device and the code manipulating them from
__acpi_device_add() and acpi_device_del().
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/scan.c | 11 +----------
include/acpi/acpi_bus.h | 2 --
2 files changed, 1 insertion(+), 12 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -365,8 +365,6 @@ struct acpi_device {
acpi_handle handle; /* no handle for fixed hardware */
struct fwnode_handle fwnode;
struct acpi_device *parent;
- struct list_head children;
- struct list_head node;
struct list_head wakeup_list;
struct list_head del_list;
struct acpi_device_status status;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -465,8 +465,6 @@ static void acpi_device_del(struct acpi_
struct acpi_device_bus_id *acpi_device_bus_id;
mutex_lock(&acpi_device_lock);
- if (device->parent)
- list_del(&device->node);
list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node)
if (!strcmp(acpi_device_bus_id->bus_id,
@@ -482,6 +480,7 @@ static void acpi_device_del(struct acpi_
}
list_del(&device->wakeup_list);
+
mutex_unlock(&acpi_device_lock);
acpi_power_add_remove_device(device, false);
@@ -674,8 +673,6 @@ static int __acpi_device_add(struct acpi
* -------
* Link this device to its parent and siblings.
*/
- INIT_LIST_HEAD(&device->children);
- INIT_LIST_HEAD(&device->node);
INIT_LIST_HEAD(&device->wakeup_list);
INIT_LIST_HEAD(&device->physical_node_list);
INIT_LIST_HEAD(&device->del_list);
@@ -715,9 +712,6 @@ static int __acpi_device_add(struct acpi
list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
}
- if (device->parent)
- list_add_tail(&device->node, &device->parent->children);
-
if (device->wakeup.flags.valid)
list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
@@ -746,9 +740,6 @@ static int __acpi_device_add(struct acpi
err:
mutex_lock(&acpi_device_lock);
- if (device->parent)
- list_del(&device->node);
-
list_del(&device->wakeup_list);
err_unlock:
On Thu, Jun 09, 2022 at 03:44:27PM +0200, Rafael J. Wysocki wrote:
> Hi All,
>
> Confusingly enough, the ACPI subsystem stores the information on the given ACPI
> device's children in two places: as the list of children in struct acpi_device
> and (as a result of device registration) in the list of children in the embedded
> struct device.
>
> These two lists agree with each other most of the time, but not always (like in
> error paths in some cases), and the list of children in struct acpi_device is
> not generally safe to use without locking. In principle, it should always be
> walked under acpi_device_lock, but in practice holding acpi_scan_lock is
> sufficient for that too. However, its users may not know whether or not
> they operate under acpi_scan_lock and at least in some cases it is not accessed
> in a safe way (note that ACPI devices may go away as a result of hot-remove,
> unlike OF nodes).
Hmm... Does it true for DT overlays? Not an expert in DT overlays, though,
adding Rob and Frank.
> For this reason, it is better to consolidate the code that needs to walk the
> children of an ACPI device which is the purpose of this patch series.
>
> Overall, it switches over all of the users of the list of children in struct
> acpi_device to using helpers based on the driver core's mechanics and finally
> drops that list, but some extra cleanups are done on the way.
>
> Please refer to the patch changelogs for details.
I'm going to look the individual patches.
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 9, 2022 at 5:57 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Jun 09, 2022 at 03:44:27PM +0200, Rafael J. Wysocki wrote:
> > Hi All,
> >
> > Confusingly enough, the ACPI subsystem stores the information on the given ACPI
> > device's children in two places: as the list of children in struct acpi_device
> > and (as a result of device registration) in the list of children in the embedded
> > struct device.
> >
> > These two lists agree with each other most of the time, but not always (like in
> > error paths in some cases), and the list of children in struct acpi_device is
> > not generally safe to use without locking. In principle, it should always be
> > walked under acpi_device_lock, but in practice holding acpi_scan_lock is
> > sufficient for that too. However, its users may not know whether or not
> > they operate under acpi_scan_lock and at least in some cases it is not accessed
> > in a safe way (note that ACPI devices may go away as a result of hot-remove,
> > unlike OF nodes).
> >
> > For this reason, it is better to consolidate the code that needs to walk the
> > children of an ACPI device which is the purpose of this patch series.
> >
> > Overall, it switches over all of the users of the list of children in struct
> > acpi_device to using helpers based on the driver core's mechanics and finally
> > drops that list, but some extra cleanups are done on the way.
> >
> > Please refer to the patch changelogs for details.
>
> Cool series, thanks for doing that!
>
> You may add my
> Revieweed-by: Andy Shevchenko <[email protected]>
> to all non-commented, by me, patches (excluding soundwire) and to ones
> where comment just about one line/two lines split (address them if you
> are okay, otherwise ignore those comments).
Thank you!
On Thu, Jun 09, 2022 at 03:44:27PM +0200, Rafael J. Wysocki wrote:
> Hi All,
>
> Confusingly enough, the ACPI subsystem stores the information on the given ACPI
> device's children in two places: as the list of children in struct acpi_device
> and (as a result of device registration) in the list of children in the embedded
> struct device.
>
> These two lists agree with each other most of the time, but not always (like in
> error paths in some cases), and the list of children in struct acpi_device is
> not generally safe to use without locking. In principle, it should always be
> walked under acpi_device_lock, but in practice holding acpi_scan_lock is
> sufficient for that too. However, its users may not know whether or not
> they operate under acpi_scan_lock and at least in some cases it is not accessed
> in a safe way (note that ACPI devices may go away as a result of hot-remove,
> unlike OF nodes).
>
> For this reason, it is better to consolidate the code that needs to walk the
> children of an ACPI device which is the purpose of this patch series.
>
> Overall, it switches over all of the users of the list of children in struct
> acpi_device to using helpers based on the driver core's mechanics and finally
> drops that list, but some extra cleanups are done on the way.
>
> Please refer to the patch changelogs for details.
Cool series, thanks for doing that!
You may add my
Revieweed-by: Andy Shevchenko <[email protected]>
to all non-commented, by me, patches (excluding soundwire) and to ones
where comment just about one line/two lines split (address them if you
are okay, otherwise ignore those comments).
--
With Best Regards,
Andy Shevchenko
On 6/9/22 11:12, Andy Shevchenko wrote:
> On Thu, Jun 09, 2022 at 03:44:27PM +0200, Rafael J. Wysocki wrote:
>> Hi All,
>>
>> Confusingly enough, the ACPI subsystem stores the information on the given ACPI
>> device's children in two places: as the list of children in struct acpi_device
>> and (as a result of device registration) in the list of children in the embedded
>> struct device.
>>
>> These two lists agree with each other most of the time, but not always (like in
>> error paths in some cases), and the list of children in struct acpi_device is
>> not generally safe to use without locking. In principle, it should always be
>> walked under acpi_device_lock, but in practice holding acpi_scan_lock is
>> sufficient for that too. However, its users may not know whether or not
>> they operate under acpi_scan_lock and at least in some cases it is not accessed
>> in a safe way (note that ACPI devices may go away as a result of hot-remove,
>
>> unlike OF nodes).
>
> Hmm... Does it true for DT overlays? Not an expert in DT overlays, though,
> adding Rob and Frank.
DT nodes can be removed. The devicetree code uses devtree_lock and of_mutex
as needed for protection.
-Frank
>
>> For this reason, it is better to consolidate the code that needs to walk the
>> children of an ACPI device which is the purpose of this patch series.
>>
>> Overall, it switches over all of the users of the list of children in struct
>> acpi_device to using helpers based on the driver core's mechanics and finally
>> drops that list, but some extra cleanups are done on the way.
>>
>> Please refer to the patch changelogs for details.
>
> I'm going to look the individual patches.
>
On Thursday, June 9, 2022 3:44:27 PM CEST Rafael J. Wysocki wrote:
> Hi All,
>
> Confusingly enough, the ACPI subsystem stores the information on the given ACPI
> device's children in two places: as the list of children in struct acpi_device
> and (as a result of device registration) in the list of children in the embedded
> struct device.
>
> These two lists agree with each other most of the time, but not always (like in
> error paths in some cases), and the list of children in struct acpi_device is
> not generally safe to use without locking. In principle, it should always be
> walked under acpi_device_lock, but in practice holding acpi_scan_lock is
> sufficient for that too. However, its users may not know whether or not
> they operate under acpi_scan_lock and at least in some cases it is not accessed
> in a safe way (note that ACPI devices may go away as a result of hot-remove,
> unlike OF nodes).
>
> For this reason, it is better to consolidate the code that needs to walk the
> children of an ACPI device which is the purpose of this patch series.
>
> Overall, it switches over all of the users of the list of children in struct
> acpi_device to using helpers based on the driver core's mechanics and finally
> drops that list, but some extra cleanups are done on the way.
>
> Please refer to the patch changelogs for details.
Here's a v2 of this series, mostly addressing review comments, but the subjects
of the Thunderbolt and USB patches have been changed too.
Thanks!
From: Rafael J. Wysocki <[email protected]>
Drop the children and node list heads that have no more users
from struct acpi_device and the code manipulating them from
__acpi_device_add() and acpi_device_del().
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
v1 -> v2:
* Add R-by from Andy.
---
drivers/acpi/scan.c | 11 +----------
include/acpi/acpi_bus.h | 2 --
2 files changed, 1 insertion(+), 12 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -365,8 +365,6 @@ struct acpi_device {
acpi_handle handle; /* no handle for fixed hardware */
struct fwnode_handle fwnode;
struct acpi_device *parent;
- struct list_head children;
- struct list_head node;
struct list_head wakeup_list;
struct list_head del_list;
struct acpi_device_status status;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -465,8 +465,6 @@ static void acpi_device_del(struct acpi_
struct acpi_device_bus_id *acpi_device_bus_id;
mutex_lock(&acpi_device_lock);
- if (device->parent)
- list_del(&device->node);
list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node)
if (!strcmp(acpi_device_bus_id->bus_id,
@@ -482,6 +480,7 @@ static void acpi_device_del(struct acpi_
}
list_del(&device->wakeup_list);
+
mutex_unlock(&acpi_device_lock);
acpi_power_add_remove_device(device, false);
@@ -674,8 +673,6 @@ static int __acpi_device_add(struct acpi
* -------
* Link this device to its parent and siblings.
*/
- INIT_LIST_HEAD(&device->children);
- INIT_LIST_HEAD(&device->node);
INIT_LIST_HEAD(&device->wakeup_list);
INIT_LIST_HEAD(&device->physical_node_list);
INIT_LIST_HEAD(&device->del_list);
@@ -715,9 +712,6 @@ static int __acpi_device_add(struct acpi
list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
}
- if (device->parent)
- list_add_tail(&device->node, &device->parent->children);
-
if (device->wakeup.flags.valid)
list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
@@ -746,9 +740,6 @@ static int __acpi_device_add(struct acpi
err:
mutex_lock(&acpi_device_lock);
- if (device->parent)
- list_del(&device->node);
-
list_del(&device->wakeup_list);
err_unlock:
From: Rafael J. Wysocki <[email protected]>
Some pieces of modular code can benefit from using
acpi_dev_for_each_child(), so export it to modules.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
v1 -> v2:
* Add R-by from Andy.
---
drivers/acpi/bus.c | 1 +
1 file changed, 1 insertion(+)
Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -1102,6 +1102,7 @@ static int acpi_dev_for_one_check(struct
return adwc->fn(to_acpi_device(dev), adwc->data);
}
+EXPORT_SYMBOL_GPL(acpi_dev_for_each_child);
int acpi_dev_for_each_child(struct acpi_device *adev,
int (*fn)(struct acpi_device *, void *), void *data)
From: Rafael J. Wysocki <[email protected]>
Instead of walking the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to carry out an action for all of
the given ACPI device's children.
This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[email protected]>
Tested-by: Pierre-Louis Bossart <[email protected]>
---
v1 -> v2:
* Make sure errors are not lost (Pierre-Louis).
* Add R-by and T-by from Pierre-Louis.
---
drivers/soundwire/slave.c | 117 ++++++++++++++++++++++++++--------------------
1 file changed, 67 insertions(+), 50 deletions(-)
Index: linux-pm/drivers/soundwire/slave.c
===================================================================
--- linux-pm.orig/drivers/soundwire/slave.c
+++ linux-pm/drivers/soundwire/slave.c
@@ -127,6 +127,71 @@ static bool find_slave(struct sdw_bus *b
return true;
}
+struct sdw_acpi_child_walk_data {
+ struct sdw_bus *bus;
+ struct acpi_device *adev;
+ struct sdw_slave_id id;
+ bool ignore_unique_id;
+};
+
+static int sdw_acpi_check_duplicate(struct acpi_device *adev, void *data)
+{
+ struct sdw_acpi_child_walk_data *cwd = data;
+ struct sdw_bus *bus = cwd->bus;
+ struct sdw_slave_id id;
+
+ if (adev == cwd->adev)
+ return 0;
+
+ if (!find_slave(bus, adev, &id))
+ return 0;
+
+ if (cwd->id.sdw_version != id.sdw_version || cwd->id.mfg_id != id.mfg_id ||
+ cwd->id.part_id != id.part_id || cwd->id.class_id != id.class_id)
+ return 0;
+
+ if (cwd->id.unique_id != id.unique_id) {
+ dev_dbg(bus->dev,
+ "Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
+ cwd->id.unique_id, id.unique_id, cwd->id.mfg_id,
+ cwd->id.part_id);
+ cwd->ignore_unique_id = false;
+ return 0;
+ }
+
+ dev_err(bus->dev,
+ "Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
+ cwd->id.unique_id, id.unique_id, cwd->id.mfg_id, cwd->id.part_id);
+ return -ENODEV;
+}
+
+static int sdw_acpi_find_one(struct acpi_device *adev, void *data)
+{
+ struct sdw_bus *bus = data;
+ struct sdw_acpi_child_walk_data cwd = {
+ .bus = bus,
+ .adev = adev,
+ .ignore_unique_id = true,
+ };
+ int ret;
+
+ if (!find_slave(bus, adev, &cwd.id))
+ return 0;
+
+ /* Brute-force O(N^2) search for duplicates. */
+ ret = acpi_dev_for_each_child(ACPI_COMPANION(bus->dev),
+ sdw_acpi_check_duplicate, &cwd);
+ if (ret)
+ return ret;
+
+ if (cwd.ignore_unique_id)
+ cwd.id.unique_id = SDW_IGNORED_UNIQUE_ID;
+
+ /* Ignore errors and continue. */
+ sdw_slave_add(bus, &cwd.id, acpi_fwnode_handle(adev));
+ return 0;
+}
+
/*
* sdw_acpi_find_slaves() - Find Slave devices in Master ACPI node
* @bus: SDW bus instance
@@ -135,8 +200,7 @@ static bool find_slave(struct sdw_bus *b
*/
int sdw_acpi_find_slaves(struct sdw_bus *bus)
{
- struct acpi_device *adev, *parent;
- struct acpi_device *adev2, *parent2;
+ struct acpi_device *parent;
parent = ACPI_COMPANION(bus->dev);
if (!parent) {
@@ -144,54 +208,7 @@ int sdw_acpi_find_slaves(struct sdw_bus
return -ENODEV;
}
- list_for_each_entry(adev, &parent->children, node) {
- struct sdw_slave_id id;
- struct sdw_slave_id id2;
- bool ignore_unique_id = true;
-
- if (!find_slave(bus, adev, &id))
- continue;
-
- /* brute-force O(N^2) search for duplicates */
- parent2 = parent;
- list_for_each_entry(adev2, &parent2->children, node) {
-
- if (adev == adev2)
- continue;
-
- if (!find_slave(bus, adev2, &id2))
- continue;
-
- if (id.sdw_version != id2.sdw_version ||
- id.mfg_id != id2.mfg_id ||
- id.part_id != id2.part_id ||
- id.class_id != id2.class_id)
- continue;
-
- if (id.unique_id != id2.unique_id) {
- dev_dbg(bus->dev,
- "Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
- id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
- ignore_unique_id = false;
- } else {
- dev_err(bus->dev,
- "Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
- id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
- return -ENODEV;
- }
- }
-
- if (ignore_unique_id)
- id.unique_id = SDW_IGNORED_UNIQUE_ID;
-
- /*
- * don't error check for sdw_slave_add as we want to continue
- * adding Slaves
- */
- sdw_slave_add(bus, &id, acpi_fwnode_handle(adev));
- }
-
- return 0;
+ return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
}
#endif
From: Rafael J. Wysocki <[email protected]>
Instead of walking the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to carry out an action for all of
the given ACPI device's children.
This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
v1 -> v2:
* Eliminate unnecessary branch (Andy).
---
drivers/platform/x86/thinkpad_acpi.c | 53 +++++++++++++++++------------------
1 file changed, 27 insertions(+), 26 deletions(-)
Index: linux-pm/drivers/platform/x86/thinkpad_acpi.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/thinkpad_acpi.c
+++ linux-pm/drivers/platform/x86/thinkpad_acpi.c
@@ -6841,6 +6841,31 @@ static const struct backlight_ops ibm_ba
/* --------------------------------------------------------------------- */
+static int __init tpacpi_evaluate_bcl(struct acpi_device *adev, void *not_used)
+{
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *obj;
+ acpi_status status;
+ int rc;
+
+ status = acpi_evaluate_object(adev->handle, "_BCL", NULL, &buffer);
+ if (ACPI_FAILURE(status))
+ return 0;
+
+ obj = buffer.pointer;
+ if (!obj || obj->type != ACPI_TYPE_PACKAGE) {
+ acpi_handle_info(adev->handle,
+ "Unknown _BCL data, please report this to %s\n",
+ TPACPI_MAIL);
+ rc = 0;
+ } else {
+ rc = obj->package.count;
+ }
+ kfree(obj);
+
+ return rc;
+}
+
/*
* Call _BCL method of video device. On some ThinkPads this will
* switch the firmware to the ACPI brightness control mode.
@@ -6848,37 +6873,13 @@ static const struct backlight_ops ibm_ba
static int __init tpacpi_query_bcl_levels(acpi_handle handle)
{
- struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
- union acpi_object *obj;
- struct acpi_device *device, *child;
- int rc;
+ struct acpi_device *device;
device = acpi_fetch_acpi_dev(handle);
if (!device)
return 0;
- rc = 0;
- list_for_each_entry(child, &device->children, node) {
- acpi_status status = acpi_evaluate_object(child->handle, "_BCL",
- NULL, &buffer);
- if (ACPI_FAILURE(status)) {
- buffer.length = ACPI_ALLOCATE_BUFFER;
- continue;
- }
-
- obj = (union acpi_object *)buffer.pointer;
- if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
- pr_err("Unknown _BCL data, please report this to %s\n",
- TPACPI_MAIL);
- rc = 0;
- } else {
- rc = obj->package.count;
- }
- break;
- }
-
- kfree(buffer.pointer);
- return rc;
+ return acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL);
}
From: Rafael J. Wysocki <[email protected]>
Instead of walking the list of children of an ACPI device directly, use
acpi_dev_for_each_child() or acpi_dev_for_each_child_reverse() to carry
out an action for all of the given ACPI device's children.
This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
v1 -> v2:
* Add R-by from Andy.
---
drivers/acpi/scan.c | 59 +++++++++++++++++++++++++---------------------------
1 file changed, 29 insertions(+), 30 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -334,10 +334,9 @@ static int acpi_scan_device_check(struct
return error;
}
-static int acpi_scan_bus_check(struct acpi_device *adev)
+static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
{
struct acpi_scan_handler *handler = adev->handler;
- struct acpi_device *child;
int error;
acpi_bus_get_status(adev);
@@ -353,19 +352,14 @@ static int acpi_scan_bus_check(struct ac
dev_warn(&adev->dev, "Namespace scan failure\n");
return error;
}
- list_for_each_entry(child, &adev->children, node) {
- error = acpi_scan_bus_check(child);
- if (error)
- return error;
- }
- return 0;
+ return acpi_dev_for_each_child(adev, acpi_scan_bus_check, NULL);
}
static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
{
switch (type) {
case ACPI_NOTIFY_BUS_CHECK:
- return acpi_scan_bus_check(adev);
+ return acpi_scan_bus_check(adev, NULL);
case ACPI_NOTIFY_DEVICE_CHECK:
return acpi_scan_device_check(adev);
case ACPI_NOTIFY_EJECT_REQUEST:
@@ -2187,9 +2181,8 @@ static int acpi_scan_attach_handler(stru
return ret;
}
-static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
+static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
{
- struct acpi_device *child;
bool skip = !first_pass && device->flags.visited;
acpi_handle ejd;
int ret;
@@ -2206,7 +2199,7 @@ static void acpi_bus_attach(struct acpi_
device->flags.initialized = false;
acpi_device_clear_enumerated(device);
device->flags.power_manageable = 0;
- return;
+ return 0;
}
if (device->handler)
goto ok;
@@ -2224,7 +2217,7 @@ static void acpi_bus_attach(struct acpi_
ret = acpi_scan_attach_handler(device);
if (ret < 0)
- return;
+ return 0;
device->flags.match_driver = true;
if (ret > 0 && !device->flags.enumeration_by_parent) {
@@ -2234,19 +2227,20 @@ static void acpi_bus_attach(struct acpi_
ret = device_attach(&device->dev);
if (ret < 0)
- return;
+ return 0;
if (device->pnp.type.platform_id || device->flags.enumeration_by_parent)
acpi_default_enumeration(device);
else
acpi_device_set_enumerated(device);
- ok:
- list_for_each_entry(child, &device->children, node)
- acpi_bus_attach(child, first_pass);
+ok:
+ acpi_dev_for_each_child(device, acpi_bus_attach, first_pass);
if (!skip && device->handler && device->handler->hotplug.notify_online)
device->handler->hotplug.notify_online(device);
+
+ return 0;
}
static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
@@ -2274,7 +2268,7 @@ static void acpi_scan_clear_dep_fn(struc
cdw = container_of(work, struct acpi_scan_clear_dep_work, work);
acpi_scan_lock_acquire();
- acpi_bus_attach(cdw->adev, true);
+ acpi_bus_attach(cdw->adev, (void *)true);
acpi_scan_lock_release();
acpi_dev_put(cdw->adev);
@@ -2432,7 +2426,7 @@ int acpi_bus_scan(acpi_handle handle)
if (!device)
return -ENODEV;
- acpi_bus_attach(device, true);
+ acpi_bus_attach(device, (void *)true);
if (!acpi_bus_scan_second_pass)
return 0;
@@ -2446,25 +2440,17 @@ int acpi_bus_scan(acpi_handle handle)
acpi_bus_check_add_2, NULL, NULL,
(void **)&device);
- acpi_bus_attach(device, false);
+ acpi_bus_attach(device, NULL);
return 0;
}
EXPORT_SYMBOL(acpi_bus_scan);
-/**
- * acpi_bus_trim - Detach scan handlers and drivers from ACPI device objects.
- * @adev: Root of the ACPI namespace scope to walk.
- *
- * Must be called under acpi_scan_lock.
- */
-void acpi_bus_trim(struct acpi_device *adev)
+int acpi_bus_trim_one(struct acpi_device *adev, void *not_used)
{
struct acpi_scan_handler *handler = adev->handler;
- struct acpi_device *child;
- list_for_each_entry_reverse(child, &adev->children, node)
- acpi_bus_trim(child);
+ acpi_dev_for_each_child_reverse(adev, acpi_bus_trim_one, NULL);
adev->flags.match_driver = false;
if (handler) {
@@ -2482,6 +2468,19 @@ void acpi_bus_trim(struct acpi_device *a
acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
adev->flags.initialized = false;
acpi_device_clear_enumerated(adev);
+
+ return 0;
+}
+
+/**
+ * acpi_bus_trim - Detach scan handlers and drivers from ACPI device objects.
+ * @adev: Root of the ACPI namespace scope to walk.
+ *
+ * Must be called under acpi_scan_lock.
+ */
+void acpi_bus_trim(struct acpi_device *adev)
+{
+ acpi_bus_trim_one(adev, NULL);
}
EXPORT_SYMBOL_GPL(acpi_bus_trim);
On Mon, Jun 13, 2022 at 08:30:19PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Instead of walking the list of children of an ACPI device directly,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.
>
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).
Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> v1 -> v2:
> * Eliminate unnecessary branch (Andy).
>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 53 +++++++++++++++++------------------
> 1 file changed, 27 insertions(+), 26 deletions(-)
>
> Index: linux-pm/drivers/platform/x86/thinkpad_acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/thinkpad_acpi.c
> +++ linux-pm/drivers/platform/x86/thinkpad_acpi.c
> @@ -6841,6 +6841,31 @@ static const struct backlight_ops ibm_ba
>
> /* --------------------------------------------------------------------- */
>
> +static int __init tpacpi_evaluate_bcl(struct acpi_device *adev, void *not_used)
> +{
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + acpi_status status;
> + int rc;
> +
> + status = acpi_evaluate_object(adev->handle, "_BCL", NULL, &buffer);
> + if (ACPI_FAILURE(status))
> + return 0;
> +
> + obj = buffer.pointer;
> + if (!obj || obj->type != ACPI_TYPE_PACKAGE) {
> + acpi_handle_info(adev->handle,
> + "Unknown _BCL data, please report this to %s\n",
> + TPACPI_MAIL);
> + rc = 0;
> + } else {
> + rc = obj->package.count;
> + }
> + kfree(obj);
> +
> + return rc;
> +}
> +
> /*
> * Call _BCL method of video device. On some ThinkPads this will
> * switch the firmware to the ACPI brightness control mode.
> @@ -6848,37 +6873,13 @@ static const struct backlight_ops ibm_ba
>
> static int __init tpacpi_query_bcl_levels(acpi_handle handle)
> {
> - struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> - struct acpi_device *device, *child;
> - int rc;
> + struct acpi_device *device;
>
> device = acpi_fetch_acpi_dev(handle);
> if (!device)
> return 0;
>
> - rc = 0;
> - list_for_each_entry(child, &device->children, node) {
> - acpi_status status = acpi_evaluate_object(child->handle, "_BCL",
> - NULL, &buffer);
> - if (ACPI_FAILURE(status)) {
> - buffer.length = ACPI_ALLOCATE_BUFFER;
> - continue;
> - }
> -
> - obj = (union acpi_object *)buffer.pointer;
> - if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
> - pr_err("Unknown _BCL data, please report this to %s\n",
> - TPACPI_MAIL);
> - rc = 0;
> - } else {
> - rc = obj->package.count;
> - }
> - break;
> - }
> -
> - kfree(buffer.pointer);
> - return rc;
> + return acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL);
> }
>
>
>
>
>
--
With Best Regards,
Andy Shevchenko
From: Rafael J. Wysocki <[email protected]>
Instead of walking the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to carry out an action for all of
the given ACPI device's children.
This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
v1 -> v2:
* Do not break the acpi_dev_for_each_child() call line (Andy).
* Add R-by from Andy.
---
drivers/acpi/container.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
Index: linux-pm/drivers/acpi/container.c
===================================================================
--- linux-pm.orig/drivers/acpi/container.c
+++ linux-pm/drivers/acpi/container.c
@@ -23,17 +23,18 @@ static const struct acpi_device_id conta
#ifdef CONFIG_ACPI_CONTAINER
-static int acpi_container_offline(struct container_dev *cdev)
+static int check_offline(struct acpi_device *adev, void *not_used)
{
- struct acpi_device *adev = ACPI_COMPANION(&cdev->dev);
- struct acpi_device *child;
+ if (acpi_scan_is_offline(adev, false))
+ return 0;
- /* Check all of the dependent devices' physical companions. */
- list_for_each_entry(child, &adev->children, node)
- if (!acpi_scan_is_offline(child, false))
- return -EBUSY;
+ return -EBUSY;
+}
- return 0;
+static int acpi_container_offline(struct container_dev *cdev)
+{
+ /* Check all of the dependent devices' physical companions. */
+ return acpi_dev_for_each_child(ACPI_COMPANION(&cdev->dev), check_offline, NULL);
}
static void acpi_container_release(struct device *dev)
From: Rafael J. Wysocki <[email protected]>
Instead of walking the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to carry out an action for all of
the given ACPI device's children.
This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
v1 -> v2:
* Add R-by from Andy.
---
drivers/acpi/glue.c | 103 +++++++++++++++++++++++++++++++---------------------
1 file changed, 63 insertions(+), 40 deletions(-)
Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -105,51 +105,74 @@ static int find_child_checks(struct acpi
return FIND_CHILD_MAX_SCORE;
}
+struct find_child_walk_data {
+ struct acpi_device *adev;
+ u64 address;
+ int score;
+ bool check_children;
+};
+
+static int check_one_child(struct acpi_device *adev, void *data)
+{
+ struct find_child_walk_data *wd = data;
+ int score;
+
+ if (!adev->pnp.type.bus_address || acpi_device_adr(adev) != wd->address)
+ return 0;
+
+ if (!wd->adev) {
+ /* This is the first matching object. Save it and continue. */
+ wd->adev = adev;
+ return 0;
+ }
+
+ /*
+ * There is more than one matching device object with the same _ADR
+ * value. That really is unexpected, so we are kind of beyond the scope
+ * of the spec here. We have to choose which one to return, though.
+ *
+ * First, get the score for the previously found object and terminate
+ * the walk if it is maximum.
+ */
+ if (!wd->score) {
+ score = find_child_checks(wd->adev, wd->check_children);
+ if (score == FIND_CHILD_MAX_SCORE)
+ return 1;
+
+ wd->score = score;
+ }
+ /*
+ * Second, if the object that has just been found has a better score,
+ * replace the previously found one with it and terminate the walk if
+ * the new score is maximum.
+ */
+ score = find_child_checks(adev, wd->check_children);
+ if (score > wd->score) {
+ wd->adev = adev;
+ if (score == FIND_CHILD_MAX_SCORE)
+ return 1;
+
+ wd->score = score;
+ }
+
+ /* Continue, because there may be better matches. */
+ return 0;
+}
+
struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
u64 address, bool check_children)
{
- struct acpi_device *adev, *ret = NULL;
- int ret_score = 0;
+ struct find_child_walk_data wd = {
+ .address = address,
+ .check_children = check_children,
+ .adev = NULL,
+ .score = 0,
+ };
- if (!parent)
- return NULL;
+ if (parent)
+ acpi_dev_for_each_child(parent, check_one_child, &wd);
- list_for_each_entry(adev, &parent->children, node) {
- acpi_bus_address addr = acpi_device_adr(adev);
- int score;
-
- if (!adev->pnp.type.bus_address || addr != address)
- continue;
-
- if (!ret) {
- /* This is the first matching object. Save it. */
- ret = adev;
- continue;
- }
- /*
- * There is more than one matching device object with the same
- * _ADR value. That really is unexpected, so we are kind of
- * beyond the scope of the spec here. We have to choose which
- * one to return, though.
- *
- * First, check if the previously found object is good enough
- * and return it if so. Second, do the same for the object that
- * we've just found.
- */
- if (!ret_score) {
- ret_score = find_child_checks(ret, check_children);
- if (ret_score == FIND_CHILD_MAX_SCORE)
- return ret;
- }
- score = find_child_checks(adev, check_children);
- if (score == FIND_CHILD_MAX_SCORE) {
- return adev;
- } else if (score > ret_score) {
- ret = adev;
- ret_score = score;
- }
- }
- return ret;
+ return wd.adev;
}
EXPORT_SYMBOL_GPL(acpi_find_child_device);
From: Rafael J. Wysocki <[email protected]>
Rearrange the ACPI device lookup code used internally by
acpi_find_child_device() so it can avoid extra checks after finding
one object with a matching _ADR and use it for defining
acpi_find_child_by_adr() that will allow the callers to find a given
ACPI device's child matching a given bus address without doing any
other checks in check_one_child().
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
v1 -> v2:
* Add R-by from Andy.
---
drivers/acpi/glue.c | 28 ++++++++++++++++++++++++----
include/acpi/acpi_bus.h | 2 ++
2 files changed, 26 insertions(+), 4 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -622,6 +622,8 @@ static inline int acpi_dma_configure(str
}
struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
u64 address, bool check_children);
+struct acpi_device *acpi_find_child_by_adr(struct acpi_device *adev,
+ acpi_bus_address adr);
int acpi_is_root_bridge(acpi_handle);
struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);
Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -119,6 +119,7 @@ struct find_child_walk_data {
struct acpi_device *adev;
u64 address;
int score;
+ bool check_sta;
bool check_children;
};
@@ -131,9 +132,13 @@ static int check_one_child(struct acpi_d
return 0;
if (!wd->adev) {
- /* This is the first matching object. Save it and continue. */
+ /*
+ * This is the first matching object, so save it. If it is not
+ * necessary to look for any other matching objects, stop the
+ * search.
+ */
wd->adev = adev;
- return 0;
+ return !(wd->check_sta || wd->check_children);
}
/*
@@ -169,12 +174,14 @@ static int check_one_child(struct acpi_d
return 0;
}
-struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
- u64 address, bool check_children)
+static struct acpi_device *acpi_find_child(struct acpi_device *parent,
+ u64 address, bool check_children,
+ bool check_sta)
{
struct find_child_walk_data wd = {
.address = address,
.check_children = check_children,
+ .check_sta = check_sta,
.adev = NULL,
.score = 0,
};
@@ -184,8 +191,21 @@ struct acpi_device *acpi_find_child_devi
return wd.adev;
}
+
+struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
+ u64 address, bool check_children)
+{
+ return acpi_find_child(parent, address, check_children, true);
+}
EXPORT_SYMBOL_GPL(acpi_find_child_device);
+struct acpi_device *acpi_find_child_by_adr(struct acpi_device *adev,
+ acpi_bus_address adr)
+{
+ return acpi_find_child(adev, adr, false, false);
+}
+EXPORT_SYMBOL_GPL(acpi_find_child_by_adr);
+
static void acpi_physnode_link_name(char *buf, unsigned int node_id)
{
if (node_id > 0)
Hi,
On 6/13/22 20:30, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Instead of walking the list of children of an ACPI device directly,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.
>
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Rafael, feel free to take this upstream through the apci tree.
Regards,
Hans
> ---
>
> v1 -> v2:
> * Eliminate unnecessary branch (Andy).
>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 53 +++++++++++++++++------------------
> 1 file changed, 27 insertions(+), 26 deletions(-)
>
> Index: linux-pm/drivers/platform/x86/thinkpad_acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/thinkpad_acpi.c
> +++ linux-pm/drivers/platform/x86/thinkpad_acpi.c
> @@ -6841,6 +6841,31 @@ static const struct backlight_ops ibm_ba
>
> /* --------------------------------------------------------------------- */
>
> +static int __init tpacpi_evaluate_bcl(struct acpi_device *adev, void *not_used)
> +{
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + acpi_status status;
> + int rc;
> +
> + status = acpi_evaluate_object(adev->handle, "_BCL", NULL, &buffer);
> + if (ACPI_FAILURE(status))
> + return 0;
> +
> + obj = buffer.pointer;
> + if (!obj || obj->type != ACPI_TYPE_PACKAGE) {
> + acpi_handle_info(adev->handle,
> + "Unknown _BCL data, please report this to %s\n",
> + TPACPI_MAIL);
> + rc = 0;
> + } else {
> + rc = obj->package.count;
> + }
> + kfree(obj);
> +
> + return rc;
> +}
> +
> /*
> * Call _BCL method of video device. On some ThinkPads this will
> * switch the firmware to the ACPI brightness control mode.
> @@ -6848,37 +6873,13 @@ static const struct backlight_ops ibm_ba
>
> static int __init tpacpi_query_bcl_levels(acpi_handle handle)
> {
> - struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> - struct acpi_device *device, *child;
> - int rc;
> + struct acpi_device *device;
>
> device = acpi_fetch_acpi_dev(handle);
> if (!device)
> return 0;
>
> - rc = 0;
> - list_for_each_entry(child, &device->children, node) {
> - acpi_status status = acpi_evaluate_object(child->handle, "_BCL",
> - NULL, &buffer);
> - if (ACPI_FAILURE(status)) {
> - buffer.length = ACPI_ALLOCATE_BUFFER;
> - continue;
> - }
> -
> - obj = (union acpi_object *)buffer.pointer;
> - if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
> - pr_err("Unknown _BCL data, please report this to %s\n",
> - TPACPI_MAIL);
> - rc = 0;
> - } else {
> - rc = obj->package.count;
> - }
> - break;
> - }
> -
> - kfree(buffer.pointer);
> - return rc;
> + return acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL);
> }
>
>
>
>
>
On 13-06-22, 20:35, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Instead of walking the list of children of an ACPI device directly,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.
>
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).
Applied, thanks
--
~Vinod
On Thu, Jun 23, 2022 at 10:10 AM Vinod Koul <[email protected]> wrote:
>
> On 13-06-22, 20:35, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Instead of walking the list of children of an ACPI device directly,
> > use acpi_dev_for_each_child() to carry out an action for all of
> > the given ACPI device's children.
> >
> > This will help to eliminate the children list head from struct
> > acpi_device as it is redundant and it is used in questionable ways
> > in some places (in particular, locking is needed for walking the
> > list pointed to it safely, but it is often missing).
>
> Applied, thanks
Thanks, but the export of acpi_dev_for_each_child() is being added by
one of the previous patches in the series, so this one will not
compile without the rest of the series in the modular case.
Is this not a problem?
On 23-06-22, 14:29, Rafael J. Wysocki wrote:
> On Thu, Jun 23, 2022 at 10:10 AM Vinod Koul <[email protected]> wrote:
> >
> > On 13-06-22, 20:35, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > Instead of walking the list of children of an ACPI device directly,
> > > use acpi_dev_for_each_child() to carry out an action for all of
> > > the given ACPI device's children.
> > >
> > > This will help to eliminate the children list head from struct
> > > acpi_device as it is redundant and it is used in questionable ways
> > > in some places (in particular, locking is needed for walking the
> > > list pointed to it safely, but it is often missing).
> >
> > Applied, thanks
>
> Thanks, but the export of acpi_dev_for_each_child() is being added by
> one of the previous patches in the series, so this one will not
> compile without the rest of the series in the modular case.
Aha, I checked the symbol exists and my test build passed!
>
> Is this not a problem?
Yes indeed, so can you give a tag for that and or would you like to taje
this thru ACPI tree, in that case
Acked-By: Vinod Koul <[email protected]>
BR
--
~Vinod
On Thu, Jun 23, 2022 at 2:41 PM Vinod Koul <[email protected]> wrote:
>
> On 23-06-22, 14:29, Rafael J. Wysocki wrote:
> > On Thu, Jun 23, 2022 at 10:10 AM Vinod Koul <[email protected]> wrote:
> > >
> > > On 13-06-22, 20:35, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > Instead of walking the list of children of an ACPI device directly,
> > > > use acpi_dev_for_each_child() to carry out an action for all of
> > > > the given ACPI device's children.
> > > >
> > > > This will help to eliminate the children list head from struct
> > > > acpi_device as it is redundant and it is used in questionable ways
> > > > in some places (in particular, locking is needed for walking the
> > > > list pointed to it safely, but it is often missing).
> > >
> > > Applied, thanks
> >
> > Thanks, but the export of acpi_dev_for_each_child() is being added by
> > one of the previous patches in the series, so this one will not
> > compile without the rest of the series in the modular case.
>
> Aha, I checked the symbol exists and my test build passed!
> >
> > Is this not a problem?
>
> Yes indeed, so can you give a tag for that and or would you like to taje
> this thru ACPI tree, in that case
I'll take it.
> Acked-By: Vinod Koul <[email protected]>
Thank you!