Re: [PATCH v6 3/4] firmware: smccc: arm-cca-guest: Bind the TSM provider to an SMCCC device

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Sudeep Holla <sudeep.holla@kernel.org> writes:

...

> +static const struct smccc_device_info smccc_devices[] __initconst = {
> +       {
> +               .func_id        = ARM_SMCCC_TRNG_VERSION,
> +               .requires_smc   = false,
> +               .min_return     = ARM_SMCCC_TRNG_MIN_VERSION,
> +               .device_name    = "arm-smccc-trng",
> +       },
> +};
> +
> +static bool __init
> +smccc_probe_smccc_device(const struct smccc_device_info *smccc_dev)
> +{
> +       struct arm_smccc_res res;
> +       unsigned long ret;
> +
> +       if (!IS_ENABLED(CONFIG_ARM64))
> +               return false;
> +
> +       if (smccc_conduit == SMCCC_CONDUIT_NONE)
> +               return false;
> +
> +       if (smccc_dev->requires_smc && smccc_conduit != SMCCC_CONDUIT_SMC)
> +               return false;
> +
> +       arm_smccc_1_1_invoke(smccc_dev->func_id, &res);
> +       ret = res.a0;
> +
> +       if ((s32)ret < 0)
> +               return false;
> +
> +       return ret >= smccc_dev->min_return;
> +}
> +
>

I am not sure we want the check to be as simple as ret < 0. Some
function IDs may return input errors based on the supplied arguments
(for example, RMI_ERROR_INPUT). In those cases, we would likely want
this to be handled via a callback.

We also want to use conditional compilation for some function IDs.
Given the callback approach and the #ifdefs, I wonder whether what we
currently have is actually simpler and more flexible.”

>  void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
>  {
>         struct arm_smccc_res res;
> @@ -31,7 +68,7 @@ void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
>         smccc_version = version;
>         smccc_conduit = conduit;
>
> -       smccc_trng_available = smccc_probe_trng();
> +       smccc_trng_available = smccc_probe_smccc_device(&smccc_devices[0]);
>
>         if ((smccc_version >= ARM_SMCCC_VERSION_1_2) &&
>             (smccc_conduit != SMCCC_CONDUIT_NONE)) {
> @@ -241,14 +278,20 @@ subsys_initcall(arm_smccc_bus_init);
>
>  static int __init smccc_devices_init(void)
>  {
> -       struct platform_device *pdev;
> -
> -       if (smccc_trng_available) {
> -               pdev = platform_device_register_simple("smccc_trng", -1,
> -                                                      NULL, 0);
> -               if (IS_ERR(pdev))
> -                       pr_err("smccc_trng: could not register device: %ld\n",
> -                              PTR_ERR(pdev));
> +       const struct smccc_device_info *smccc_dev;
> +       struct arm_smccc_device *sdev;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(smccc_devices); i++) {
> +               smccc_dev = &smccc_devices[i];
> +
> +               if (!smccc_probe_smccc_device(smccc_dev))
> +                       continue;
> +
> +               sdev = arm_smccc_device_register(smccc_dev->device_name);
> +               if (IS_ERR(sdev))
> +                       pr_err("%s: could not register device: %ld\n",
> +                              smccc_dev->device_name, PTR_ERR(sdev));
>         }
>
>         return 0;
>

-aneesh