Re: [PATCH v5 1/2] tee: add revision sysfs attribute

["Mario Limonciello (AMD) (kernel.org)" <superm1@kernel.org>]

On 1/7/2026 8:55 PM, Aristo Chen wrote:
> Hi Mario,
> 
> Mario Limonciello <superm1@kernel.org> 於 2026年1月7日週三 下午11:28寫道：
>>
>> On 1/7/26 9:26 AM, Aristo Chen wrote:
>>> Add a generic TEE revision sysfs attribute backed by a new
>>> optional get_tee_revision() callback. The revision string is
>>> diagnostic-only and must not be used to infer feature support.
>>>
>>> Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
>>> ---
>>>    Documentation/ABI/testing/sysfs-class-tee | 10 +++++
>>>    drivers/tee/tee_core.c                    | 51 ++++++++++++++++++++++-
>>>    include/linux/tee_core.h                  |  9 ++++
>>>    3 files changed, 69 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-tee b/Documentation/ABI/testing/sysfs-class-tee
>>> index c9144d16003e..6e783210104e 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-tee
>>> +++ b/Documentation/ABI/testing/sysfs-class-tee
>>> @@ -13,3 +13,13 @@ Description:
>>>                space if the variable is absent. The primary purpose
>>>                of this variable is to let systemd know whether
>>>                tee-supplicant is needed in the early boot with initramfs.
>>> +
>>> +What:                /sys/class/tee/tee{,priv}X/revision
>>> +Date:                Dec 2025
>>> +KernelVersion:       6.18
>>
>> This needs to be bumped up and dates pushed out.
> 
> I will fix this in the v6 patch, thanks!
> 
>>
>>> +Contact:     op-tee@lists.trustedfirmware.org
>>> +Description:
>>> +             Read-only revision string reported by the TEE driver. This is
>>> +             for diagnostics only and must not be used to infer feature
>>> +             support. Use TEE_IOC_VERSION for capability and compatibility
>>> +             checks.
>>> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
>>> index d65d47cc154e..0a00499811c1 100644
>>> --- a/drivers/tee/tee_core.c
>>> +++ b/drivers/tee/tee_core.c
>>> @@ -1146,7 +1146,56 @@ static struct attribute *tee_dev_attrs[] = {
>>>        NULL
>>>    };
>>>
>>> -ATTRIBUTE_GROUPS(tee_dev);
>>> +static const struct attribute_group tee_dev_group = {
>>> +     .attrs = tee_dev_attrs,
>>> +};
>>> +
>>> +static ssize_t revision_show(struct device *dev,
>>> +                          struct device_attribute *attr, char *buf)
>>> +{
>>> +     struct tee_device *teedev = container_of(dev, struct tee_device, dev);
>>> +     char version[TEE_REVISION_STR_SIZE];
>>> +     int ret;
>>> +
>>> +     if (!teedev->desc->ops->get_tee_revision)
>>> +             return -ENODEV;
>>> +
>>> +     ret = teedev->desc->ops->get_tee_revision(teedev, version,
>>> +                                               sizeof(version));
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     return sysfs_emit(buf, "%s\n", version);
>>> +}
>>> +static DEVICE_ATTR_RO(revision);
>>> +
>>> +static struct attribute *tee_revision_attrs[] = {
>>> +     &dev_attr_revision.attr,
>>> +     NULL
>>> +};
>>> +
>>> +static umode_t tee_revision_attr_is_visible(struct kobject *kobj,
>>> +                                         struct attribute *attr, int n)
>>> +{
>>> +     struct device *dev = kobj_to_dev(kobj);
>>> +     struct tee_device *teedev = container_of(dev, struct tee_device, dev);
>>> +
>>> +     if (teedev->desc->ops->get_tee_revision)
>>> +             return attr->mode;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct attribute_group tee_revision_group = {
>>> +     .attrs = tee_revision_attrs,
>>> +     .is_visible = tee_revision_attr_is_visible,
>>> +};
>>> +
>>> +static const struct attribute_group *tee_dev_groups[] = {
>>> +     &tee_dev_group,
>>> +     &tee_revision_group,
>>> +     NULL
>>> +};
>>>
>>>    static const struct class tee_class = {
>>>        .name = "tee",
>>> diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h
>>> index 1f3e5dad6d0d..ee5f0bd41f43 100644
>>> --- a/include/linux/tee_core.h
>>> +++ b/include/linux/tee_core.h
>>> @@ -76,6 +76,9 @@ struct tee_device {
>>>    /**
>>>     * struct tee_driver_ops - driver operations vtable
>>>     * @get_version:    returns version of driver
>>> + * @get_tee_revision:        returns revision string (diagnostic only);
>>
>> Why is this comment here about it being for diagnostics only?  I feel
>> it's up to the implementation how it would be used.
> 
> According to the previous discussion, we would like to prevent user
> thinking about optee os version x.y means z feature, and we should
> always use TEE_IOC_VERSION for capability and compatibility
> check.
> 
> Is there any other specific use case that makes you think removing
> the wording is required?

Ah I didn't realize there was previous discussion that lead to this, I 
saw some earlier versions in my holiday mailbox glut but ignored them 
when I saw the new one.

Leave it as is then.

> 
>>
>>> + *                   do not infer feature support from this, use
>>> + *                   TEE_IOC_VERSION instead
>>>     * @open:           called for a context when the device file is opened
>>>     * @close_context:  called when the device file is closed
>>>     * @release:                called to release the context
>>> @@ -95,9 +98,12 @@ struct tee_device {
>>>     * client closes the device file, even if there are existing references to the
>>>     * context. The TEE driver can use @close_context to start cleaning up.
>>>     */
>>> +
>>>    struct tee_driver_ops {
>>>        void (*get_version)(struct tee_device *teedev,
>>>                            struct tee_ioctl_version_data *vers);
>>> +     int (*get_tee_revision)(struct tee_device *teedev,
>>> +                             char *buf, size_t len);
>>>        int (*open)(struct tee_context *ctx);
>>>        void (*close_context)(struct tee_context *ctx);
>>>        void (*release)(struct tee_context *ctx);
>>> @@ -123,6 +129,9 @@ struct tee_driver_ops {
>>>        int (*shm_unregister)(struct tee_context *ctx, struct tee_shm *shm);
>>>    };
>>>
>>> +/* Size for TEE revision string buffer used by get_tee_revision(). */
>>> +#define TEE_REVISION_STR_SIZE        128
>>> +
>>>    /**
>>>     * struct tee_desc - Describes the TEE driver to the subsystem
>>>     * @name:   name of driver
>>
> 
> Best regards,
> Aristo