linux-kernel - Re: [PATCH 09/13] media: qcom: camss: vfe: Move common code into vfe core

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: <14e297e3-6d70-4270-9477-6bb094ccc621@quicinc.com>
Date: Mon, 2 Sep 2024 21:11:16 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
        Bryan O'Donoghue
	<bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
        <todor.too@...il.com>, <mchehab@...nel.org>, <robh@...nel.org>,
        <krzk+dt@...nel.org>, <conor+dt@...nel.org>
CC: <linux-arm-msm@...r.kernel.org>, <linux-media@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <kernel@...cinc.com>
Subject: Re: [PATCH 09/13] media: qcom: camss: vfe: Move common code into vfe
 core

Hi Vladimir,


On 8/24/2024 9:06 PM, Vladimir Zapolskiy wrote:

>>> +
>>> +/*
>>> + * vfe_enable_v2 - Enable streaming on VFE line
>>> + * @line: VFE line
>>> + *
>>> + * Return 0 on success or a negative error code otherwise
>>> + */
>>> +int vfe_enable_v2(struct vfe_line *line)
>>> +{
>>> +    struct vfe_device *vfe = to_vfe(line);
>>> +    int ret;
>>> +
>>> +    mutex_lock(&vfe->stream_lock);
>>> +
>>> +    if (vfe->res->hw_ops->enable_irq)
>>> +        vfe->res->hw_ops->enable_irq(vfe);
>>
>> Right so generally speaking I don't believe we should have any null
>> function pointers.
>>
>> We just mandate that to be comitted, an impelmentation must provide a
>> dummy but, in this case when do we ever want a dummy function anyway
>> surely enable_irq() is a fundamental operation that is core to the logic.
> 
> Why? What could be a justification here?
> 
> The image capturing media pipeline for all recent Qualcomm SoCs, including
> this one in the series for SM8550, can be set up and enabled without
> touching VFE interrupts at all.
> 
> It might be extremely confusing to see in the code that some not ever
> requested interrupts are enabled/disabled, and then to discover that just
> some stubs around VFE interrupts are added. And it's the case especially
> in this new vfe_enable_v2() function, which I believe is intended for
> CAMSS support on new platforms.
> 
> What's worse, since these VFE interrupts are not needed on the modern
> platforms, it will require to add a proposed dummy "return 0" function
> into any CAMSS support for new platforms forever. I believe it'd be better
> to clearly say that it's a legacy to have an obligatory support of VFE
> interrupts.
> 

Sure, I will add a proposed dummy "return 0" function for these interfaces.

Thanks,
Depeng