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