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: <5bdfb8db.1c69fb81.c8f62.9586@mx.google.com> <CAJr6mf1nOouU31WF_xb_46wDebPQ-7+NTCiSKBjLyQta2mny6w@mail.gmail.com>
 <7a3922bc5089dfe0978e07a809606360c74c940c.camel@perches.com>
In-Reply-To: <7a3922bc5089dfe0978e07a809606360c74c940c.camel@perches.com>
From:   Souptick Joarder <jrdr.linux@gmail.com>
Date:   Sat, 17 Nov 2018 12:40:14 +0530
Message-ID: <CAFqt6zY2bv=G1N13E4-SA2DE6ey3kw378fFzgEHV7wdpJ5hp5g@mail.gmail.com>
Subject: Re: [PATCH] arch/powerpc: Use dma_zalloc_coherent
To:     Joe Perches <joe@perches.com>
Cc:     Sabyasachi Gupta <sabyasachi.linux@gmail.com>,
        benh@kernel.crashing.org, paulus@samba.org, mpe@ellerman.id.au,
        darren@stevens-zone.net, christophe.leroy@c-s.fr,
        geoff@infradead.org, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Joe,

On Fri, Nov 16, 2018 at 12:55 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2018-11-15 at 23:29 +0530, Sabyasachi Gupta wrote:
> > On Mon, Nov 5, 2018 at 8:58 AM Sabyasachi Gupta
> > <sabyasachi.linux@gmail.com> wrote:
> > > Replaced dma_alloc_coherent + memset with dma_zalloc_coherent
> > >
> > > Signed-off-by: Sabyasachi Gupta <sabyasachi.linux@gmail.com>
> >
> > Any comment on this patch?
>
> It's obviously correct.
>
> You might realign the arguments on the next lines
> to the open parenthesis.
>
> Perhaps there should be new function calls
> added for symmetry to the other alloc functions
> for multiplication overflow protection.
>
> Perhaps:
>
> void *dma_alloc_array_coherent()
> void *dma_calloc_coherent()
>
> Something like
> ---
>  include/linux/dma-mapping.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 15bd41447025..95bebf8883b1 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -565,6 +565,25 @@ static inline void *dma_alloc_coherent(struct device *dev, size_t size,
>                         (gfp & __GFP_NOWARN) ? DMA_ATTR_NO_WARN : 0);
>  }
>
> +static inline void *dma_alloc_array_coherent(struct device *dev,
> +                                            size_t n, size_t size,
> +                                            dma_addr_t *dma_handle, gfp_t gfp)
> +{
> +       size_t bytes;
> +
> +       if (unlikely(check_mul_overflow(n, size, &bytes)))
> +               return NULL;
> +       return dma_alloc_coherent(dev, bytes, dma_handle, gfp);
> +}
> +
> +static inline void *dma_calloc_coherent(struct device *dev,
> +                                       size_t n, size_t size,
> +                                       dma_addr_t *dma_handle, gfp_t gfp)
> +{
> +       return dma_alloc_array_coherent(dev, n, size, dma_handle,
> +                                       gfp | __GFP_ZERO);
> +}
> +

If I understood correctly, you are talking about adding these 2 new inline
functions. We can do that.

Can you please help to understand the consumers of these 2 new inline ?

>  static inline void dma_free_coherent(struct device *dev, size_t size,
>                 void *cpu_addr, dma_addr_t dma_handle)
>  {
>
> ---
> > > diff --git a/arch/powerpc/platforms/pasemi/dma_lib.c b/arch/powerpc/platforms/pasemi/dma_lib.c
> []
> > > @@ -255,15 +255,13 @@ int pasemi_dma_alloc_ring(struct pasemi_dmachan *chan, int ring_size)
> > >
> > >         chan->ring_size = ring_size;
> > >
> > > -       chan->ring_virt = dma_alloc_coherent(&dma_pdev->dev,
> > > +       chan->ring_virt = dma_zalloc_coherent(&dma_pdev->dev,
> > >                                              ring_size * sizeof(u64),
> > >                                              &chan->ring_dma, GFP_KERNEL);
> > >  en
> > >         if (!chan->ring_virt)
> > >                 return -ENOMEM;
> > >
> > > -       memset(chan->ring_virt, 0, ring_size * sizeof(u64));
> > > -
> > >         return 0;
> > >  }
> > >  EXPORT_SYMBOL(pasemi_dma_alloc_ring);
>
>