| Message ID | 20240409113753.2181368-1-gbayer@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d51dc8dd6ab6f93a894ff8b38d3b8d02c98eb9fb |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] Revert "s390/ism: fix receive message buffer allocation" | expand |
Hi, On Tue, 2024-04-09 at 13:37 +0200, Gerd Bayer wrote: > This reverts commit 58effa3476536215530c9ec4910ffc981613b413. > Review was not finished on this patch. So it's not ready for > upstreaming. > > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com> It's not a big deal (no need to repost), but should the need arise again in the future it would be better explicitly marking the reverted commit in the tag area as 'Fixes'. The full hash in the commit message will likely save the day to stable teams, but better safe then sorry! Thanks, Paolo
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Tue, 9 Apr 2024 13:37:53 +0200 you wrote: > This reverts commit 58effa3476536215530c9ec4910ffc981613b413. > Review was not finished on this patch. So it's not ready for > upstreaming. > > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com> > --- > > [...] Here is the summary with links: - [net] Revert "s390/ism: fix receive message buffer allocation" https://git.kernel.org/netdev/net/c/d51dc8dd6ab6 You are awesome, thank you!
On Thu, 2024-04-11 at 09:16 +0200, Paolo Abeni wrote: > Hi, > > On Tue, 2024-04-09 at 13:37 +0200, Gerd Bayer wrote: > > This reverts commit 58effa3476536215530c9ec4910ffc981613b413. > > Review was not finished on this patch. So it's not ready for > > upstreaming. > > > > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com> > > It's not a big deal (no need to repost), but should the need arise > again in the future it would be better explicitly marking the > reverted commit in the tag area as 'Fixes'. The full hash in the > commit message will likely save the day to stable teams, but better > safe then sorry! Thanks Paolo for the explanation. I was not even sure if the commit hash of the erroneous commit will remain stable when this tree will be merged upstream. In my (naive?) view this could be "autosquashed" into nothing at the time of the merge. But since there appears to be time for the next pull request to upstream, I'll send a new version of the original patch with all the review comments addressed. Thanks again, Gerd
On Thu, Apr 11, 2024 at 11:13:53AM +0200, Gerd Bayer wrote: > On Thu, 2024-04-11 at 09:16 +0200, Paolo Abeni wrote: > > Hi, > > > > On Tue, 2024-04-09 at 13:37 +0200, Gerd Bayer wrote: > > > This reverts commit 58effa3476536215530c9ec4910ffc981613b413. > > > Review was not finished on this patch. So it's not ready for > > > upstreaming. > > > > > > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com> > > > > It's not a big deal (no need to repost), but should the need arise > > again in the future it would be better explicitly marking the > > reverted commit in the tag area as 'Fixes'. The full hash in the > > commit message will likely save the day to stable teams, but better > > safe then sorry! > > Thanks Paolo for the explanation. I was not even sure if the commit > hash of the erroneous commit will remain stable when this tree will be > merged upstream. In my (naive?) view this could be "autosquashed" into > nothing at the time of the merge. net and net-next hashes are always stable. > But since there appears to be time for the next pull request to > upstream, I'll send a new version of the original patch with all the > review comments addressed. It looks like this commit was accepted, so we have what it is.
diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c index affb05521e14..2c8e964425dc 100644 --- a/drivers/s390/net/ism_drv.c +++ b/drivers/s390/net/ism_drv.c @@ -14,8 +14,6 @@ #include <linux/err.h> #include <linux/ctype.h> #include <linux/processor.h> -#include <linux/dma-mapping.h> -#include <linux/mm.h> #include "ism.h" @@ -294,15 +292,13 @@ 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_unmap_page(&ism->pdev->dev, dmb->dma_addr, dmb->dmb_len, - DMA_FROM_DEVICE); - folio_put(virt_to_folio(dmb->cpu_addr)); + dma_free_coherent(&ism->pdev->dev, dmb->dmb_len, + dmb->cpu_addr, dmb->dma_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; @@ -319,30 +315,14 @@ 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 = - folio_address(folio_alloc(GFP_KERNEL | __GFP_NOWARN | - __GFP_NOMEMALLOC | __GFP_NORETRY, - get_order(dmb->dmb_len))); + 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); - 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; - -out_free: - kfree(dmb->cpu_addr); -out_bit: - clear_bit(dmb->sba_idx, ism->sba_bitmap); - return rc; + return dmb->cpu_addr ? 0 : -ENOMEM; } int ism_register_dmb(struct ism_dev *ism, struct ism_dmb *dmb,
This reverts commit 58effa3476536215530c9ec4910ffc981613b413. Review was not finished on this patch. So it's not ready for upstreaming. Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com> --- Hi, this is broken for a little while already and I'd rather have the full solution properly reviewed and acked in one patch. So please do not upstream this yet. Thank you, Gerd drivers/s390/net/ism_drv.c | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) base-commit: b46f4eaa4f0ec38909fb0072eea3aeddb32f954e