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: <tqbm672pi223ipcw7btiemlb745weeeiy4gnazzeghozhq2emj@wppbkms6hir5>
Date: Mon, 3 Feb 2025 17:16:50 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Johan Hovold <johan@...nel.org>
Cc: Abhinav Kumar <quic_abhinavk@...cinc.com>, 
	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, 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 Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote:
> 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).

I also don't think that it's a regression. I think Krzysztof was
concerned about the 'failed to bind' messages in dmesg.

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

I think it has been written several time (not sure about this commit):
to provide a way for a migration path _while_ people are working on Iris
features. Being able to A/B test Venus and Iris drivers and to report
possible regressions or lack of those on the common platforms supported
by those (sm8250 at this moment).

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

No, Kconfig complicates A/B testing. What you usually want to do is to
boot a kernel, then go over a bunch of files testing that they work with
both drivers. Kconfig implies booting abother kernel, etc.

> 
> Keep it simple.
> 
> Johan

-- 
With best wishes
Dmitry