linux-kernel - Re: [PATCH v3 3/9] remoteproc: qcom_q6v5

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: <c104d173-6316-4bcf-b89b-62417d0b30c8@kernel.org>
Date: Fri, 2 Jan 2026 09:55:00 +0000
From: Bryan O'Donoghue <bod@...nel.org>
To: barnabas.czeman@...nlining.org
Cc: Bjorn Andersson <andersson@...nel.org>,
 Mathieu Poirier <mathieu.poirier@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Stephan Gerhold <stephan@...hold.net>,
 linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/9] remoteproc: qcom_q6v5_mss: Add MDM9607

On 01/01/2026 21:57, barnabas.czeman@...nlining.org wrote:
> On 2026-01-01 21:58, Bryan O'Donoghue wrote:
>> On 01/01/2026 13:50, barnabas.czeman@...nlining.org wrote:
>>>>> +				for (; i >= 0; i--) {
>>>>> +					val |= BIT(i);
>>>>> +					writel(val, qproc->reg_base + mem_pwr_ctl);
>>>>> +					/*
>>>>> +					 * Read back value to ensure the write is done then
>>>>> +					 * wait for 1us for both memory peripheral and data
>>>>> +					 * array to turn on.
>>>>> +					 */
>>>>> +					val |= readl(qproc->reg_base + mem_pwr_ctl);
>>>>> +					udelay(1);
>>>> Isn't the logic here inverted ?
>>>>
>>>> i.e. you've written a thing and ostensibly require a delay for that
>>>> thing to take effect, the power to switch on in this case.
>>>>
>>>> It makes more sense to write, delay and read back rather than write,
>>>> readback and delay surely...
>>> This is the original reset sequence without modification, i have just
>>> moved it in a else case when it is not an MDM9607, MSM8917 or MSM8937.
>>
>> Doesn't make it correct, we fix upstream logic bugs all the time...
> Here is the original upstream logic
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/remoteproc/qcom_q6v5_mss.c?h=next-20251219#n823
> and here is the same at downstream 3.18
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.6.6.c32-05500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L451
> and same from downstream 4.9
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.10.6.2.c26-01500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L518

Plenty of downstream bugs...

Let's assume those are posted writes i.e. the IO fabric equivalent of 
UDP - I'm not sure I'd say the downstream code is consistent in its 
treatement of write transactions..

But aside from just pointing at downstream - how is val |= readl() a 
correct thing versus val = readl();

...

I mean its just not

> 
>>
>> For example a read-back to ensure write completion is only required for
>> posted memory transactions.
>>
>> Is this a posted write ?
>>
>> Is there an io-fabric in the world which exceeds 1 microsecond to
>> perform a write transaction ?
>>
>> Anyway leaving that aside the bit that's really objectionable and IMO
>> obvious a bug is val |= readl();
>>
>> Why or the bit back in ? and then why not check the bit was set on the
>> read ?
>>
>> val = readl() is a lot less janky and shouldn't it matter that the bit
>> we tried to set is actually reflected in the read-back ?
>>
>> Failure to set the bit would certainly be a problem...
>>
>> ---
>> bod