Hi Krzysztof,
Thanks for the feedback.
On 15 Feb 2024, at 9:59, Krzysztof Kozlowski wrote:
> On 13/02/2024 15:52, Balint Dobszay wrote:
>> The Trusted Services project provides a framework for developing and
>> deploying device Root of Trust services in FF-A Secure Partitions. The
>> FF-A SPs are accessible through the FF-A driver, but this doesn't
>> provide a user space interface. The goal of this TEE driver is to make
>> Trusted Services SPs accessible for user space clients.
>>
>> All TS SPs have the same FF-A UUID, it identifies the RPC protocol used
>> by TS. A TS SP can host one or more services, a service is identified by
>> its service UUID. The same type of service cannot be present twice in
>> the same SP. During SP boot each service in an SP is assigned an
>> interface ID, this is just a short ID to simplify message addressing.
>> There is 1:1 mapping between TS SPs and TEE devices, i.e. a separate TEE
>> device is registered for each TS SP. This is required since contrary to
>> the generic TEE design where memory is shared with the whole TEE
>> implementation, in case of FF-A, memory is shared with a specific SP. A
>> user space client has to be able to separately share memory with each SP
>> based on its endpoint ID.
>>
>> Signed-off-by: Balint Dobszay <balint.dobszay@xxxxxxx>
>> ---
>
>
>> +static int tstee_probe(struct ffa_device *ffa_dev)
>> +{
>> + struct tstee *tstee;
>> + int rc;
>> +
>> + ffa_dev->ops->msg_ops->mode_32bit_set(ffa_dev);
>> +
>> + if (!tstee_check_rpc_compatible(ffa_dev))
>> + return -EINVAL;
>> +
>> + tstee = kzalloc(sizeof(*tstee), GFP_KERNEL);
>> + if (!tstee)
>> + return -ENOMEM;
>> +
>> + tstee->ffa_dev = ffa_dev;
>> +
>> + tstee->pool = tstee_create_shm_pool();
>> + if (IS_ERR(tstee->pool)) {
>> + rc = PTR_ERR(tstee->pool);
>> + tstee->pool = NULL;
>> + goto err;
>
> Is it logically correct to call here tee_device_unregister()?
As Jens mentioned it doesn't cause any issues. But I see you point, it's
not logically correct. I'll refactor this.
>> + }
>> +
>> + tstee->teedev = tee_device_alloc(&tstee_desc, NULL, tstee->pool, tstee);
>> + if (IS_ERR(tstee->teedev)) {
>> + rc = PTR_ERR(tstee->teedev);
>> + tstee->teedev = NULL;
>> + goto err;
>> + }
>> +
>> + rc = tee_device_register(tstee->teedev);
>> + if (rc)
>> + goto err;
>> +
>> + ffa_dev_set_drvdata(ffa_dev, tstee);
>> +
>> + pr_info("driver initialized for endpoint 0x%x\n", ffa_dev->vm_id);
>
> Don't print simple probe success messages. Anyway all prints in device
> context should be dev_*.
Okay, I'll remove this.
>> +
>> + return 0;
>> +
>> +err:
>> + tstee_deinit_common(tstee);
>> + return rc;
>> +}
>> +
>> +static void tstee_remove(struct ffa_device *ffa_dev)
>> +{
>> + tstee_deinit_common(ffa_dev->dev.driver_data);
>> +}
>> +
>> +static const struct ffa_device_id tstee_device_ids[] = {
>> + /* TS RPC protocol UUID: bdcd76d7-825e-4751-963b-86d4f84943ac */
>> + { TS_RPC_UUID },
>> + {}
>> +};
>> +
>> +static struct ffa_driver tstee_driver = {
>> + .name = "arm_tstee",
>> + .probe = tstee_probe,
>> + .remove = tstee_remove,
>> + .id_table = tstee_device_ids,
>> +};
>> +
>> +static int __init mod_init(void)
>> +{
>> + return ffa_register(&tstee_driver);
>> +}
>> +module_init(mod_init)
>> +
>> +static void __exit mod_exit(void)
>> +{
>> + ffa_unregister(&tstee_driver);
>> +}
>> +module_exit(mod_exit)
>> +
>> +MODULE_ALIAS("arm-tstee");
>
> Why do you need this alias? I don't see MODULE_DEVICE_TABLE, so how this
> bus handles module loading?
You're right, the alias is not needed, I'll remove it.
Regarding MODULE_DEVICE_TABLE, AFAIK this mechanism is currently not
supported for the arm_ffa bus type. Maybe Sudeep can chime in on this?
Regards,
Balint
