Re: [PATCH] arch/powerpc: Use dma_zalloc_coherent

[Souptick Joarder <jrdr.linux@gmail.com>]

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);
>
>