linux-kernel - Re: [PATCH v1 5/5] remoteproc: add support for Microchip IPC remoteproc platform 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] [day] [month] [year] [list]

Message-ID: <275b1b46-4ed0-44d8-a240-93a422e96bbf@kernel.org>
Date: Sun, 22 Sep 2024 22:21:08 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Valentina.FernandezAlanis@...rochip.com, paul.walmsley@...ive.com,
 palmer@...belt.com, aou@...s.berkeley.edu, peterlin@...estech.com,
 dminus@...estech.com, Conor.Dooley@...rochip.com, conor+dt@...nel.org,
 ycliang@...estech.com, jassisinghbrar@...il.com, robh@...nel.org,
 krzk+dt@...nel.org, andersson@...nel.org, mathieu.poirier@...aro.org
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v1 5/5] remoteproc: add support for Microchip IPC
 remoteproc platform driver

On 18/09/2024 17:51, Valentina.FernandezAlanis@...rochip.com wrote:
> On 16/09/2024 21:18, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 12/09/2024 19:00, Valentina Fernandez wrote:
>>> The Microchip family of RISC-V SoCs typically has one or more clusters.
>>> These clusters can be configured to run in Asymmetric Multi-Processing
>>> (AMP) mode.
>>>
>>> Add a remoteproc platform driver to be able to load and boot firmware
>>> to the remote processor(s).
>>
>> ...
>>
>>> +
>>> +static int mchp_ipc_rproc_prepare(struct rproc *rproc)
>>> +{
>>> +     struct mchp_ipc_rproc *priv = rproc->priv;
>>> +     struct device_node *np = priv->dev->of_node;
>>> +     struct rproc_mem_entry *mem;
>>> +     struct reserved_mem *rmem;
>>> +     struct of_phandle_iterator it;
>>> +     u64 device_address;
>>> +
>>> +     reinit_completion(&priv->start_done);
>>> +
>>> +     of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>>> +     while (of_phandle_iterator_next(&it) == 0) {
>>> +             /*
>>> +              * Ignore the first memory region which will be used vdev
>>> +              * buffer. No need to do extra handlings, rproc_add_virtio_dev
>>> +              * will handle it.
>>> +              */
>>> +             if (!strcmp(it.node->name, "vdev0buffer"))
>>
>> What? If you ignore the first, then why are you checking names? This
>> does not make sense. Especially that your binding did not say anything
>> about these phandles being somehow special.
> 
> The idea in the code above is to skip the vdev buffer allocation and 
> carveout registration. Later, when the remoteproc virtio driver 
> registers the virtio device (rproc_add_virtio_dev), it will attempt to 
> find the carveout. Since the carveout wasn’t registered, it will use the 
> first memory region from the parent by calling 
> of_reserved_mem_device_init_by_idx.
> 
> This behavior is based on some existing platform drivers. However, upon 
> further inspection, it seems that some newer drivers use 
> rproc_of_resm_mem_entry_init to allocate vdev buffers.
> 
> I will restructure this section and rephase/drop the comment.
> 
> With regards the bindings, I'll explain better all the memory regions 
> for v2.
> 
> Just for everyone’s information, we have the following use cases:
> 
> Early boot: Remote processors are booted by another entity before Linux, 
> so we only need to attach. For this mode, we require the resource table 
> as a memory region in the device tree.
> 
> Late boot - Linux is responsible for loading the firmware and starting 
> it on the remote processors. For this, we need the region used for the 
> firmware image.
> 
> In both cases, rpmsg communication is optional. This means that the vdev 
> buffers and vrings memory regions are also optional.
> 
> There could also be a mixed case where we start with early boot mode by 
> attaching to an existing remoteproc, and then stop, start, and load 
> another firmware once Linux has booted. In this case, we would require 
> the resource table and firmware image region, and optionally, vdev 
> buffers and vrings.
> 
>>
>>> +                     continue;
>>> +
>>> +             if (!strcmp(it.node->name, "rsc-table"))
>>
>> Nope.
> Since the resource table is only needed for early boot mode and does not 
> need to be a carveout region, we are skipping that.
> 
> I will work on making the resource table a fixed index in the 
> memory-region property so that it doesn't have a fixed name.

The list of memory-regions already HAS fixed indices. All this is not
only confusing, but incorrect. I commented that if I call the node
"rsc-not-a-table" your code will stop working.

Best regards,
Krzysztof