Re: [RFC PATCH 1/1] s390/cio: make ccw_device_dma_* more robust

[Halil Pasic <pasic@linux.ibm.com>]

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