On Tue, Feb 20, 2024 at 10:54:02AM +0100, Bartosz Golaszewski wrote:
> On Sun, Feb 18, 2024 at 4:08 AM Bjorn Andersson <[email protected]> wrote:
> >
> > On Mon, Feb 05, 2024 at 07:28:06PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > 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]
On Tue, Feb 27, 2024 at 5:53 PM Bjorn Andersson <[email protected]> wrote:
>
> On Tue, Feb 20, 2024 at 10:54:02AM +0100, Bartosz Golaszewski wrote:
> >
> > 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.
>
This series would be 50% bigger for no reason if we split every patch
using the new allocator into two. I absolutely don't see how this
makes any sense. We're removing the calls to old interfaces and using
the new ones instead. The new ones happen to support cleanup so we use
it right away. If the old ones supported cleanup then maybeeee it
would make some sense to convert them first and then use tzmem. As it
is, there's really no point.
Bart
On Sun, Mar 03, 2024 at 05:18:18PM +0100, Bartosz Golaszewski wrote:
> On Tue, Feb 27, 2024 at 5:53 PM Bjorn Andersson <[email protected]> wrote:
> >
> > On Tue, Feb 20, 2024 at 10:54:02AM +0100, Bartosz Golaszewski wrote:
> > >
> > > 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.
> >
>
> This series would be 50% bigger for no reason if we split every patch
> using the new allocator into two.
I'm not asking you to split every patch into two, unless that makes
sense.
> I absolutely don't see how this makes any sense.
I find it unnecessarily hard to determine which parts of _this_ patch is
functional and which is cleanup.
> We're removing the calls to old interfaces and using
> the new ones instead. The new ones happen to support cleanup so we use
> it right away. If the old ones supported cleanup then maybeeee it
> would make some sense to convert them first and then use tzmem. As it
> is, there's really no point.
>
The old interface is kzalloc(). I haven't used the cleanup mechanism
myself yet, but are you saying that there's no cleanup support for
kzalloc()?
Regards,
Bjorn