Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap

[John Stultz]

On Fri, Oct 16, 2020 at 12:03 PM John Stultz <john.stultz@xxxxxxxxxx> wrote:
> On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey <brian.starkey@xxxxxxx> wrote:
> > On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> > > @@ -426,6 +487,16 @@ static int system_heap_create(void)
> > >       if (IS_ERR(sys_heap))
> > >               return PTR_ERR(sys_heap);
> > >
> > > +     exp_info.name = "system-uncached";
> > > +     exp_info.ops = &system_uncached_heap_ops;
> > > +     exp_info.priv = NULL;
> > > +
> > > +     sys_uncached_heap = dma_heap_add(&exp_info);
> > > +     if (IS_ERR(sys_uncached_heap))
> > > +             return PTR_ERR(sys_heap);
> > > +
> >
> > In principle, there's a race here between the heap getting registered
> > to sysfs and the dma_mask getting updated.
> >
> > I don't think it would cause a problem in practice, but with the API
> > as it is, there's no way to avoid it - so I wonder if the
> > dma_heap_get_dev() accessor isn't the right API design.
>
> Hrm.  I guess to address your concern we would need split
> dma_heap_add() into something like:
>   dma_heap_create()
>   dma_heap_add()
>
> Which breaks the creation of the heap with the registering it to the
> subsystem, so some attributes can be tweaked inbetween?

Looking at this some more, this approach isn't going to work. We
create the device and then we call dma_coerce_mask_and_coherent() on
it, but as soon as the device is created it seems possible for
userland to directly access it. Again, though, as you mentioned this
isn't terribly likely in practice.

The best thing I can think of for now is to have the uncached heap's
allocate pointer initially point to a dummy function that returns
EBUSY and then after we update the dma mask then we can set it to the
real alloc.  I'll go with that for now, but let me know if you have
other suggestions.

thanks
-john