[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <odnisr4flot3rgwwqpjob3qtw63jow7hcj4guy6ao6spdz6fm4@wtcm62o3hgxm>
Date: Tue, 27 Feb 2024 10:53:39 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Andy Gross <agross@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>, Elliot Berman <quic_eberman@...cinc.com>,
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Guru Das Srinagesh <quic_gurus@...cinc.com>,
Andrew Halaney <ahalaney@...hat.com>, Maximilian Luz <luzmaximilian@...il.com>,
Alex Elder <elder@...aro.org>, Srini Kandagatla <srinivas.kandagatla@...aro.org>,
Arnd Bergmann <arnd@...db.de>, linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, kernel@...cinc.com,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Deepti Jaggi <quic_djaggi@...cinc.com>
Subject: Re: [PATCH v7 08/12] firmware: qcom: qseecom: convert to using the
TZ allocator
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@...nel.org> wrote:
> >
> > On Mon, Feb 05, 2024 at 07:28:06PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.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]
Powered by blists - more mailing lists