Re: [PATCH v7 08/12] firmware: qcom: qseecom: convert to using the TZ allocator

[Bartosz Golaszewski]

On Sun, Feb 18, 2024 at 4:08 AM Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
>
> On Mon, Feb 05, 2024 at 07:28:06PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> > convert all users of it in the qseecom module to using the TZ allocator
> > for creating SCM call buffers.
>
> This reads as if this is removal of duplication, now that we have the TZ
> allocation. But wasn't there something about you not being able to mix
> and match shmbridge and non-shmbridge allocations in the interface, so
> this transition is actually required? Or did I get that wrong and this
> just reduction in duplication?
>

What is the question exactly? Yes it is required because once we
enable SHM bridge, "normal" memory will no longer be accepted for SCM
calls.

> > Together with using the cleanup macros,
> > it has the added benefit of a significant code shrink.
>
> That is true, but the move to using cleanup macros at the same time as
> changing the implementation makes it unnecessarily hard to reason about
> this patch.
>
> This patch would be much better if split in two.
>

I disagree. If we have a better interface in place, then let's use it
right away, otherwise it's just useless churn.

> > As this is
> > largely a module separate from the SCM driver, let's use a separate
> > memory pool.
> >
>
> This module is effectively used to read and write EFI variables today.
> Is that worth statically removing 256kb of DDR for? Is this done solely
> because it logically makes sense, or did you choose this for a reason?
>

Well, it will stop working (with SHM bridge enabled) if we don't. We
can possibly release the pool once we know we'll no longer need to
access EFI variables but I'm not sure if that makes sense? Or maybe
remove the pool after some time of driver inactivity and create a new
one when it's needed again?

Bart

[snip]