linux-kernel - Re: [RFC PATCH v10 1/2] media: iris: introduce helper module to select video driver

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: <Z6B822-6UTxQfX46@hovoldconsulting.com>
Date: Mon, 3 Feb 2025 09:22:51 +0100
From: Johan Hovold <johan@...nel.org>
To: Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc: 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, 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
Subject: Re: [RFC PATCH v10 1/2] media: iris: introduce helper module to
 select video driver

On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote:
> On 1/29/2025 2:44 AM, Krzysztof Kozlowski wrote:
> > On 28/01/2025 09:04, Dikshita Agarwal wrote:

> >> 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?

No, this is perfectly fine. Probe can return -ENODEV and driver core
will continue with any further matches.

> >> +#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.

> 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

Module blacklisting is not the way to go, you shouldn't have two drivers
racing to bind to the same device ever.

> 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.

How can that be a regression? One driver must fail to probe (see above).
 
> 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.

This sounds wrong too.

Look, first you guys need to explain *why* you want to have two drivers
for the same hardware (not just to me, in the commit message and cover
letter).

That's something that really should never be the case and would need to
be motivated properly.

Second, if the reasons for keeping both drivers are deemed justifiable,
you need to come up with mechanism for only binding one of them.

I already told you that module parameters is not the way to go here (and
the msm drm driver's abuse of module parameters is not a good precedent
here).

If this is a transitional thing (which it must be), then just add a
Kconfig symbol to determine which driver should probe. That's good
enough for evaluating whatever needs to be evaluated, and doesn't
depend on adding anti-patterns like module parameters (and helper
modules for them).

Keep it simple.

Johan