Re: [PATCH 3/5] firmware: qcom: Add an TEE bus based ICE back-end

[Harshal Dev <harshal.dev@oss.qualcomm.com>]

Hi Konrad,

On 22-10-2026 01:22 pm, Konrad Dybcio wrote:
> On 10/21/26 5:43 PM, Harshal Dev wrote:
>> Add support for a TEE client driver which forwards calls from the Inline
>> Crypto Engine (ICE) service (invoked by the ICE driver) to the OPTEE ICE
>> Psuedo-TA.
>>
>> The ICE PTA supports both ICE Legacy and Standard modes. In Standard mode
>> Hardware Key Manager v1 is supported.
>>
>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +static int qcom_ice_tee_generate_key(struct device *dev,
>> +				     u8 *lt_key, size_t lt_key_size)
>> +{
>> +	struct qcom_ice_tee_private *data = dev_get_drvdata(dev);
>> +	struct tee_ioctl_invoke_arg inv_arg = {
>> +		.func = PTA_QCOM_ICE_GENERATE_KEY,
>> +		.session = data->session_id,
>> +		.num_params = TEE_NUM_PARAMS,
>> +	};
>> +	struct tee_param param[TEE_NUM_PARAMS] = { };
>> +	struct tee_shm *key_shm;
>> +	u8 *buf;
>> +	size_t out_size;
>> +	int ret;
>> +
>> +	key_shm = tee_shm_alloc_kernel_buf(data->ctx, lt_key_size);
>> +	if (IS_ERR(key_shm))
>> +		return PTR_ERR(key_shm);
>> +
>> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
>> +	param[0].u.memref.shm = key_shm;
>> +	param[0].u.memref.shm_offs = 0;
>> +	param[0].u.memref.size = lt_key_size;
>> +
>> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
>> +	if (ret < 0 || inv_arg.ret != 0) {
>> +		dev_err(dev,
>> +			"ICE generate_key failed, ret=%d, err=0x%x\n",
>> +			ret, inv_arg.ret);
>> +		ret = ret ?: -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	buf = tee_shm_get_va(key_shm, 0);
>> +	out_size = param[0].u.memref.size;
>> +
>> +	memcpy(lt_key, buf, lt_key_size);
>> +	memzero_explicit(buf, out_size);
> 
> LLM says: s/out_size/lt_key_size here?
> 
> Or said another way, currently lt_key_size and out_size are not
> cross checked
>

Ack. Will cross-check these values. 
> [...]
> 
>> +static int qcom_ice_tee_probe(struct tee_client_device *ice_dev)
>> +{
>> +	struct device *dev = &ice_dev->dev;
>> +	struct qcom_ice_tee_private *data;
>> +	struct tee_ioctl_open_session_arg sess_arg = {
>> +		.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL,
>> +	};
>> +	int ret, err = -ENODEV;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
>> +	if (IS_ERR(data->ctx))
>> +		return -ENODEV;
>> +
>> +	export_uuid(sess_arg.uuid, &ice_dev->id.uuid);
>> +	ret = tee_client_open_session(data->ctx, &sess_arg, NULL);
>> +	if (ret < 0 || sess_arg.ret != 0) {
>> +		dev_err(dev,
>> +			"tee_client_open_session failed, ret=%d, err=0x%x\n",
>> +			ret, sess_arg.ret);
>> +		err = ret ?: -EINVAL;
>> +		goto out_ctx;
>> +	}
>> +
>> +	data->session_id = sess_arg.session;
>> +	dev_set_drvdata(dev, data);
>> +
>> +	qcom_ice_ops_tee.dev = dev;
>> +	qcom_ice_svc_ops_register(&qcom_ice_ops_tee);
>> +
>> +	return ret;
> 
> This is always a 'return 0'
>

Ack.

Regards,
Harshal 
> Konrad