hw/arm/sysbus-fdt: Only call match_fn callback if the type matches

PATCH RESEND Superseded Eric Auger 2 participants 2 messages Nov 5, 2018 – Nov 6, 2018

Share & tools

Eric Auger

author

2018-11-05 15:47 UTC #0 lore raw

PATCH RESEND 1/1 hw/arm/sysbus-fdt: Only call match_fn callback if the type matches

Commit af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with DT compatible value) introduced a match_fn callback which gets called for each registered combo to check whether a sysbus device can be dynamically instantiated. However the callback gets called even if the device type does not match the binding combo typename field.

Let's change the add_fdt_node() logic so that we first check the type and if the match_fn callback is defined, then we also call it.

Binding combos only requesting a type check do not define the match_fn callback.

Fixes af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with

DT compatible value)

Signed-off-by Eric Auger <[email protected]>

Reported-by Thomas Huth <[email protected]>

---
 hw/arm/sysbus-fdt.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

hw/arm/sysbus-fdt.c +13 -5

index 0e24c803a1..a44cf7f255 100644
@@ -449,7 +449,7 @@ static bool type_match(SysBusDevice *sbdev, const BindingEntry *entry)
     return !strcmp(object_get_typename(OBJECT(sbdev)), entry->typename);
 }
 
-#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), type_match}
+#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), NULL}
 
 /* list of supported dynamic sysbus bindings */
 static const BindingEntry bindings[] = {

@@ -481,10 +481,18 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
     for (i = 0; i < ARRAY_SIZE(bindings); i++) {
         const BindingEntry *iter = &bindings[i];
 
-        if (iter->match_fn(sbdev, iter)) {
-            ret = iter->add_fn(sbdev, opaque);
-            assert(!ret);
-            return;
+        if (type_match(sbdev, iter)) {
+            if (iter->match_fn) {
+                if (iter->match_fn(sbdev, iter)) {
+                    ret = iter->add_fn(sbdev, opaque);
+                    assert(!ret);
+                    return;
+                }
+            } else {
+                ret = iter->add_fn(sbdev, opaque);
+                assert(!ret);
+                return;
+            }
         }
     }
     error_report("Device %s can not be dynamically instantiated",

Signature · 1 line

2.17.2

Peter Maydell

2018-11-06 15:47 UTC #1 lore raw

Quoted 35 lines · Eric Auger

Commit af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with DT compatible value) introduced a match_fn callback which gets called for each registered combo to check whether a sysbus device can be dynamically instantiated. However the callback gets called even if the device type does not match the binding combo typename field.
Let's change the add_fdt_node() logic so that we first check the type and if the match_fn callback is defined, then we also call it.
Binding combos only requesting a type check do not define the match_fn callback.
Fixes af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with
DT compatible value)
Signed-off-by Eric Auger <[email protected]>
Reported-by Thomas Huth <[email protected]>
---
 hw/arm/sysbus-fdt.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
hw/arm/sysbus-fdt.c +1 -1
index 0e24c803a1..a44cf7f255 100644
@@ -449,7 +449,7 @@ static bool type_match(SysBusDevice *sbdev, const BindingEntry *entry)
     return !strcmp(object_get_typename(OBJECT(sbdev)), entry->typename);
 }
 
-#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), type_match}
+#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), NULL}
 
 /* list of supported dynamic sysbus bindings */
 static const BindingEntry bindings[] = {

hw/arm/sysbus-fdt.c static void add_fdt_node(SysBusDevice *sbdev, void *opaque) · line 486

Quoted 10 lines

hw/arm/sysbus-fdt.c +3 -4

@@ -481,10 +481,18 @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque)
     for (i = 0; i < ARRAY_SIZE(bindings); i++) {
         const BindingEntry *iter = &bindings[i];
 
-        if (iter->match_fn(sbdev, iter)) {
-            ret = iter->add_fn(sbdev, opaque);
-            assert(!ret);
-            return;
+        if (type_match(sbdev, iter)) {
+            if (iter->match_fn) {
+                if (iter->match_fn(sbdev, iter)) {

"if (!iter->match_fn || iter->match_fn(sbdev, iter)) {"

would let you avoid duplicating the code in the body of this if and the else clause later.

(Or you could have a match_always() function that always returns true instead of having to special case NULL.)

hw/arm/sysbus-fdt.c static void add_fdt_node(SysBusDevice *sbdev, void *opaque) · line 496

Quoted 10 lines

hw/arm/sysbus-fdt.c +9 -0

+                    ret = iter->add_fn(sbdev, opaque);
+                    assert(!ret);
+                    return;
+                }
+            } else {
+                ret = iter->add_fn(sbdev, opaque);
+                assert(!ret);
+                return;
+            }
         }

thanks
-- PMM