linux-kernel - Re: [PATCH v2 2/2] firmware: add exynos acpm 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: <88800ef0-2fb5-41f8-a303-e149ade7ed47@linaro.org>
Date: Fri, 25 Oct 2024 11:00:52 +0100
From: Tudor Ambarus <tudor.ambarus@...aro.org>
To: Krzysztof Kozlowski <krzk@...nel.org>, jassisinghbrar@...il.com
Cc: alim.akhtar@...sung.com, mst@...hat.com, javierm@...hat.com,
 tzimmermann@...e.de, bartosz.golaszewski@...aro.org,
 luzmaximilian@...il.com, sudeep.holla@....com, conor.dooley@...rochip.com,
 bjorn@...osinc.com, ulf.hansson@...aro.org,
 linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, marcan@...can.st, neal@...pa.dev,
 alyssa@...enzweig.io, broonie@...nel.org, andre.draszik@...aro.org,
 willmcvicker@...gle.com, peter.griffin@...aro.org, kernel-team@...roid.com,
 vincent.guittot@...aro.org, daniel.lezcano@...aro.org
Subject: Re: [PATCH v2 2/2] firmware: add exynos acpm driver



On 10/24/24 10:36 AM, Krzysztof Kozlowski wrote:
> On 23/10/2024 11:53, Tudor Ambarus wrote:
>>
>>
>> On 10/23/24 10:00 AM, Krzysztof Kozlowski wrote:
>>>>>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>>>>> I'm not writing any SRAM configuration fields, these fields are used to
>>>>>> read/retrive the channel parameters from SRAM.
>>>>> I meany tx_base is always 0. Where is this property set? Ever?
>>>> It's not zero. My assumption is it is set in the acpm firmware, but I
>>> Where is any assignment to this member?
>>
>> In probe() you'll see that exynos_acpm->shmem is a pointer in SRAM to a
>> struct exynos_acpm_shmem __iomem *shmem;
>>
>> Then in:
>>
>> static int exynos_acpm_chans_init()
>> {
>> 	struct exynos_acpm_shmem_chan __iomem *shmem_chans, *shmem_chan;
>> 	struct exynos_acpm_shmem __iomem *shmem = exynos_acpm->shmem;
>> 	...
>>
>> 	shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
>> 						 &shmem->chans);
>> 	...
>> }
>>
>> shmem->chans is not initialized (or tx_base). I'm using its address in
>> SRAM (&shmem->chans) which I then read it with readl_relaxed().
>>
>> I guess one can do the same using offsetof:
>> shmem_chans = readl_realaxed(shmem + offsetof(struct exynos_acpm_shmem,
>> 					      chans));
>>
> 
> I see, the code and the naming is confusing. Two exynos_acpm_shmem_chan

Noted. I'll refactor exynos_acpm_chans_init() in the next version.

> variables and one exynos_acpm_shmem. shmem_chans is used as an array,
> but nowhere pointed or indicated that it is array of some size.
>

I understand , will update. I added documentation for `struct
exynos_acpm_shmem` describing the array of chans and the number of
chans, but I'll figure something more, to be clearer.

> All this could be clearer if exynos_acpm_shmem_chan was packed, because
> then it is obvious it points to defined memory, but maybe packed is not

__packed shall be alright, but it's not needed because all the members
of the struct are u32 and the address of the struct is u64 aligned.

> correct here? Probably splitting all this into logical chunks would be
> useful. Like not mixing reading offsets with reading values, because I
> really have to spend a lot of time to identify which one is which in
> exynos_acpm_chans_init().
> 

I understand, will update. Need to figure out what other options we have
with the mailbox core changes first. Thanks for the suggestions!

Cheers,
ta