| 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: <f1344e49-61b6-4115-ae88-55b4a3cfed28@quicinc.com>
Date: Fri, 31 Jan 2025 10:44:28 -0800
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
Dikshita Agarwal
<quic_dikshita@...cinc.com>,
<quic_vgarodia@...cinc.com>, <mchehab@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <p.zabel@...gutronix.de>
CC: <hverkuil@...all.nl>, <sebastian.fricke@...labora.com>,
<bryan.odonoghue@...aro.org>, <dmitry.baryshkov@...aro.org>,
<neil.armstrong@...aro.org>, <nicolas@...fresne.ca>,
<u.kleine-koenig@...libre.com>, <stefan.schmidt@...aro.org>,
<lujianhua000@...il.com>, <linux-arm-msm@...r.kernel.org>,
<linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <krzysztof.kozlowski@...aro.org>,
<johan@...nel.org>
Subject: Re: [RFC PATCH v10 1/2] media: iris: introduce helper module to
select video driver
On 1/29/2025 2:44 AM, Krzysztof Kozlowski wrote:
> On 28/01/2025 09:04, Dikshita Agarwal wrote:
>> Introduce a helper module with a kernel param to select between
>> venus and iris drivers for platforms supported by both drivers.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
>> ---
>> drivers/media/platform/qcom/Makefile | 1 +
>> drivers/media/platform/qcom/iris/iris_core.h | 1 +
>> drivers/media/platform/qcom/iris/iris_probe.c | 3 +
>> drivers/media/platform/qcom/venus/core.c | 5 ++
>> .../platform/qcom/video_drv_helper/Makefile | 4 ++
>> .../qcom/video_drv_helper/video_drv_helper.c | 70 +++++++++++++++++++
>> .../qcom/video_drv_helper/video_drv_helper.h | 11 +++
>> 7 files changed, 95 insertions(+)
>> create mode 100644 drivers/media/platform/qcom/video_drv_helper/Makefile
>> create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c
>> create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h
>>
>> diff --git a/drivers/media/platform/qcom/Makefile b/drivers/media/platform/qcom/Makefile
>> index ea2221a202c0..15accde3bd67 100644
>> --- a/drivers/media/platform/qcom/Makefile
>> +++ b/drivers/media/platform/qcom/Makefile
>> @@ -2,3 +2,4 @@
>> obj-y += camss/
>> obj-y += iris/
>> obj-y += venus/
>> +obj-y += video_drv_helper/
>> diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h
>> index 37fb4919fecc..7108e751ff88 100644
>> --- a/drivers/media/platform/qcom/iris/iris_core.h
>> +++ b/drivers/media/platform/qcom/iris/iris_core.h
>> @@ -107,5 +107,6 @@ struct iris_core {
>>
>> int iris_core_init(struct iris_core *core);
>> void iris_core_deinit(struct iris_core *core);
>> +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver);
>>
>> #endif
>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
>> index 954cc7c0cc97..276461ade811 100644
>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev)
>> u64 dma_mask;
>> int ret;
>>
>> + if (!video_drv_should_bind(&pdev->dev, true))
>> + return -ENODEV;
>
> Wouldn't it mark the probe as failed and cause dmesg regressions?
>
>> +
>> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
>> if (!core)
>> return -ENOMEM;
>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
>> index 77d48578ecd2..b38be7812efe 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -369,12 +369,17 @@ static int venus_add_dynamic_nodes(struct venus_core *core)
>> static void venus_remove_dynamic_nodes(struct venus_core *core) {}
>> #endif
>>
>> +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver);
>
> You just defined it in the header. Why is this here?
>
>> +
>> static int venus_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> struct venus_core *core;
>> int ret;
>>
>> + if (!video_drv_should_bind(&pdev->dev, false))
>> + return -ENODEV;
>
> Same problems - d,esg regression.
>
>> +
>> core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
>> if (!core)
>> return -ENOMEM;
>> diff --git a/drivers/media/platform/qcom/video_drv_helper/Makefile b/drivers/media/platform/qcom/video_drv_helper/Makefile
>> new file mode 100644
>> index 000000000000..82567e0392fb
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/video_drv_helper/Makefile
>> @@ -0,0 +1,4 @@
>
> Missing SPDX
>
>> +# Makefile for Video driver helper
>> +
>> +obj-m := video_drv_helper.o
>> +
>> diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c
>> new file mode 100644
>> index 000000000000..9009c2906e54
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +
>> +#include "video_drv_helper.h"
>> +
>> +/* The venus driver supports only hfi gen1 to communicate with the firmware while
>
> Use Linux generic coding style comment, not netdev.
>
>> + * the iris driver supports both hfi gen1 and hfi gen2.
>> + * The support of hfi gen1 is added to the iris driver with the intention that
>> + * it can support old gen1 interface based firmware, while enabling gen2 based future SOCs.
>> + * With this, the plan is to migrate older SOCs from venus to iris.
>> + * As of now, since the iris driver supports only entry level features and doesn't have
>> + * feature parity with the venus driver, a runtime-selection is provided to user via
>> + * module parameter 'prefer_venus' to select the driver.
>> + * This selection is available only for the SoCs which are supported by both venus
>> + * and iris eg: SM8250.
>> + * When the feature parity is achieved, the plan is to switch the default to point to
>> + * the iris driver, then gradually start removing platforms from venus.
>> + * Hardware supported by only venus - 8916, 8996, SDM660, SDM845, SC7180, SC7280
>> + * Hardware supported by only iris - SM8550
>> + * Hardware supported by both venus and iris - SM8250
>> + */
>> +
>> +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS)
>> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver)
>> +{
>> + /* If just a single driver is enabled, use it no matter what */
>> + return true;
>> +}
>> +
>> +#else
>> +static bool prefer_venus = true;
>> +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred");
>> +module_param(prefer_venus, bool, 0444);
>
>
> The choice of driver is by module blacklisting, not by failing probes.
>
> I don't understand why this patchset is needed and neither commit msg
> nor above longer code comment explain me that. Just blacklist the module.
>
> Best regards,
> Krzysztof
Summarizing the discussion with myself, Krzysztof and Dmitry:
1) module blacklisting solution will not be ideal if users want to have
both venus and iris or either of them built-in
2) with current approach, one of the probes (either venus or iris) will
certainly fail as video_drv_should_bind() will fail for one of them.
This can be considered as a regression and should not happen.
Solution: If the user prefers iris driver and iris driver has not probed
yet, and if venus tries to probe ahead of iris we keep -EDEFERing till
iris probes and succeeds. Vice-versa when the preference is venus as well.
Powered by blists - more mailing lists