Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1551819273-640-1-git-send-email-john.stultz@linaro.org>
 <1551819273-640-3-git-send-email-john.stultz@linaro.org> <20190315090646.GC4470@infradead.org>
In-Reply-To: <20190315090646.GC4470@infradead.org>
From:   John Stultz <john.stultz@linaro.org>
Date:   Thu, 21 Mar 2019 13:01:12 -0700
Message-ID: <CALAqxLXBC1x230qNgtxFdJ46tqqR8b_4o7rakH3GWjm--xX2=A@mail.gmail.com>
Subject: Re: [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers
To:     Christoph Hellwig <hch@infradead.org>
Cc:     lkml <linux-kernel@vger.kernel.org>,
        Laura Abbott <labbott@redhat.com>,
        Benjamin Gaignard <benjamin.gaignard@linaro.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Liam Mark <lmark@codeaurora.org>,
        Brian Starkey <Brian.Starkey@arm.com>,
        "Andrew F . Davis" <afd@ti.com>, Chenbo Feng <fengc@google.com>,
        Alistair Strachan <astrachan@google.com>,
        dri-devel <dri-devel@lists.freedesktop.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 15, 2019 at 2:06 AM Christoph Hellwig <hch@infradead.org> wrote:
> > +     if (buffer->kmap_cnt) {
> > +             buffer->kmap_cnt++;
> > +             return buffer->vaddr;
> > +     }
> > +     vaddr = dma_heap_map_kernel(buffer);
> > +     if (WARN_ONCE(!vaddr,
> > +                   "heap->ops->map_kernel should return ERR_PTR on error"))
> > +             return ERR_PTR(-EINVAL);
> > +     if (IS_ERR(vaddr))
> > +             return vaddr;
> > +     buffer->vaddr = vaddr;
> > +     buffer->kmap_cnt++;
>
> The cnt manioulation is odd.  The normal way to make this readable
> is to use a postfix op on the check, as that makes it clear to everyone,
> e.g.:
>
>         if (buffer->kmap_cnt++)
>                 return buffer->vaddr;
>         ..

Thanks again on the feedback, I have had some other distractions
recently, so just getting around to these details again now.

The trouble w/ the suggestion here, if we increment in the check, then
if we trip on any of the other error paths, the cnt value will be
wrong when we return (and doing the extra decrementing in the error
paths feels as ugly as just doing the increment at the end of the
success paths)

> > +     buffer->kmap_cnt--;
> > +     if (!buffer->kmap_cnt) {
> > +             vunmap(buffer->vaddr);
> > +             buffer->vaddr = NULL;
> > +     }
>
> Same here, just with an infix.
>
> > +static inline void INIT_HEAP_HELPER_BUFFER(struct heap_helper_buffer *buffer,
> > +                              void (*free)(struct heap_helper_buffer *))
> > +{
> > +     buffer->private_flags = 0;
> > +     buffer->priv_virt = NULL;
> > +     mutex_init(&buffer->lock);
> > +     buffer->kmap_cnt = 0;
> > +     buffer->vaddr = NULL;
> > +     buffer->sg_table = NULL;
> > +     INIT_LIST_HEAD(&buffer->attachments);
> > +     buffer->free = free;
> > +}
>
> There is absolutely no reason to inlines this as far as I can tell.

Yea, I think I was mimicing some of the helpers like INIT_LIST_HEAD()
But sounds good. I can uninline it.

> Also it would seem much simpler to simply let the caller assign the
> free callback.

Yea, its a bit ugly but I worry the caller might forget?

Thanks again for the feedback!
-john