Re: [PATCH net 1/1] s390/ism: fix receive message buffer allocation

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2024-03-28 at 16:41 +0100, Gerd Bayer wrote:
> Since [1], dma_alloc_coherent() does not accept requests for GFP_COMP
> anymore, even on archs that may be able to fulfill this. Functionality that
> relied on the receive buffer being a compound page broke at that point:
> The SMC-D protocol, that utilizes the ism device driver, passes receive
> buffers to the splice processor in a struct splice_pipe_desc with a
> single entry list of struct pages. As the buffer is no longer a compound
> page, the splice processor now rejects requests to handle more than a
> page worth of data.
> 
> Replace dma_alloc_coherent() and allocate a buffer with kmalloc() then
> create a DMA map for it with dma_map_page(). Since only receive buffers
> on ISM devices use DMA, qualify the mapping as FROM_DEVICE.
> Since ISM devices are available on arch s390, only and on that arch all
> DMA is coherent, there is no need to introduce and export some kind of
> dma_sync_to_cpu() method to be called by the SMC-D protocol layer.
> 
> Analogously, replace dma_free_coherent by a two step dma_unmap_page,
> then kfree to free the receive buffer.
> 
> [1] https://lore.kernel.org/all/20221113163535.884299-1-hch@lst.de/
> 
> Fixes: c08004eede4b ("s390/ism: don't pass bogus GFP_ flags to dma_alloc_coherent")
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
>  drivers/s390/net/ism_drv.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
> index 2c8e964425dc..25911b887e5e 100644
> --- a/drivers/s390/net/ism_drv.c
> +++ b/drivers/s390/net/ism_drv.c
> @@ -14,6 +14,8 @@
>  #include <linux/err.h>
>  #include <linux/ctype.h>
>  #include <linux/processor.h>
> +#include <linux/dma-direction.h>
> +#include <linux/gfp_types.h>
>  
>  #include "ism.h"
>  
> @@ -292,13 +294,15 @@ static int ism_read_local_gid(struct ism_dev *ism)
>  static void ism_free_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
>  {
>  	clear_bit(dmb->sba_idx, ism->sba_bitmap);
> -	dma_free_coherent(&ism->pdev->dev, dmb->dmb_len,
> -			  dmb->cpu_addr, dmb->dma_addr);
> +	dma_unmap_page(&ism->pdev->dev, dmb->dma_addr, dmb->dmb_len,
> +		       DMA_FROM_DEVICE);
> +	kfree(dmb->cpu_addr);
>  }
>  
>  static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
>  {
>  	unsigned long bit;
> +	int rc;
>  
>  	if (PAGE_ALIGN(dmb->dmb_len) > dma_get_max_seg_size(&ism->pdev->dev))
>  		return -EINVAL;
> @@ -315,14 +319,27 @@ static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
>  	    test_and_set_bit(dmb->sba_idx, ism->sba_bitmap))
>  		return -EINVAL;
>  
> -	dmb->cpu_addr = dma_alloc_coherent(&ism->pdev->dev, dmb->dmb_len,
> -					   &dmb->dma_addr,
> -					   GFP_KERNEL | __GFP_NOWARN |
> -					   __GFP_NOMEMALLOC | __GFP_NORETRY);
> -	if (!dmb->cpu_addr)
> -		clear_bit(dmb->sba_idx, ism->sba_bitmap);
> +	dmb->cpu_addr = kmalloc(dmb->dmb_len, GFP_KERNEL | __GFP_NOWARN |
> +				__GFP_COMP | __GFP_NOMEMALLOC | __GFP_NORETRY);

Out of sheer ignorance on my side, the __GFP_COMP flag looks suspicious
here. I *think* that is relevant only for the page allocator. 

Why can't you use get_free_pages() (or similar) here? (possibly
rounding up to the relevant page_aligned size). 

Thanks!

Paolo