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: <cb665eab-ffeb-486a-bdaa-a69bdb681139@quicinc.com>
Date: Fri, 16 Aug 2024 21:07:12 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: 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 Bryan,

On 8/15/2024 8:09 AM, Bryan O'Donoghue 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.
> 
> Also a style nit-pick if you get a hw_ops pointer you don't have to jump 
> through so -> many -> indirection -> hoops.
> 

Ok, I will declare the hw_ops first.

> Code will look neater that way.
> 
> I'll go through the vfe_enable() stuff in more detail on your next drop.
> 
> Please ensure again with the hw_version() that you have equivalent logic 
> before and after => no behaviour change similarly with vfe_enable() and 
> friends.
> 
> The objective is to remove code duplication, not to change logical 
> behaviors at all, no matter how seemingly trival that change might be -> 
> hw_version 0xsomenumber instea of 0xX, 0xY 0xZ
> 
> It probably sounds dogmatic but, its safer that way.
> 

Sure, I won't change the original code.

Thanks,
Depeng