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: <0708dbf1-5914-4372-9df2-5cf590fd7bd6@kernel.org>
Date: Mon, 3 Feb 2025 17:34:20 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
 Johan Hovold <johan@...nel.org>
Cc: Abhinav Kumar <quic_abhinavk@...cinc.com>,
 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 03/02/2025 16:16, Dmitry Baryshkov wrote:
> 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.

I never used word "regression" alone. I said "dmesg regression", which
means you have error in logs or any system facility which provides you
self-information about device probe history. I don't remember if -ENODEV
leads to any printks, so maybe I am wrong here, but regardless normal
and expected operation of a driver should never result in -ERRNO, except
deferred probe of course.

Best regards,
Krzysztof