| 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
| ||
|
Date: Tue, 14 Dec 2021 00:50:16 +0200
From: Stanimir Varbanov <stanimir.varbanov@...aro.org>
To: John Stultz <john.stultz@...aro.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Tadeusz Struk <tadeusz.struk@...aro.org>,
Andy Gross <agross@...nel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Lee Jones <lee.jones@...aro.org>,
Amit Pundir <amit.pundir@...aro.org>,
linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: venus: Synchronize probe() between venus_core
and enc/dec
Hi John,
On 12/9/21 5:11 AM, John Stultz wrote:
> On Tue, Nov 30, 2021 at 8:49 PM John Stultz <john.stultz@...aro.org> wrote:
>> On Tue, Nov 23, 2021 at 7:29 PM Bjorn Andersson
>> <bjorn.andersson@...aro.org> wrote:
>>> Rather than trying to synchronize away the side effects of
>>> of_platform_populate() I think we should stop using it.
>>>
>>> I had the very same problem in the qcom_wcnss remoteproc driver and
>>> in below change I got rid of that by manually initializing a struct
>>> device for the child node. In the event that the child probe defer I
>>> would just probe defer the parent as well.
>>>
>>> 1fcef985c8bd ("remoteproc: qcom: wcnss: Fix race with iris probe")
>>>
>>> The change might look a little bit messy, but the end result it much
>>> cleaner than relying on various locks etc.
>>>
>>>
>>> But in the qcom_wcnss case I have a child _device_ because I need
>>> something to do e.g. regulator_get() on. I fail to see why venc and vdec
>>> are devices in the first place.
>>
>> I definitely agree with Bjorn that all this asynchronous component
>> probing feels overly complicated, and a rework is probably the better
>> solution.
>>
>> Though my only question is: is someone planning to do this rework?
>>
>> In the meantime, Tadeusz' patch does resolve a *very* frequent boot
>> crash seen when the venus driver is enabled.
>> So Stanimir, should we consider merging this as a stop gap until the
>> larger probe rework is done?
>
> Stanimir? Does the above sound reasonable?
Apologize for the delay.
I'd like to avoid one more mutex in the driver, I think some reordering
in the .probe and changing the firmware_request API could help. I'll
spend some time to dig more deeply into the problem.
See untested patch below (I have to simulate firmware load from ufs
partition on Debian).
>
> thanks
> -john
>
--
regards,
Stan
>From 9bfb69026374fa010d36680554e2634d5d435681 Mon Sep 17 00:00:00 2001
From: Stanimir Varbanov <stanimir.varbanov@...aro.org>
Date: Tue, 14 Dec 2021 00:45:18 +0200
Subject: [PATCH] venus: WIP: Rework and reorder firmware load
Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
---
drivers/media/platform/qcom/venus/core.c | 8 +++----
drivers/media/platform/qcom/venus/core.h | 2 ++
drivers/media/platform/qcom/venus/firmware.c | 22 +++++++++++++++++++-
drivers/media/platform/qcom/venus/vdec.c | 3 ++-
drivers/media/platform/qcom/venus/venc.c | 3 ++-
5 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c
b/drivers/media/platform/qcom/venus/core.c
index 877eca125803..7f65b08b2bac 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -344,10 +344,6 @@ static int venus_probe(struct platform_device *pdev)
if (ret < 0)
goto err_runtime_disable;
- ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
- if (ret)
- goto err_runtime_disable;
-
ret = venus_firmware_init(core);
if (ret)
goto err_of_depopulate;
@@ -372,6 +368,10 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
goto err_venus_shutdown;
+ ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+ if (ret)
+ goto err_venus_shutdown;
+
ret = pm_runtime_put_sync(dev);
if (ret) {
pm_runtime_get_noresume(dev);
diff --git a/drivers/media/platform/qcom/venus/core.h
b/drivers/media/platform/qcom/venus/core.h
index 7c3bac01cd49..6455efb35168 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -182,6 +182,8 @@ struct venus_core {
atomic_t insts_count;
unsigned int state;
struct completion done;
+ struct completion fwload_done;
+ bool fwload_success;
unsigned int error;
unsigned long sys_error;
wait_queue_head_t sys_err_done;
diff --git a/drivers/media/platform/qcom/venus/firmware.c
b/drivers/media/platform/qcom/venus/firmware.c
index 14b6f1d05991..d523fbeb9d56 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -76,6 +76,14 @@ int venus_set_hw_state(struct venus_core *core, bool
resume)
return 0;
}
+static void firmware_async_load(const struct firmware *fw, void *context)
+{
+ struct venus_core *core = context;
+
+ core->fwload_success = true;
+ complete(&core->fwload_done);
+}
+
static int venus_load_fw(struct venus_core *core, const char *fwname,
phys_addr_t *mem_phys, size_t *mem_size)
{
@@ -101,10 +109,22 @@ static int venus_load_fw(struct venus_core *core,
const char *fwname,
if (ret)
goto err_put_node;
- ret = request_firmware(&mdt, fwname, dev);
+ init_completion(&core->fwload_done);
+ core->fwload_success = false;
+
+ ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOUEVENT, fwname,
+ dev, GFP_KERNEL, core,
+ firmware_async_load);
if (ret < 0)
goto err_put_node;
+ wait_for_completion(&core->fwload_done);
+
+ if (!core->fwload_success) {
+ ret = -ENOENT;
+ goto err_put_node;
+ }
+
fw_size = qcom_mdt_get_size(mdt);
if (fw_size < 0) {
ret = fw_size;
diff --git a/drivers/media/platform/qcom/venus/vdec.c
b/drivers/media/platform/qcom/venus/vdec.c
index 91da3f509724..0e718d24a3b3 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1718,6 +1718,8 @@ static int vdec_probe(struct platform_device *pdev)
if (!vdev)
return -ENOMEM;
+ core->dev_dec = dev;
+
strscpy(vdev->name, "qcom-venus-decoder", sizeof(vdev->name));
vdev->release = video_device_release;
vdev->fops = &vdec_fops;
@@ -1731,7 +1733,6 @@ static int vdec_probe(struct platform_device *pdev)
goto err_vdev_release;
core->vdev_dec = vdev;
- core->dev_dec = dev;
video_set_drvdata(vdev, core);
pm_runtime_set_autosuspend_delay(dev, 2000);
diff --git a/drivers/media/platform/qcom/venus/venc.c
b/drivers/media/platform/qcom/venus/venc.c
index 84bafc3118cc..1b3fb927eb16 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1448,6 +1448,8 @@ static int venc_probe(struct platform_device *pdev)
if (!vdev)
return -ENOMEM;
+ core->dev_enc = dev;
+
strscpy(vdev->name, "qcom-venus-encoder", sizeof(vdev->name));
vdev->release = video_device_release;
vdev->fops = &venc_fops;
@@ -1461,7 +1463,6 @@ static int venc_probe(struct platform_device *pdev)
goto err_vdev_release;
core->vdev_enc = vdev;
- core->dev_enc = dev;
video_set_drvdata(vdev, core);
pm_runtime_set_autosuspend_delay(dev, 2000);
--
2.25.1
Powered by blists - more mailing lists