linux-kernel - Re: [PATCH v7 3/7] media: venus: Add support for AR50

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: <8ac6f365-205a-4140-98b1-847f54ce08af@linaro.org>
Date: Thu, 17 Jul 2025 10:29:44 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Jorge Ramirez <jorge.ramirez@....qualcomm.com>
Cc: quic_vgarodia@...cinc.com, quic_dikshita@...cinc.com, krzk+dt@...nel.org,
 konradybcio@...nel.org, mchehab@...nel.org, andersson@...nel.org,
 conor+dt@...nel.org, amit.kucheria@....qualcomm.com,
 linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/7] media: venus: Add support for AR50_LITE video core

On 17/07/2025 08:19, Jorge Ramirez wrote:
>>> --- a/drivers/media/platform/qcom/venus/helpers.c
>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>>> @@ -230,6 +230,24 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
>>>    }
>>>    EXPORT_SYMBOL_GPL(venus_helper_alloc_dpb_bufs);
>>> +void venus_helper_prepare_eos_data(struct venus_inst *inst,
>>> +				   struct hfi_frame_data *data)
>>> +{
>>> +	struct venus_core *core = inst->core;
>>> +
>>> +	data->buffer_type = HFI_BUFFER_INPUT;
>>> +	data->flags = HFI_BUFFERFLAG_EOS;
>>> +
>>> +	if (IS_V6(core) && is_fw_rev_or_older(core, 1, 0, 87))
>>> +		return;
>>> +
>>> +	if (IS_V4(core) && is_lite(core) && is_fw_rev_or_older(core, 6, 0, 53))
>>> +		data->alloc_len = 1;
>>> +
>>> +	data->device_addr = 0xdeadb000;
>>> +}
>>> +EXPORT_SYMBOL_GPL(venus_helper_prepare_eos_data);
>> This function doesn't appear to have alot to do with AR50_LITE as it
>> pertains to IS_V6() and IS_V4().
>>
>> This I think should be a separate patch with its own commit log to describe
>> the quite complex logic of version numbers going on here.
> Let me give it some background:
> 
> According to the HFI specification, EOS (End-of-Stream) buffers must
> have 'valid' addresses. While the firmware currently appears to make no
> use of the EOS buffer contents, allocating and mapping them would have
> been a better driver choice IMO. Hoever this one has better performance
> which is probably the reason why it has stayed.
> 
> The firmware then does perform operations involving the buffer's size
> and length fields, and enforces boundary checks accordingly. On the
> AR50_LITE platform, an earlier firmware version lacked a check on
> alloc_len, leading to a division-by-zero scenario.
> 
> This has been addressed, and we plan to release firmware version 6.0.54,
> which includes the necessary boundary check for alloc_len.
> 
> I should probaly replace IS_V4(core) && is_lite(core) with
> IS_AR50_LITE() instead of trying to give it the appearence of a design
> feature.
> 
> seems the sensible thing to do, right?

I'll stipulate to all of that.

I know I'm being pedantic but, the title and subject of this patch is 
"AR50_LITE" does stuff.

As traveler from a mirror-universe - I would read the commit log here, 
look at this function and be none the wiser what was going on.

The EOS check is a fundamental HFI capability which is why I again 
reiterate it deserves its own commit log with the above explanation - 
word-for-word would be fine from my POV, to explain what is going on.

Long live the Empire!

---
bod