linux-kernel - Re: [PATCH 4/4] media: venus: add PIL support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65b3d26a-8180-c051-1d34-44d49dca34ca@linaro.org>
Date:   Tue, 22 May 2018 18:52:36 +0300
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Vikash Garodia <vgarodia@...eaurora.org>, hverkuil@...all.nl,
        mchehab@...nel.org, andy.gross@...aro.org,
        bjorn.andersson@...aro.org
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        acourbot@...gle.com
Subject: Re: [PATCH 4/4] media: venus: add PIL support

Hi,

On 05/22/2018 04:02 PM, Stanimir Varbanov wrote:
> Hi Vikash,
> 
> On 05/17/2018 02:32 PM, Vikash Garodia wrote:
>> This adds support to load the video firmware
>> and bring ARM9 out of reset. This is useful
>> for platforms which does not have trustzone
>> to reset the ARM9.
>>
>> Signed-off-by: Vikash Garodia <vgarodia@...eaurora.org>
>> ---
>>  .../devicetree/bindings/media/qcom,venus.txt       |   8 +-
>>  drivers/media/platform/qcom/venus/core.c           |  67 +++++++--
>>  drivers/media/platform/qcom/venus/core.h           |   6 +
>>  drivers/media/platform/qcom/venus/firmware.c       | 163 +++++++++++++++++----
>>  drivers/media/platform/qcom/venus/firmware.h       |  10 +-
>>  5 files changed, 217 insertions(+), 37 deletions(-)
>>

<snip>

>>  
>> -int venus_shutdown(struct device *dev)
>> +int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys,
>> +							size_t mem_size)
>>  {
>> -	return qcom_scm_pas_shutdown(VENUS_PAS_ID);
>> +	struct iommu_domain *iommu;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	if (!core->fw.dev)
>> +		return -EPROBE_DEFER;
>> +
>> +	dev = core->fw.dev;
>> +
>> +	iommu = iommu_domain_alloc(&platform_bus_type);
>> +	if (!iommu) {
>> +		dev_err(dev, "Failed to allocate iommu domain\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	iommu->geometry.aperture_start = 0x0;
>> +	iommu->geometry.aperture_end = VENUS_FW_MEM_SIZE;
> 
> The same comment for geometry params as for venus_probe is valid here.

Infact aperture_end will be overwritten by arm-smmu driver in the next
call to iommu_attach_device(), and by chance geometry.force_aperture
will become true.

I wonder is that geometry params are supposed to be used by drivers or
by iommu drivers?

> 
>> +
>> +	ret = iommu_attach_device(iommu, dev);
>> +	if (ret) {
>> +		dev_err(dev, "could not attach device\n");
>> +		goto err_attach;
>> +	}
>> +
>> +	ret = iommu_map(iommu, core->fw.iova, mem_phys, mem_size,
>> +			IOMMU_READ|IOMMU_WRITE|IOMMU_PRIV);
> 
> iova is not initialized and is zero, maybe we don't need that variable
> in the venus_firmware structure?
> 
>> +	if (ret) {
>> +		dev_err(dev, "could not map video firmware region\n");
>> +		goto err_map;
>> +	}
>> +	core->fw.iommu_domain = iommu;
>> +	venus_reset_hw(core);
>> +
>> +	return 0;
>> +
>> +err_map:
>> +	iommu_detach_device(iommu, dev);
>> +err_attach:
>> +	iommu_domain_free(iommu);
>> +	return ret;
>>  }
>> +

<snip>

-- 
regards,
Stan