lkml.org 
[lkml]   [2021]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC PATCH 1/1] s390/cio: make ccw_device_dma_* more robust
On Mon, 11 Oct 2021 15:45:55 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> > + if (IS_ERR_OR_NULL(addr))
>
> I can be wrong but it seems that only dma_alloc_coherent() used in
> cio_gp_dma_zalloc() report an error but the error is ignored and used as
> a valid pointer.


https://www.kernel.org/doc/Documentation/DMA-API.txt says:

Part Ia - Using large DMA-coherent buffers
------------------------------------------

::

void *
dma_alloc_coherent(struct device *dev, size_t size,
dma_addr_t *dma_handle, gfp_t flag)

[..]

It returns a pointer to the allocated region (in the processor's virtual
address space) or NULL if the allocation failed.

I hope that is still true. If not we should fix cio_gp_dma_zalloc().

>
> So shouldn't we modify this function and just test for a NULL address here?
>

Isn't IS_ERR_OR_NULL() safer, in a sense that even if we decided to
eventually return an error code, this piece of code would be robust
and safe?

We may exploit the knowledge that cio_gp_dma_zalloc() either
returns NULL or a valid pointer, but doing it like this is IMHO also an
option.

> here what I mean:---------------------------------
>
> diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> index 2bc55ccf3f23..b45fbaa7131b 100644
> --- a/drivers/s390/cio/css.c
> +++ b/drivers/s390/cio/css.c
> @@ -1176,7 +1176,7 @@ void *cio_gp_dma_zalloc(struct gen_pool *gp_dma,
> struct device *dma_dev,
> chunk_size = round_up(size, PAGE_SIZE);
> addr = (unsigned long) dma_alloc_coherent(dma_dev,
> chunk_size, &dma_addr,
> CIO_DMA_GFP);
> - if (!addr)
> + if (IS_ERR_OR_NULL(addr))
> return NULL;
> gen_pool_add_virt(gp_dma, addr, dma_addr, chunk_size, -1);
> addr = gen_pool_alloc(gp_dma, size);
>
> ---------------------------------
>
> > + put_device(&cdev->dev);
>
> addr is not null if addr is ERR.
>

Your point?

> > + return addr;
>
> may be return IS_ERR_OR_NULL(addr)? NULL : addr;
>

See above. I don't think that is necessary.

> > }
> > EXPORT_SYMBOL(ccw_device_dma_zalloc);
> >
> > void ccw_device_dma_free(struct ccw_device *cdev, void *cpu_addr, size_t size)
> > {
> > + if (!cpu_addr)
> > + return;
>
> no need, cpu_addr is already tested in cio_gp_dma_free()
>

This is added in because of the put_device(). An alternative would be
to call cio_gp_dma_free() unconditionally do the check just for the
put_device(). But I like this one better.

Thanks for your feedback!

Halil
> > cio_gp_dma_free(cdev->private->dma_pool, cpu_addr, size);
> > + put_device(&cdev->dev);
> > }
> > EXPORT_SYMBOL(ccw_device_dma_free);
> >
> >

\
 
 \ /
  Last update: 2021-10-11 20:45    [W:0.026 / U:0.816 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site