Skip to contentSkip to conversation

driver core: Extend device_is_dependent()

PATCH In review Rafael J. Wysocki 4 participants 5 messages Jan 15, 2021 – Jan 19, 2021 Driver core

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 driver core: Extend device_is_dependent()

From: Rafael J. Wysocki <[email protected]>

If the device passed as the target (second argument) to device_is_dependent() is not completely registered (that is, it has been initialized, but not added yet), but the parent pointer of it is set, it may be missing from the list of the parent's children and device_for_each_child() called by device_is_dependent() cannot be relied on to catch that dependency.

For this reason, modify device_is_dependent() to check the ancestors of the target device by following its parent pointer in addition to the device_for_each_child() walk.

Fixes 9ed9895370ae ("driver core: Functional dependencies tracking support")

---

-> v2:
* Improve the changelog.
* Add a comment to explain the reason for the extra check.
* Add tags.

No code changes.

---
 drivers/base/core.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
drivers/base/core.c +16 -1
Index: linux-pm/drivers/base/core.c
===================================================================
@@ -208,6 +208,16 @@ int device_links_read_lock_held(void)
 #endif
 #endif /* !CONFIG_SRCU */
 
+static bool device_is_ancestor(struct device *dev, struct device *target)
+{
+	while (target->parent) {
+		target = target->parent;
+		if (dev == target)
+			return true;
+	}
+	return false;
+}
+
 /**
  * device_is_dependent - Check if one device depends on another one
  * @dev: Device to check dependencies for.
@@ -221,7 +231,12 @@ int device_is_dependent(struct device *d
 	struct device_link *link;
 	int ret;
 
-	if (dev == target)
+	/*
+	 * The "ancestors" check is needed to catch the case when the target
+	 * device has not been completely initialized yet and it is still
+	 * missing from the list of children of its parent device.
+	 */
+	if (dev == target || device_is_ancestor(dev, target))
 		return 1;
 
 	ret = device_for_each_child(dev, target, device_is_dependent);
Reviewed-by on v2 1/1
#1 lore raw
Quoted 57 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If the device passed as the target (second argument) to device_is_dependent() is not completely registered (that is, it has been initialized, but not added yet), but the parent pointer of it is set, it may be missing from the list of the parent's children and device_for_each_child() called by device_is_dependent() cannot be relied on to catch that dependency.

For this reason, modify device_is_dependent() to check the ancestors of the target device by following its parent pointer in addition to the device_for_each_child() walk.

Fixes 9ed9895370ae ("driver core: Functional dependencies tracking support")

---

-> v2:
* Improve the changelog.
* Add a comment to explain the reason for the extra check.
* Add tags.

No code changes.

---
 drivers/base/core.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/core.c
===================================================================

drivers/base/core.c +10 -0
@@ -208,6 +208,16 @@ int device_links_read_lock_held(void)
 #endif
 #endif /* !CONFIG_SRCU */
 
+static bool device_is_ancestor(struct device *dev, struct device *target)
+{
+       while (target->parent) {
+               target = target->parent;
+               if (dev == target)
+                       return true;
+       }
+       return false;
+}
+
 /**
  * device_is_dependent - Check if one device depends on another one
  * @dev: Device to check dependencies for.
@@ -221,7 +231,12 @@ int device_is_dependent(struct device *d
        struct device_link *link;
        int ret;
 

drivers/base/core.c int device_is_dependent(struct device *d · line 242

Quoted 10 lines
drivers/base/core.c +6 -1
-       if (dev == target)
+       /*
+        * The "ancestors" check is needed to catch the case when the target
+        * device has not been completely initialized yet and it is still
+        * missing from the list of children of its parent device.
+        */
+       if (dev == target || device_is_ancestor(dev, target))
                return 1;
 
        ret = device_for_each_child(dev, target, device_is_dependent);

fw_devlink_relax_cycle() needs a similar fix. Want me to handle that as a separate patch to driver-core-next? Or do you want to combine that in this patch?

-Saravana

#2 lore raw
Quoted 67 lines · Saravana Kannan
Quoted 65 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If the device passed as the target (second argument) to device_is_dependent() is not completely registered (that is, it has been initialized, but not added yet), but the parent pointer of it is set, it may be missing from the list of the parent's children and device_for_each_child() called by device_is_dependent() cannot be relied on to catch that dependency.

For this reason, modify device_is_dependent() to check the ancestors of the target device by following its parent pointer in addition to the device_for_each_child() walk.

Fixes 9ed9895370ae ("driver core: Functional dependencies tracking support")

---

-> v2:
* Improve the changelog.
* Add a comment to explain the reason for the extra check.
* Add tags.

No code changes.

---
 drivers/base/core.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/core.c
===================================================================

drivers/base/core.c +16 -1
@@ -208,6 +208,16 @@ int device_links_read_lock_held(void)
 #endif
 #endif /* !CONFIG_SRCU */
 
+static bool device_is_ancestor(struct device *dev, struct device *target)
+{
+       while (target->parent) {
+               target = target->parent;
+               if (dev == target)
+                       return true;
+       }
+       return false;
+}
+
 /**
  * device_is_dependent - Check if one device depends on another one
  * @dev: Device to check dependencies for.
@@ -221,7 +231,12 @@ int device_is_dependent(struct device *d
        struct device_link *link;
        int ret;
 
-       if (dev == target)
+       /*
+        * The "ancestors" check is needed to catch the case when the target
+        * device has not been completely initialized yet and it is still
+        * missing from the list of children of its parent device.
+        */
+       if (dev == target || device_is_ancestor(dev, target))
                return 1;
 
Quoted 8 lines

drivers/base/core.c int device_is_dependent(struct device *d · line 242

Quoted 2 lines
drivers/base/core.c +0 -0
        ret = device_for_each_child(dev, target, device_is_dependent);

fw_devlink_relax_cycle() needs a similar fix. Want me to handle that as a separate patch to driver-core-next? Or do you want to combine that in this patch?

I would prefer that to be a different patch because of the Fixes: tag.

Also fw_devlink_relax_cycle() is in linux-next only for now, so it is better to avoid mixing a mainline fix with updating that one.

#3 lore raw
In reply to Rafael J. Wysocki
Quoted 10 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If the device passed as the target (second argument) to device_is_dependent() is not completely registered (that is, it has been initialized, but not added yet), but the parent pointer of it is set, it may be missing from the list of the parent's children and device_for_each_child() called by device_is_dependent() cannot be relied on to catch that dependency.

Quoted 8 lines

For this reason, modify device_is_dependent() to check the ancestors of the target device by following its parent pointer in addition to the device_for_each_child() walk.

Fixes 9ed9895370ae ("driver core: Functional dependencies tracking support")

Greg, are you going to pick up this one or do you want me to take care of it?

It has been reviewed by Saravana.

Quoted 51 lines

---

-> v2:
* Improve the changelog.
* Add a comment to explain the reason for the extra check.
* Add tags.

No code changes.

---
 drivers/base/core.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/base/core.c
===================================================================

drivers/base/core.c +16 -1
@@ -208,6 +208,16 @@ int device_links_read_lock_held(void)
 #endif
 #endif /* !CONFIG_SRCU */
 
+static bool device_is_ancestor(struct device *dev, struct device *target)
+{
+       while (target->parent) {
+               target = target->parent;
+               if (dev == target)
+                       return true;
+       }
+       return false;
+}
+
 /**
  * device_is_dependent - Check if one device depends on another one
  * @dev: Device to check dependencies for.
@@ -221,7 +231,12 @@ int device_is_dependent(struct device *d
        struct device_link *link;
        int ret;
 
-       if (dev == target)
+       /*
+        * The "ancestors" check is needed to catch the case when the target
+        * device has not been completely initialized yet and it is still
+        * missing from the list of children of its parent device.
+        */
+       if (dev == target || device_is_ancestor(dev, target))
                return 1;
 
        ret = device_for_each_child(dev, target, device_is_dependent);
#4 lore raw
Quoted 15 lines · Rafael J. Wysocki
Quoted 14 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

If the device passed as the target (second argument) to device_is_dependent() is not completely registered (that is, it has been initialized, but not added yet), but the parent pointer of it is set, it may be missing from the list of the parent's children and device_for_each_child() called by device_is_dependent() cannot be relied on to catch that dependency.

For this reason, modify device_is_dependent() to check the ancestors of the target device by following its parent pointer in addition to the device_for_each_child() walk.

Quoted 8 lines
Quoted 4 lines
Fixes 9ed9895370ae ("driver core: Functional dependencies tracking support")

Greg, are you going to pick up this one or do you want me to take care of it?

It has been reviewed by Saravana.

I'll take it, thanks. sorry for the delay.

greg k-h