Re: [PATCH v6 1/5] ARM: add basic support for Trusted Foundations

[Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Fri, Sep 20, 2013 at 5:49 AM, Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> writes:
>
>> Trusted Foundations is a TrustZone-based secure monitor for ARM that
>> can be invoked using the same SMC-based API on all supported
>> platforms. This patch adds initial basic support for Trusted
>> Foundations using the ARM firmware API. Current features are limited
>> to the ability to boot secondary processors.
>
> [...]
>
>> +#if IS_ENABLED(CONFIG_OF)
>> +void of_register_trusted_foundations(void)
>> +{
>> +     struct device_node *node;
>> +     struct trusted_foundations_platform_data pdata;
>> +     int err;
>> +
>> +     node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>> +     if (!node)
>> +             return;
>> +
>> +     err = of_property_read_u32(node, "version-major", &pdata.version_major);
>> +     if (err != 0)
>> +             panic("Trusted Foundation: missing version-major property\n");
>
> Do really need to panic() the whole kernel for a missing property?
> Surely this can more gracefully recover, or assume some defaults?
>
> Same comment for the other uses of panic() in this patch.

For something as essential as the secure monitor (for which there
cannot be "safe" defaults) it makes sense to have the version
explicitly mentioned. Version is a mandatory property in the bindings,
its implementors just need to follow it.

As for the other uses of panic(), that's because you just cannot
reasonably continue booting the system if trusted foundations is
required but not support for it is built into the kernel. Once again,
I don't feel this is out of place.

Alex.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html