[v7,08/12] firmware: qcom: qseecom: convert to using the TZ allocator

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
convert all users of it in the qseecom module to using the TZ allocator
for creating SCM call buffers. Together with using the cleanup macros,
it has the added benefit of a significant code shrink. As this is
largely a module separate from the SCM driver, let's use a separate
memory pool.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
Tested-by: Maximilian Luz <luzmaximilian@gmail.com>
Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s
Tested-by: Deepti Jaggi <quic_djaggi@quicinc.com> #sa8775p-ride
Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
---
 .../firmware/qcom/qcom_qseecom_uefisecapp.c   | 281 +++++++-----------
 drivers/firmware/qcom/qcom_scm.c              |  30 +-
 include/linux/firmware/qcom/qcom_qseecom.h    |   4 +-
 3 files changed, 111 insertions(+), 204 deletions(-)

On Mon, Feb 05, 2024 at 07:28:06PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> convert all users of it in the qseecom module to using the TZ allocator
> for creating SCM call buffers.

This reads as if this is removal of duplication, now that we have the TZ
allocation. But wasn't there something about you not being able to mix
and match shmbridge and non-shmbridge allocations in the interface, so
this transition is actually required? Or did I get that wrong and this
just reduction in duplication?

> Together with using the cleanup macros,
> it has the added benefit of a significant code shrink.

That is true, but the move to using cleanup macros at the same time as
changing the implementation makes it unnecessarily hard to reason about
this patch.

This patch would be much better if split in two.

> As this is
> largely a module separate from the SCM driver, let's use a separate
> memory pool.
> 

This module is effectively used to read and write EFI variables today.
Is that worth statically removing 256kb of DDR for? Is this done solely
because it logically makes sense, or did you choose this for a reason?

Regards,
Bjorn

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> Tested-by: Maximilian Luz <luzmaximilian@gmail.com>
> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s
> Tested-by: Deepti Jaggi <quic_djaggi@quicinc.com> #sa8775p-ride
> Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  .../firmware/qcom/qcom_qseecom_uefisecapp.c   | 281 +++++++-----------
>  drivers/firmware/qcom/qcom_scm.c              |  30 +-
>  include/linux/firmware/qcom/qcom_qseecom.h    |   4 +-
>  3 files changed, 111 insertions(+), 204 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> index 32188f098ef3..3a068f8b6990 100644
> --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> @@ -7,17 +7,21 @@
>   * Copyright (C) 2023 Maximilian Luz <luzmaximilian@gmail.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/sizes.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/ucs2_string.h>
>  
>  #include <linux/firmware/qcom/qcom_qseecom.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/firmware/qcom/qcom_tzmem.h>
>  
>  /* -- Qualcomm "uefisecapp" interface definitions. -------------------------- */
>  
> @@ -253,6 +257,7 @@ struct qsee_rsp_uefi_query_variable_info {
>  struct qcuefi_client {
>  	struct qseecom_client *client;
>  	struct efivars efivars;
> +	struct qcom_tzmem_pool *mempool;
>  };
>  
>  static struct device *qcuefi_dev(struct qcuefi_client *qcuefi)
> @@ -272,11 +277,11 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>  					   const efi_guid_t *guid, u32 *attributes,
>  					   unsigned long *data_size, void *data)
>  {
> -	struct qsee_req_uefi_get_variable *req_data;
> -	struct qsee_rsp_uefi_get_variable *rsp_data;
> +	struct qsee_req_uefi_get_variable *req_data __free(qcom_tzmem) = NULL;
> +	struct qsee_rsp_uefi_get_variable *rsp_data __free(qcom_tzmem) = NULL;
>  	unsigned long buffer_size = *data_size;
> -	efi_status_t efi_status = EFI_SUCCESS;
>  	unsigned long name_length;
> +	efi_status_t efi_status;
>  	size_t guid_offs;
>  	size_t name_offs;
>  	size_t req_size;
> @@ -304,17 +309,13 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>  		__array(u8, buffer_size)
>  	);
>  
> -	req_data = kzalloc(req_size, GFP_KERNEL);
> -	if (!req_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out;
> -	}
> +	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
> +	if (!req_data)
> +		return EFI_OUT_OF_RESOURCES;
>  
> -	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
> -	if (!rsp_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out_free_req;
> -	}
> +	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, rsp_size, GFP_KERNEL);
> +	if (!rsp_data)
> +		return EFI_OUT_OF_RESOURCES;
>  
>  	req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE;
>  	req_data->data_size = buffer_size;
> @@ -325,28 +326,20 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>  	req_data->length = req_size;
>  
>  	status = ucs2_strscpy(((void *)req_data) + req_data->name_offset, name, name_length);
> -	if (status < 0) {
> -		efi_status = EFI_INVALID_PARAMETER;
> -		goto out_free;
> -	}
> +	if (status < 0)
> +		return EFI_INVALID_PARAMETER;
>  
>  	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
>  
>  	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->length < sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length < sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>  
>  	if (rsp_data->status) {
>  		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> @@ -360,18 +353,14 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>  				*attributes = rsp_data->attributes;
>  		}
>  
> -		goto out_free;
> +		return efi_status;
>  	}
>  
> -	if (rsp_data->length > rsp_size) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length > rsp_size)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length)
> +		return EFI_DEVICE_ERROR;
>  
>  	/*
>  	 * Note: We need to set attributes and data size even if the buffer is
> @@ -394,33 +383,23 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>  	if (attributes)
>  		*attributes = rsp_data->attributes;
>  
> -	if (buffer_size == 0 && !data) {
> -		efi_status = EFI_SUCCESS;
> -		goto out_free;
> -	}
> +	if (buffer_size == 0 && !data)
> +		return EFI_SUCCESS;
>  
> -	if (buffer_size < rsp_data->data_size) {
> -		efi_status = EFI_BUFFER_TOO_SMALL;
> -		goto out_free;
> -	}
> +	if (buffer_size < rsp_data->data_size)
> +		return EFI_BUFFER_TOO_SMALL;
>  
>  	memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size);
>  
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>  }
>  
>  static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
>  					   const efi_guid_t *guid, u32 attributes,
>  					   unsigned long data_size, const void *data)
>  {
> -	struct qsee_req_uefi_set_variable *req_data;
> -	struct qsee_rsp_uefi_set_variable *rsp_data;
> -	efi_status_t efi_status = EFI_SUCCESS;
> +	struct qsee_req_uefi_set_variable *req_data __free(qcom_tzmem) = NULL;
> +	struct qsee_rsp_uefi_set_variable *rsp_data __free(qcom_tzmem) = NULL;
>  	unsigned long name_length;
>  	size_t name_offs;
>  	size_t guid_offs;
> @@ -450,17 +429,14 @@ static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e
>  		__array_offs(u8, data_size, &data_offs)
>  	);
>  
> -	req_data = kzalloc(req_size, GFP_KERNEL);
> -	if (!req_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out;
> -	}
> +	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
> +	if (!req_data)
> +		return EFI_OUT_OF_RESOURCES;
>  
> -	rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL);
> -	if (!rsp_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out_free_req;
> -	}
> +	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*rsp_data),
> +				    GFP_KERNEL);
> +	if (!rsp_data)
> +		return EFI_OUT_OF_RESOURCES;
>  
>  	req_data->command_id = QSEE_CMD_UEFI_SET_VARIABLE;
>  	req_data->attributes = attributes;
> @@ -473,10 +449,8 @@ static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e
>  	req_data->length = req_size;
>  
>  	status = ucs2_strscpy(((void *)req_data) + req_data->name_offset, name, name_length);
> -	if (status < 0) {
> -		efi_status = EFI_INVALID_PARAMETER;
> -		goto out_free;
> -	}
> +	if (status < 0)
> +		return EFI_INVALID_PARAMETER;
>  
>  	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
>  
> @@ -485,42 +459,31 @@ static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e
>  
>  	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data,
>  				       sizeof(*rsp_data));
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->length != sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length != sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>  
>  	if (rsp_data->status) {
>  		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
>  			__func__, rsp_data->status);
> -		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
> +		return qsee_uefi_status_to_efi(rsp_data->status);
>  	}
>  
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>  }
>  
>  static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>  						unsigned long *name_size, efi_char16_t *name,
>  						efi_guid_t *guid)
>  {
> -	struct qsee_req_uefi_get_next_variable *req_data;
> -	struct qsee_rsp_uefi_get_next_variable *rsp_data;
> -	efi_status_t efi_status = EFI_SUCCESS;
> +	struct qsee_req_uefi_get_next_variable *req_data __free(qcom_tzmem) = NULL;
> +	struct qsee_rsp_uefi_get_next_variable *rsp_data __free(qcom_tzmem) = NULL;
> +	efi_status_t efi_status;
>  	size_t guid_offs;
>  	size_t name_offs;
>  	size_t req_size;
> @@ -545,17 +508,13 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>  		__array(*name, *name_size / sizeof(*name))
>  	);
>  
> -	req_data = kzalloc(req_size, GFP_KERNEL);
> -	if (!req_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out;
> -	}
> +	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
> +	if (!req_data)
> +		return EFI_OUT_OF_RESOURCES;
>  
> -	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
> -	if (!rsp_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out_free_req;
> -	}
> +	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, rsp_size, GFP_KERNEL);
> +	if (!rsp_data)
> +		return EFI_OUT_OF_RESOURCES;
>  
>  	req_data->command_id = QSEE_CMD_UEFI_GET_NEXT_VARIABLE;
>  	req_data->guid_offset = guid_offs;
> @@ -567,26 +526,18 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>  	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
>  	status = ucs2_strscpy(((void *)req_data) + req_data->name_offset, name,
>  			      *name_size / sizeof(*name));
> -	if (status < 0) {
> -		efi_status = EFI_INVALID_PARAMETER;
> -		goto out_free;
> -	}
> +	if (status < 0)
> +		return EFI_INVALID_PARAMETER;
>  
>  	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->length < sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length < sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>  
>  	if (rsp_data->status) {
>  		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> @@ -601,77 +552,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>  		if (efi_status == EFI_BUFFER_TOO_SMALL)
>  			*name_size = rsp_data->name_size;
>  
> -		goto out_free;
> +		return efi_status;
>  	}
>  
> -	if (rsp_data->length > rsp_size) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length > rsp_size)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length)
> +		return EFI_DEVICE_ERROR;
>  
>  	if (rsp_data->name_size > *name_size) {
>  		*name_size = rsp_data->name_size;
> -		efi_status = EFI_BUFFER_TOO_SMALL;
> -		goto out_free;
> +		return EFI_BUFFER_TOO_SMALL;
>  	}
>  
> -	if (rsp_data->guid_size != sizeof(*guid)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->guid_size != sizeof(*guid))
> +		return EFI_DEVICE_ERROR;
>  
>  	memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size);
>  	status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset,
>  			      rsp_data->name_size / sizeof(*name));
>  	*name_size = rsp_data->name_size;
>  
> -	if (status < 0) {
> +	if (status < 0)
>  		/*
>  		 * Return EFI_DEVICE_ERROR here because the buffer size should
>  		 * have already been validated above, causing this function to
>  		 * bail with EFI_BUFFER_TOO_SMALL.
>  		 */
> -		efi_status = EFI_DEVICE_ERROR;
> -	}
> +		return EFI_DEVICE_ERROR;
>  
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>  }
>  
>  static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, u32 attr,
>  						  u64 *storage_space, u64 *remaining_space,
>  						  u64 *max_variable_size)
>  {
> -	struct qsee_req_uefi_query_variable_info *req_data;
> -	struct qsee_rsp_uefi_query_variable_info *rsp_data;
> -	efi_status_t efi_status = EFI_SUCCESS;
> +	struct qsee_req_uefi_query_variable_info *req_data __free(qcom_tzmem) = NULL;
> +	struct qsee_rsp_uefi_query_variable_info *rsp_data __free(qcom_tzmem) = NULL;
>  	int status;
>  
> -	req_data = kzalloc(sizeof(*req_data), GFP_KERNEL);
> -	if (!req_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out;
> -	}
> +	req_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*req_data),
> +				    GFP_KERNEL);
> +	if (!req_data)
> +		return EFI_OUT_OF_RESOURCES;
>  
> -	rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL);
> -	if (!rsp_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out_free_req;
> -	}
> +	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*rsp_data),
> +				    GFP_KERNEL);
> +	if (!rsp_data)
> +		return EFI_OUT_OF_RESOURCES;
>  
>  	req_data->command_id = QSEE_CMD_UEFI_QUERY_VARIABLE_INFO;
>  	req_data->attributes = attr;
> @@ -679,26 +612,19 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
>  
>  	status = qcom_qseecom_app_send(qcuefi->client, req_data, sizeof(*req_data), rsp_data,
>  				       sizeof(*rsp_data));
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->length != sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length != sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>  
>  	if (rsp_data->status) {
>  		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
>  			__func__, rsp_data->status);
> -		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
> -		goto out_free;
> +		return qsee_uefi_status_to_efi(rsp_data->status);
>  	}
>  
>  	if (storage_space)
> @@ -710,12 +636,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
>  	if (max_variable_size)
>  		*max_variable_size = rsp_data->max_variable_size;
>  
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>  }
>  
>  /* -- Global efivar interface. ---------------------------------------------- */
> @@ -844,6 +765,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
>  	if (status)
>  		qcuefi_set_reference(NULL);
>  
> +	qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
> +	if (IS_ERR(qcuefi->mempool))
> +		return PTR_ERR(qcuefi->mempool);
> +
>  	return status;
>  }
>  
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 3a6cefb4eb2e..318d7d398e5f 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1567,9 +1567,9 @@ EXPORT_SYMBOL_GPL(qcom_scm_qseecom_app_get_id);
>  /**
>   * qcom_scm_qseecom_app_send() - Send to and receive data from a given QSEE app.
>   * @app_id:   The ID of the target app.
> - * @req:      Request buffer sent to the app (must be DMA-mappable).
> + * @req:      Request buffer sent to the app (must be TZ memory)
>   * @req_size: Size of the request buffer.
> - * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
> + * @rsp:      Response buffer, written to by the app (must be TZ memory)
>   * @rsp_size: Size of the response buffer.
>   *
>   * Sends a request to the QSEE app associated with the given ID and read back
> @@ -1585,26 +1585,12 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
>  {
>  	struct qcom_scm_qseecom_resp res = {};
>  	struct qcom_scm_desc desc = {};
> -	dma_addr_t req_phys;
> -	dma_addr_t rsp_phys;
> +	phys_addr_t req_phys;
> +	phys_addr_t rsp_phys;
>  	int status;
>  
> -	/* Map request buffer */
> -	req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE);
> -	status = dma_mapping_error(__scm->dev, req_phys);
> -	if (status) {
> -		dev_err(__scm->dev, "qseecom: failed to map request buffer\n");
> -		return status;
> -	}
> -
> -	/* Map response buffer */
> -	rsp_phys = dma_map_single(__scm->dev, rsp, rsp_size, DMA_FROM_DEVICE);
> -	status = dma_mapping_error(__scm->dev, rsp_phys);
> -	if (status) {
> -		dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE);
> -		dev_err(__scm->dev, "qseecom: failed to map response buffer\n");
> -		return status;
> -	}
> +	req_phys = qcom_tzmem_to_phys(req);
> +	rsp_phys = qcom_tzmem_to_phys(rsp);
>  
>  	/* Set up SCM call data */
>  	desc.owner = QSEECOM_TZ_OWNER_TZ_APPS;
> @@ -1622,10 +1608,6 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
>  	/* Perform call */
>  	status = qcom_scm_qseecom_call(&desc, &res);
>  
> -	/* Unmap buffers */
> -	dma_unmap_single(__scm->dev, rsp_phys, rsp_size, DMA_FROM_DEVICE);
> -	dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE);
> -
>  	if (status)
>  		return status;
>  
> diff --git a/include/linux/firmware/qcom/qcom_qseecom.h b/include/linux/firmware/qcom/qcom_qseecom.h
> index 5c28298a98be..e868fac55675 100644
> --- a/include/linux/firmware/qcom/qcom_qseecom.h
> +++ b/include/linux/firmware/qcom/qcom_qseecom.h
> @@ -27,9 +27,9 @@ struct qseecom_client {
>  /**
>   * qcom_qseecom_app_send() - Send to and receive data from a given QSEE app.
>   * @client:   The QSEECOM client associated with the target app.
> - * @req:      Request buffer sent to the app (must be DMA-mappable).
> + * @req:      Request buffer sent to the app (must be TZ memory).
>   * @req_size: Size of the request buffer.
> - * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
> + * @rsp:      Response buffer, written to by the app (must be TZ memory).
>   * @rsp_size: Size of the response buffer.
>   *
>   * Sends a request to the QSEE app associated with the given client and read
> -- 
> 2.40.1
>

On Sun, Feb 18, 2024 at 4:08 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Mon, Feb 05, 2024 at 07:28:06PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> > convert all users of it in the qseecom module to using the TZ allocator
> > for creating SCM call buffers.
>
> This reads as if this is removal of duplication, now that we have the TZ
> allocation. But wasn't there something about you not being able to mix
> and match shmbridge and non-shmbridge allocations in the interface, so
> this transition is actually required? Or did I get that wrong and this
> just reduction in duplication?
>

What is the question exactly? Yes it is required because once we
enable SHM bridge, "normal" memory will no longer be accepted for SCM
calls.

> > Together with using the cleanup macros,
> > it has the added benefit of a significant code shrink.
>
> That is true, but the move to using cleanup macros at the same time as
> changing the implementation makes it unnecessarily hard to reason about
> this patch.
>
> This patch would be much better if split in two.
>

I disagree. If we have a better interface in place, then let's use it
right away, otherwise it's just useless churn.

> > As this is
> > largely a module separate from the SCM driver, let's use a separate
> > memory pool.
> >
>
> This module is effectively used to read and write EFI variables today.
> Is that worth statically removing 256kb of DDR for? Is this done solely
> because it logically makes sense, or did you choose this for a reason?
>

Well, it will stop working (with SHM bridge enabled) if we don't. We
can possibly release the pool once we know we'll no longer need to
access EFI variables but I'm not sure if that makes sense? Or maybe
remove the pool after some time of driver inactivity and create a new
one when it's needed again?

Bart

[snip]

On Tue, Feb 20, 2024 at 10:54:02AM +0100, Bartosz Golaszewski wrote:
> On Sun, Feb 18, 2024 at 4:08 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Mon, Feb 05, 2024 at 07:28:06PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> > > convert all users of it in the qseecom module to using the TZ allocator
> > > for creating SCM call buffers.
> >
> > This reads as if this is removal of duplication, now that we have the TZ
> > allocation. But wasn't there something about you not being able to mix
> > and match shmbridge and non-shmbridge allocations in the interface, so
> > this transition is actually required? Or did I get that wrong and this
> > just reduction in duplication?
> >
> 
> What is the question exactly? Yes it is required because once we
> enable SHM bridge, "normal" memory will no longer be accepted for SCM
> calls.
> 

This fact is not covered anywhere in the series.

> > > Together with using the cleanup macros,
> > > it has the added benefit of a significant code shrink.
> >
> > That is true, but the move to using cleanup macros at the same time as
> > changing the implementation makes it unnecessarily hard to reason about
> > this patch.
> >
> > This patch would be much better if split in two.
> >
> 
> I disagree. If we have a better interface in place, then let's use it
> right away, otherwise it's just useless churn.
> 

The functional change and the use of cleanup macros, could be done
independently of each other, each one fully beneficial on their own.

As such I don't find it hard to claim that they are two independent
changes.

> > > As this is
> > > largely a module separate from the SCM driver, let's use a separate
> > > memory pool.
> > >
> >
> > This module is effectively used to read and write EFI variables today.
> > Is that worth statically removing 256kb of DDR for? Is this done solely
> > because it logically makes sense, or did you choose this for a reason?
> >
> 
> Well, it will stop working (with SHM bridge enabled) if we don't. We
> can possibly release the pool once we know we'll no longer need to
> access EFI variables but I'm not sure if that makes sense? Or maybe
> remove the pool after some time of driver inactivity and create a new
> one when it's needed again?
> 

Sounds like a good motivation to me, let's document it so that the next
guy understand why this was done.

Regards,
Bjorn

> Bart
> 
> [snip]

On Tue, Feb 27, 2024 at 5:53 PM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Tue, Feb 20, 2024 at 10:54:02AM +0100, Bartosz Golaszewski wrote:
> >
> > I disagree. If we have a better interface in place, then let's use it
> > right away, otherwise it's just useless churn.
> >
>
> The functional change and the use of cleanup macros, could be done
> independently of each other, each one fully beneficial on their own.
>
> As such I don't find it hard to claim that they are two independent
> changes.
>

This series would be 50% bigger for no reason if we split every patch
using the new allocator into two. I absolutely don't see how this
makes any sense. We're removing the calls to old interfaces and using
the new ones instead. The new ones happen to support cleanup so we use
it right away. If the old ones supported cleanup then maybeeee it
would make some sense to convert them first and then use tzmem. As it
is, there's really no point.

Bart

On Sun, Mar 03, 2024 at 05:18:18PM +0100, Bartosz Golaszewski wrote:
> On Tue, Feb 27, 2024 at 5:53 PM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Tue, Feb 20, 2024 at 10:54:02AM +0100, Bartosz Golaszewski wrote:
> > >
> > > I disagree. If we have a better interface in place, then let's use it
> > > right away, otherwise it's just useless churn.
> > >
> >
> > The functional change and the use of cleanup macros, could be done
> > independently of each other, each one fully beneficial on their own.
> >
> > As such I don't find it hard to claim that they are two independent
> > changes.
> >
> 
> This series would be 50% bigger for no reason if we split every patch
> using the new allocator into two.

I'm not asking you to split every patch into two, unless that makes
sense.

> I absolutely don't see how this makes any sense.

I find it unnecessarily hard to determine which parts of _this_ patch is
functional and which is cleanup.

> We're removing the calls to old interfaces and using
> the new ones instead. The new ones happen to support cleanup so we use
> it right away. If the old ones supported cleanup then maybeeee it
> would make some sense to convert them first and then use tzmem. As it
> is, there's really no point.
> 

The old interface is kzalloc(). I haven't used the cleanup mechanism
myself yet, but are you saying that there's no cleanup support for
kzalloc()?

Regards,
Bjorn