qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [RESEND PATCH for-3.1] hw/arm/sysbus-fdt: Only call match


From: Peter Maydell
Subject: Re: [Qemu-arm] [RESEND PATCH for-3.1] hw/arm/sysbus-fdt: Only call match_fn callback if the type matches
Date: Tue, 6 Nov 2018 15:47:34 +0000

On 5 November 2018 at 15:35, Eric Auger <address@hidden> 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 <address@hidden>
> Reported-by: Thomas Huth <address@hidden>
> ---
>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]