On Mon, 11 Oct 2021 15:45:55 +0200 Pierre Morel <pmorel@xxxxxxxxxxxxx> 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); > > > >
