Messages in this thread Patch in this message | | | Date | Mon, 11 Oct 2021 20:42:49 +0200 | From | Halil Pasic <> | Subject | Re: [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); > > > >
| |