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

Bjorn Andersson
2024-02-27T16:53:42Z

On Tue, Feb 20, 2024 at 10:54:02AM +0100, Bartosz Golaszewski wrote:
> On Sun, Feb 18, 2024 at 4:08 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Mon, Feb 05, 2024 at 07:28:06PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > 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.
>

This fact is not covered anywhere in the series.

> > > 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.
>

The functional change and the use of cleanup macros, could be done
independently of each other, each one fully beneficial on their own.

As such I don't find it hard to claim that they are two independent
changes.

> > > 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?
>

Sounds like a good motivation to me, let's document it so that the next
guy understand why this was done.

Regards,
Bjorn

> Bart
>
> [snip]