Received-SPF: pass (google.com: domain of linux-kernel+bounces-83656-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 27 Feb 2024 10:53:39 -0600
From: Bjorn Andersson <andersson@kernel.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Andy Gross <agross@kernel.org>, 
	Konrad Dybcio <konrad.dybcio@linaro.org>, Elliot Berman <quic_eberman@quicinc.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Guru Das Srinagesh <quic_gurus@quicinc.com>, 
	Andrew Halaney <ahalaney@redhat.com>, Maximilian Luz <luzmaximilian@gmail.com>, 
	Alex Elder <elder@linaro.org>, Srini Kandagatla <srinivas.kandagatla@linaro.org>, 
	Arnd Bergmann <arnd@arndb.de>, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, kernel@quicinc.com, 
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>, Deepti Jaggi <quic_djaggi@quicinc.com>
Subject: Re: [PATCH v7 08/12] firmware: qcom: qseecom: convert to using the
 TZ allocator
Message-ID: <odnisr4flot3rgwwqpjob3qtw63jow7hcj4guy6ao6spdz6fm4@wtcm62o3hgxm>
References: <20240205182810.58382-1-brgl@bgdev.pl>
 <20240205182810.58382-9-brgl@bgdev.pl>
 <yu5uhamdlygti3jo73ipy3gxhcmgxrm5g6imgqg6ksleim4ton@npvzlex2j4xi>
 <CAMRc=Mctm-cyYPpu-Vb+fr1cWJPUW49shaa9HEXYp+rkF_CHUw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMRc=Mctm-cyYPpu-Vb+fr1cWJPUW49shaa9HEXYp+rkF_CHUw@mail.gmail.com>

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]