| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dc27b259-437d-177f-4e9c-73830b6656e2@xs4all.nl>
Date: Thu, 20 Jul 2017 13:26:16 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Stanimir Varbanov <stanimir.varbanov@...aro.org>
Cc: Arnd Bergmann <arnd@...db.de>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2] media: venus: don't abuse dma_alloc for non-DMA
allocations
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@...aro.org>
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@vger.kernel.org/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
>
Powered by blists - more mailing lists