[PATCH net 0/1] s390/ism: Fix splice for SMC-D

[PATCH net 0/1] s390/ism: Fix splice for SMC-D

[Gerd Bayer]

Hi all,

due to a change in the DMA API - to no longer provide compound
pages with dma_alloc_coherent - splice broke for the SMC-D protocol.
Here, I'm proposing a fix for that.

I'm aware that this is a rather coarse fix attempt and a proper
solution would be to rework the	SMC-D protocol to drop the requirement
for compound pages. That work might take some more time.

Meanwhile, I'd like to probe if this change in how DMA buffers for ISM
devices get allocated is acceptable as an interim solution.

With this change applied on top of current master, our test-cases for
SMC-D splice complete successfully again.

Gerd Bayer (1):
  s390/ism: fix receive message buffer allocation

 drivers/s390/net/ism_drv.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

-- 
2.44.0

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

[Gerd Bayer]

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);
+	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,
-- 
2.44.0

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

[Gerd Bayer]

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,

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

[Paolo Abeni]

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

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

[Gerd Bayer]

On Thu, 2024-04-04 at 10:13 +0200, Paolo Abeni wrote:
> 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")
> > 

[...]

> > @@ -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 for your suggestion. However, I wanted to stay as close to
the implementation pre [1] - that used to use __GFP_COMP, too. I'd
rather avoid to change interfaces from "cpu_addr" to "struct page*" at
this point. In the long run, I'd like to drop the requirement for
compound pages entirely, since that *appears* to exist primarily for a
simplified handling of the interface to splice_to_pipe() in
net/smc/smc_rx.c. And of course there might be performance
implications...

At this point, I'm more concerned about my usage of the DMA API with
this patch.

Thanks again,
Gerd

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

[Christoph Hellwig]

On Thu, Apr 04, 2024 at 01:10:20PM +0200, Gerd Bayer wrote:
> > Why can't you use get_free_pages() (or similar) here? (possibly
> > rounding up to the relevant page_aligned size). 
> 
> Thanks Paolo for your suggestion. However, I wanted to stay as close to
> the implementation pre [1] - that used to use __GFP_COMP, too. I'd
> rather avoid to change interfaces from "cpu_addr" to "struct page*" at
> this point. In the long run, I'd like to drop the requirement for

The right interface actually is to simply use folio_alloc, which adds
__GFP_COMP and is a fully supported and understood interface. You can
just convert the folio to a kernel virtual address using folio_address()
right after allocating it.

(get_free_pages also retunrs a kernel virtual address, just awkwardly as
an unsigned long. In doubt don't use this interface for new code..)

> compound pages entirely, since that *appears* to exist primarily for a
> simplified handling of the interface to splice_to_pipe() in
> net/smc/smc_rx.c. And of course there might be performance
> implications...

While compounds pages might sound awkward, they are the new normal in
form of folios.  So just use folios.

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

[Gerd Bayer]

On Fri, 2024-04-05 at 08:49 +0200, Christoph Hellwig wrote:
> On Thu, Apr 04, 2024 at 01:10:20PM +0200, Gerd Bayer wrote:
> > > Why can't you use get_free_pages() (or similar) here? (possibly
> > > rounding up to the relevant page_aligned size). 
> > 
> > Thanks Paolo for your suggestion. However, I wanted to stay as
> > close to the implementation pre [1] - that used to use __GFP_COMP,
> > too. I'd rather avoid to change interfaces from "cpu_addr" to
> > "struct page*" at this point. In the long run, I'd like to drop the
> > requirement for
> 
> The right interface actually is to simply use folio_alloc, which adds
> __GFP_COMP and is a fully supported and understood interface. You can
> just convert the folio to a kernel virtual address using
> folio_address() right after allocating it.

Thanks for pointing me to folios.
After a good night's sleep, I figured that I was thinking too
complicated when I dismissed Paolo's suggestion.

> (get_free_pages also retunrs a kernel virtual address, just awkwardly
> as an unsigned long. In doubt don't use this interface for new
> code..)
> 
> > compound pages entirely, since that *appears* to exist primarily
> > for a
> > simplified handling of the interface to splice_to_pipe() in
> > net/smc/smc_rx.c. And of course there might be performance
> > implications...
> 
> While compounds pages might sound awkward, they are the new normal in
> form of folios.  So just use folios.

With the following fixup, my tests were just as successful.
I'll send that out as a v2.

Thank you, Christoph and Paolo!



diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index 25911b887e5e..affb05521e14 100644
--- a/drivers/s390/net/ism_drv.c
+++ b/drivers/s390/net/ism_drv.c
@@ -14,8 +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 <linux/dma-mapping.h>
+#include <linux/mm.h>
 
 #include "ism.h"
 
@@ -296,7 +296,7 @@ static void ism_free_dmb(struct ism_dev *ism,
struct ism_dmb *dmb)
 	clear_bit(dmb->sba_idx, ism->sba_bitmap);
 	dma_unmap_page(&ism->pdev->dev, dmb->dma_addr, dmb->dmb_len,
 		       DMA_FROM_DEVICE);
-	kfree(dmb->cpu_addr);
+	folio_put(virt_to_folio(dmb->cpu_addr));
 }
 
 static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
@@ -319,8 +319,11 @@ 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 = kmalloc(dmb->dmb_len, GFP_KERNEL |
__GFP_NOWARN |
-				__GFP_COMP | __GFP_NOMEMALLOC |
__GFP_NORETRY);
+	dmb->cpu_addr =
+		folio_address(folio_alloc(GFP_KERNEL | __GFP_NOWARN |
+					  __GFP_NOMEMALLOC |
__GFP_NORETRY,
+					  get_order(dmb->dmb_len)));
+
 	if (!dmb->cpu_addr) {
 		rc = -ENOMEM;
 		goto out_bit;
-- 
2.44.0

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

[Niklas Schnelle]

On Fri, 2024-04-05 at 12:42 +0200, Gerd Bayer wrote:
> On Fri, 2024-04-05 at 08:49 +0200, Christoph Hellwig wrote:
> > On Thu, Apr 04, 2024 at 01:10:20PM +0200, Gerd Bayer wrote:
> > > > Why can't you use get_free_pages() (or similar) here? (possibly
> > > > rounding up to the relevant page_aligned size). 
> > > 
> > > Thanks Paolo for your suggestion. However, I wanted to stay as
> > > close to the implementation pre [1] - that used to use __GFP_COMP,
> > > too. I'd rather avoid to change interfaces from "cpu_addr" to
> > > "struct page*" at this point. In the long run, I'd like to drop the
> > > requirement for
> > 
> > The right interface actually is to simply use folio_alloc, which adds
> > __GFP_COMP and is a fully supported and understood interface. You can
> > just convert the folio to a kernel virtual address using
> > folio_address() right after allocating it.
> 
> Thanks for pointing me to folios.
> After a good night's sleep, I figured that I was thinking too
> complicated when I dismissed Paolo's suggestion.
> 
> > (get_free_pages also retunrs a kernel virtual address, just awkwardly
> > as an unsigned long. In doubt don't use this interface for new
> > code..)
> > 
> > > compound pages entirely, since that *appears* to exist primarily
> > > for a
> > > simplified handling of the interface to splice_to_pipe() in
> > > net/smc/smc_rx.c. And of course there might be performance
> > > implications...
> > 
> > While compounds pages might sound awkward, they are the new normal in
> > form of folios.  So just use folios.
> 
> With the following fixup, my tests were just as successful.
> I'll send that out as a v2.
> 
> Thank you, Christoph and Paolo!
> 
> 
> 
> diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
> index 25911b887e5e..affb05521e14 100644
> --- a/drivers/s390/net/ism_drv.c
> +++ b/drivers/s390/net/ism_drv.c
> @@ -14,8 +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 <linux/dma-mapping.h>
> +#include <linux/mm.h>
>  
>  #include "ism.h"
>  
> @@ -296,7 +296,7 @@ static void ism_free_dmb(struct ism_dev *ism,
> struct ism_dmb *dmb)
>  	clear_bit(dmb->sba_idx, ism->sba_bitmap);
>  	dma_unmap_page(&ism->pdev->dev, dmb->dma_addr, dmb->dmb_len,
>  		       DMA_FROM_DEVICE);
> -	kfree(dmb->cpu_addr);
> +	folio_put(virt_to_folio(dmb->cpu_addr));
>  }
>  
>  static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
> @@ -319,8 +319,11 @@ 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 = kmalloc(dmb->dmb_len, GFP_KERNEL |
> __GFP_NOWARN |
> -				__GFP_COMP | __GFP_NOMEMALLOC |
> __GFP_NORETRY);
> +	dmb->cpu_addr =
> +		folio_address(folio_alloc(GFP_KERNEL | __GFP_NOWARN |
> +					  __GFP_NOMEMALLOC |

Personally I'd go with a temporary variable here if only to make the
lines a bit shorter and easier to read. I also think above is not
correct for allocation failure since folio_address() accesses folio-
>page without first checking for NULL. So I'm guessing the NULL check
needs to move and be done on the temporary struct folio*.

> __GFP_NORETRY,
> +					  get_order(dmb->dmb_len)));
> +
>  	if (!dmb->cpu_addr) {
>  		rc = -ENOMEM;
>  		goto out_bit;

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

[Christoph Hellwig]

On Fri, Apr 05, 2024 at 01:29:55PM +0200, Niklas Schnelle wrote:
> Personally I'd go with a temporary variable here if only to make the
> lines a bit shorter and easier to read. I also think above is not
> correct for allocation failure since folio_address() accesses folio-
> >page without first checking for NULL. So I'm guessing the NULL check
> needs to move and be done on the temporary struct folio*.

Yes, it needs a local variable to NULL check the folio_alloc return.

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

[Gerd Bayer]

On Fri, 2024-04-05 at 16:36 +0200, Christoph Hellwig wrote:
> On Fri, Apr 05, 2024 at 01:29:55PM +0200, Niklas Schnelle wrote:
> > Personally I'd go with a temporary variable here if only to make
> > the
> > lines a bit shorter and easier to read. I also think above is not
> > correct for allocation failure since folio_address() accesses
> > folio-
> > > page without first checking for NULL. So I'm guessing the NULL
> > > check
> > needs to move and be done on the temporary struct folio*.
> 
> Yes, it needs a local variable to NULL check the folio_alloc return.
> 

Hi, just a heads-up:

v2 that still missed this check got picked then dropped through the
netdev tree. Meanwhile I've sent new proper patch to this list of
recipients:

https://lore.kernel.org/all/20240415131507.156931-1-gbayer@linux.ibm.com/

Thanks,
Gerd