Re: [PATCH v2] media: venus: don't abuse dma_alloc for non-DMA allocations

[Hans Verkuil]

On 19/07/17 13:51, Stanimir Varbanov wrote:
> In venus_boot(), we pass a pointer to a phys_addr_t
> into dmam_alloc_coherent, which the compiler warns about:
> 
> platform/qcom/venus/firmware.c: In function 'venus_boot':
> platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of 'dmam_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 
> To avoid the error refactor venus_boot function by discard
> dma_alloc_coherent invocation because we don't want to map the
> memory for the device.  Something more, the usage of
> DMA mapping API is actually wrong and the current 
> implementation relies on several bugs in DMA mapping code.
> When these bugs are fixed that will break firmware loading,
> so fix this now to avoid future troubles.
> 
> The meaning of venus_boot is to copy the content of the
> firmware buffer into reserved (and memblock removed)
> block of memory and pass that physical address to the
> trusted zone for authentication and mapping through iommu
> form the secure world. After iommu mapping is done the iova
> is passed as ane entry point to the remote processor.
> 
> After this change memory-region property is parsed manually 
> and the physical address is memremap to CPU, call mdt_load to
> load firmware segments into proper places and unmap
> reserved memory.
> 
> Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>

Arnd, is this OK for you?

Regards,

	Hans

> ---
> this is v2 of the patch 2/4 form this [1] patchset. The only
> change is rewording in the patch description.
> 
> [1] http://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg115717.html
> 
>  drivers/media/platform/qcom/venus/core.c     | 10 ++--
>  drivers/media/platform/qcom/venus/core.h     |  1 -
>  drivers/media/platform/qcom/venus/firmware.c | 74 ++++++++++++----------------
>  drivers/media/platform/qcom/venus/firmware.h |  5 +-
>  4 files changed, 39 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 694f57a78288..a70368cb713f 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -76,7 +76,7 @@ static void venus_sys_error_handler(struct work_struct *work)
>  	hfi_core_deinit(core, true);
>  	hfi_destroy(core);
>  	mutex_lock(&core->lock);
> -	venus_shutdown(&core->dev_fw);
> +	venus_shutdown(core->dev);
>  
>  	pm_runtime_put_sync(core->dev);
>  
> @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct *work)
>  
>  	pm_runtime_get_sync(core->dev);
>  
> -	ret |= venus_boot(core->dev, &core->dev_fw, core->res->fwname);
> +	ret |= venus_boot(core->dev, core->res->fwname);
>  
>  	ret |= hfi_core_resume(core, true);
>  
> @@ -207,7 +207,7 @@ static int venus_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_runtime_disable;
>  
> -	ret = venus_boot(dev, &core->dev_fw, core->res->fwname);
> +	ret = venus_boot(dev, core->res->fwname);
>  	if (ret)
>  		goto err_runtime_disable;
>  
> @@ -238,7 +238,7 @@ static int venus_probe(struct platform_device *pdev)
>  err_core_deinit:
>  	hfi_core_deinit(core, false);
>  err_venus_shutdown:
> -	venus_shutdown(&core->dev_fw);
> +	venus_shutdown(dev);
>  err_runtime_disable:
>  	pm_runtime_set_suspended(dev);
>  	pm_runtime_disable(dev);
> @@ -259,7 +259,7 @@ static int venus_remove(struct platform_device *pdev)
>  	WARN_ON(ret);
>  
>  	hfi_destroy(core);
> -	venus_shutdown(&core->dev_fw);
> +	venus_shutdown(dev);
>  	of_platform_depopulate(dev);
>  
>  	pm_runtime_put_sync(dev);
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index e542700eee32..cba092bcb76d 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -101,7 +101,6 @@ struct venus_core {
>  	struct device *dev;
>  	struct device *dev_dec;
>  	struct device *dev_enc;
> -	struct device dev_fw;
>  	struct mutex lock;
>  	struct list_head instances;
>  	atomic_t insts_count;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 1b1a4f355918..d6d9560c1c19 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -12,29 +12,27 @@
>   *
>   */
>  
> -#include <linux/dma-mapping.h>
> +#include <linux/device.h>
>  #include <linux/firmware.h>
>  #include <linux/kernel.h>
> +#include <linux/io.h>
>  #include <linux/of.h>
> -#include <linux/of_reserved_mem.h>
> -#include <linux/slab.h>
> +#include <linux/of_address.h>
>  #include <linux/qcom_scm.h>
> +#include <linux/sizes.h>
>  #include <linux/soc/qcom/mdt_loader.h>
>  
>  #include "firmware.h"
>  
>  #define VENUS_PAS_ID			9
> -#define VENUS_FW_MEM_SIZE		SZ_8M
> +#define VENUS_FW_MEM_SIZE		(6 * SZ_1M)
>  
> -static void device_release_dummy(struct device *dev)
> -{
> -	of_reserved_mem_device_release(dev);
> -}
> -
> -int venus_boot(struct device *parent, struct device *fw_dev, const char *fwname)
> +int venus_boot(struct device *dev, const char *fwname)
>  {
>  	const struct firmware *mdt;
> +	struct device_node *node;
>  	phys_addr_t mem_phys;
> +	struct resource r;
>  	ssize_t fw_size;
>  	size_t mem_size;
>  	void *mem_va;
> @@ -43,66 +41,58 @@ int venus_boot(struct device *parent, struct device *fw_dev, const char *fwname)
>  	if (!qcom_scm_is_available())
>  		return -EPROBE_DEFER;
>  
> -	fw_dev->parent = parent;
> -	fw_dev->release = device_release_dummy;
> +	node = of_parse_phandle(dev->of_node, "memory-region", 0);
> +	if (!node) {
> +		dev_err(dev, "no memory-region specified\n");
> +		return -EINVAL;
> +	}
>  
> -	ret = dev_set_name(fw_dev, "%s:%s", dev_name(parent), "firmware");
> +	ret = of_address_to_resource(node, 0, &r);
>  	if (ret)
>  		return ret;
>  
> -	ret = device_register(fw_dev);
> -	if (ret < 0)
> -		return ret;
> +	mem_phys = r.start;
> +	mem_size = resource_size(&r);
>  
> -	ret = of_reserved_mem_device_init_by_idx(fw_dev, parent->of_node, 0);
> -	if (ret)
> -		goto err_unreg_device;
> +	if (mem_size < VENUS_FW_MEM_SIZE)
> +		return -EINVAL;
>  
> -	mem_size = VENUS_FW_MEM_SIZE;
> -
> -	mem_va = dmam_alloc_coherent(fw_dev, mem_size, &mem_phys, GFP_KERNEL);
> +	mem_va = memremap(r.start, mem_size, MEMREMAP_WC);
>  	if (!mem_va) {
> -		ret = -ENOMEM;
> -		goto err_unreg_device;
> +		dev_err(dev, "unable to map memory region: %pa+%zx\n",
> +			&r.start, mem_size);
> +		return -ENOMEM;
>  	}
>  
> -	ret = request_firmware(&mdt, fwname, fw_dev);
> +	ret = request_firmware(&mdt, fwname, dev);
>  	if (ret < 0)
> -		goto err_unreg_device;
> +		goto err_unmap;
>  
>  	fw_size = qcom_mdt_get_size(mdt);
>  	if (fw_size < 0) {
>  		ret = fw_size;
>  		release_firmware(mdt);
> -		goto err_unreg_device;
> +		goto err_unmap;
>  	}
>  
> -	ret = qcom_mdt_load(fw_dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
> +	ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
>  			    mem_size);
>  
>  	release_firmware(mdt);
>  
>  	if (ret)
> -		goto err_unreg_device;
> +		goto err_unmap;
>  
>  	ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
>  	if (ret)
> -		goto err_unreg_device;
> -
> -	return 0;
> +		goto err_unmap;
>  
> -err_unreg_device:
> -	device_unregister(fw_dev);
> +err_unmap:
> +	memunmap(mem_va);
>  	return ret;
>  }
>  
> -int venus_shutdown(struct device *fw_dev)
> +int venus_shutdown(struct device *dev)
>  {
> -	int ret;
> -
> -	ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
> -	device_unregister(fw_dev);
> -	memset(fw_dev, 0, sizeof(*fw_dev));
> -
> -	return ret;
> +	return qcom_scm_pas_shutdown(VENUS_PAS_ID);
>  }
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index f81a98979798..428efb56d339 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -16,8 +16,7 @@
>  
>  struct device;
>  
> -int venus_boot(struct device *parent, struct device *fw_dev,
> -	       const char *fwname);
> -int venus_shutdown(struct device *fw_dev);
> +int venus_boot(struct device *dev, const char *fwname);
> +int venus_shutdown(struct device *dev);
>  
>  #endif
>