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

Gerd Bayer
2024-03-28T16:00:06Z

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

Late adding Christoph as the "blamed" committer.

> 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);
> + if (!dmb->cpu_addr) {
> + rc = -ENOMEM;
> + goto out_bit;
> + }
> + dmb->dma_addr = dma_map_page(&ism->pdev->dev,
> +      virt_to_page(dmb->cpu_addr), 0,
> +      dmb->dmb_len, DMA_FROM_DEVICE);
> + if (dma_mapping_error(&ism->pdev->dev, dmb->dma_addr)) {
> + rc = -ENOMEM;
> + goto out_free;
> + }
> +
> + return 0;
>
> - return dmb->cpu_addr ? 0 : -ENOMEM;
> +out_free:
> + kfree(dmb->cpu_addr);
> +out_bit:
> + clear_bit(dmb->sba_idx, ism->sba_bitmap);
> + return rc;
>  }
>
>  int ism_register_dmb(struct ism_dev *ism, struct ism_dmb *dmb,