On 5 November 2018 at 15:35, Eric Auger <eric.au...@redhat.com> wrote: > 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 <eric.au...@redhat.com> > Reported-by: Thomas Huth <th...@redhat.com> > --- > hw/arm/sysbus-fdt.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c > index 0e24c803a1..a44cf7f255 100644 > --- a/hw/arm/sysbus-fdt.c > +++ b/hw/arm/sysbus-fdt.c > @@ -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)) {
"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.) > + ret = iter->add_fn(sbdev, opaque); > + assert(!ret); > + return; > + } > + } else { > + ret = iter->add_fn(sbdev, opaque); > + assert(!ret); > + return; > + } > } thanks -- PMM