Re: [PATCH RFC 03/10] firmware: add RPMI TEE bus support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] firmware: rpmi-tee: stub returns NULL instead of ERR_PTR
- [Medium] firmware: rpmi-tee: missing file2alias integration for module auto-loading
- [Low] firmware: rpmi-tee: missing newline in modalias sysfs attribute
--

commit c7783a9ac1224efdc38ee5d5e0d605481f36e278
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

    firmware: add RPMI TEE bus support
    
    Add a bus for RISC-V RPMI TEE services. Each service exposed by a
    TEE endpoint is registered as its own device on this bus, identified
    by a UUID, so that a single physical TEE can host multiple
    independently-bindable service drivers.

> diff --git a/drivers/firmware/riscv_rpmi_tee/bus.c b/drivers/firmware/riscv_rpmi_tee/bus.c
> index 0000000000000..32f555c2f1f2a
> --- /dev/null
> +++ b/drivers/firmware/riscv_rpmi_tee/bus.c
> @@ -0,0 +1,202 @@
[ ... ]
> +#define RPMI_TEE_UEVENT_MODALIAS_FMT	"rpmi_tee:%pUb"
> +
> +static DEFINE_IDA(rpmi_tee_bus_id);

[Severity: Low]
Does this macro need a trailing newline?

When used by modalias_show() via sysfs_emit(), it will output a string
without a newline, which violates standard sysfs formatting conventions.

[ ... ]
> +static int rpmi_tee_device_uevent(const struct device *dev,
> +				  struct kobj_uevent_env *env)
> +{
> +	const struct rpmi_tee_device *rdev = to_rpmi_tee_dev(dev);
> +
> +	return add_uevent_var(env, "MODALIAS=" RPMI_TEE_UEVENT_MODALIAS_FMT,
> +			      &rdev->uuid);
> +}

[Severity: Medium]
Will this uevent fail to trigger automatic module loading?

If service drivers use MODULE_DEVICE_TABLE(rpmi_tee, ...) to export aliases,
the modpost tool will ignore them because scripts/mod/file2alias.c lacks
support for parsing the rpmi_tee_device_id structure.

[ ... ]
> diff --git a/include/linux/rpmi_tee.h b/include/linux/rpmi_tee.h
> index 0000000000000..c499f0427833e
> --- /dev/null
> +++ b/include/linux/rpmi_tee.h
[ ... ]
> +struct rpmi_tee_device_id {
> +	uuid_t uuid;
> +};

Should this structure be defined in include/linux/mod_devicetable.h to
properly integrate with file2alias.c?

[ ... ]
> +#else
> +static inline struct rpmi_tee_device *
> +rpmi_tee_device_register(const uuid_t *uuid, u32 endpoint_id,
> +			 const struct rpmi_tee_ops *ops, struct device *parent)
> +{
> +	return NULL;
> +}

[Severity: Medium]
Should this stub return an error pointer like ERR_PTR(-ENODEV) instead of
NULL?

The active implementation of rpmi_tee_device_register() guarantees an ERR_PTR
on failure. If a caller relies on IS_ERR() to check the return value, this
fallback stub will incorrectly pass the check and lead to a NULL pointer
dereference when the caller attempts to use the returned device pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-riscv-rpmi-tee-abi-v1-0-04908b81d885@oss.qualcomm.com?part=3