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