[PATCH v2 0/3] UIO_MEM_DMA_COHERENT for cnic/bnx2/bnx2x

[PATCH v2 0/3] UIO_MEM_DMA_COHERENT for cnic/bnx2/bnx2x

[Nilesh Javali]

During bnx2i iSCSI testing we ran into page refcounting issues in the
uio mmaps exported from cnic to the iscsiuio process, and bisected back
to the removal of the __GFP_COMP flag from dma_alloc_coherent calls.

In order to fix these drivers to be able to mmap dma coherent memory via
a uio device, without resorting to hacks and working with an iommu
enabled, introduce a new uio mmap type backed by dma_mmap_coherent.

While converting the uio interface, I also noticed that not all of these
allocations were PAGE_SIZE aligned. Particularly the bnx2/bnx2x status
block mapping was much smaller than any architecture page size, and I
was concerned that it could be unintentionally exposing kernel memory.

v2:
- expose only the dma_addr within uio and cnic.
- Cleanup newly added unions comprising virtual_addr
  and struct device

Chris Leech (3):
  uio: introduce UIO_MEM_DMA_COHERENT type
  cnic,bnx2,bnx2x: use UIO_MEM_DMA_COHERENT
  cnic,bnx2,bnx2x: page align uio mmap allocations

 drivers/net/ethernet/broadcom/bnx2.c          |  2 +
 .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 10 +++--
 drivers/net/ethernet/broadcom/cnic.c          | 26 ++++++++-----
 drivers/net/ethernet/broadcom/cnic.h          |  1 +
 drivers/net/ethernet/broadcom/cnic_if.h       |  1 +
 drivers/uio/uio.c                             | 38 +++++++++++++++++++
 include/linux/uio_driver.h                    |  2 +
 7 files changed, 67 insertions(+), 13 deletions(-)

-- 
2.23.1

[PATCH v2 1/3] uio: introduce UIO_MEM_DMA_COHERENT type

[Nilesh Javali]

From: Chris Leech <cleech@redhat.com>

Add a UIO memtype specifically for sharing dma_alloc_coherent
memory with userspace, backed by dma_mmap_coherent.

Signed-off-by: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Chris Leech <cleech@redhat.com>
---
v2:
- expose only the dma_addr within uio_mem
- Cleanup newly added unions comprising virtual_addr
  and struct device
 drivers/uio/uio.c          | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/uio_driver.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 62082d64ece0..9869703450d8 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -24,6 +24,7 @@
 #include <linux/kobject.h>
 #include <linux/cdev.h>
 #include <linux/uio_driver.h>
+#include <linux/dma-mapping.h>
 
 #define UIO_MAX_DEVICES		(1U << MINORBITS)
 
@@ -759,6 +760,40 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 			       vma->vm_page_prot);
 }
 
+static int uio_mmap_dma_coherent(struct vm_area_struct *vma)
+{
+	struct uio_device *idev = vma->vm_private_data;
+	struct uio_mem *mem;
+	int ret = 0;
+	int mi;
+
+	mi = uio_find_mem_index(vma);
+	if (mi < 0)
+		return -EINVAL;
+
+	mem = idev->info->mem + mi;
+
+	if (mem->dma_addr & ~PAGE_MASK)
+		return -ENODEV;
+	if (vma->vm_end - vma->vm_start > mem->size)
+		return -EINVAL;
+
+	/*
+	 * UIO uses offset to index into the maps for a device.
+	 * We need to clear vm_pgoff for dma_mmap_coherent.
+	 */
+	vma->vm_pgoff = 0;
+
+	ret = dma_mmap_coherent(&idev->dev,
+				vma,
+				(void *)mem->addr,
+				mem->dma_addr,
+				vma->vm_end - vma->vm_start);
+	vma->vm_pgoff = mi;
+
+	return ret;
+}
+
 static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 {
 	struct uio_listener *listener = filep->private_data;
@@ -806,6 +841,9 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	case UIO_MEM_VIRTUAL:
 		ret = uio_mmap_logical(vma);
 		break;
+	case UIO_MEM_DMA_COHERENT:
+		ret = uio_mmap_dma_coherent(vma);
+		break;
 	default:
 		ret = -EINVAL;
 	}
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 47c5962b876b..7efa81497183 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -37,6 +37,7 @@ struct uio_map;
 struct uio_mem {
 	const char		*name;
 	phys_addr_t		addr;
+	dma_addr_t		dma_addr;
 	unsigned long		offs;
 	resource_size_t		size;
 	int			memtype;
@@ -158,6 +159,7 @@ extern int __must_check
 #define UIO_MEM_LOGICAL	2
 #define UIO_MEM_VIRTUAL 3
 #define UIO_MEM_IOVA	4
+#define UIO_MEM_DMA_COHERENT	5
 
 /* defines for uio_port->porttype */
 #define UIO_PORT_NONE	0
-- 
2.23.1

[PATCH v2 2/3] cnic,bnx2,bnx2x: use UIO_MEM_DMA_COHERENT

[Nilesh Javali]

From: Chris Leech <cleech@redhat.com>

Use new UIO_MEM_DMA_COHERENT type to properly handle
mmap for dma_alloc_coherent buffers.

The cnic l2_ring and l2_buf mmaps have caused page
refcount issues as the dma_alloc_coherent no more
provide __GFP_COMP allocation as per commit,
dma-mapping: reject __GFP_COMP in dma_alloc_attrs.

Fix this by having the uio device use dma_mmap_coherent.

The bnx2 and bnx2x status block allocations are also dma_alloc_coherent,
and should use dma_mmap_coherent. They didn't allocate multiple pages,
but also didn't seem to work correctly with an iommu enabled.

Fixes: bb73955c0b1d ("cnic: don't pass bogus GFP_ flags to dma_alloc_coherent")
Signed-off-by: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Chris Leech <cleech@redhat.com>
---
v2:
- expose only the dma_addr from cnic to uio
- Cleanup newly added unions comprising virtual_addr
  and struct device
 drivers/net/ethernet/broadcom/bnx2.c           |  1 +
 .../net/ethernet/broadcom/bnx2x/bnx2x_main.c   |  2 ++
 drivers/net/ethernet/broadcom/cnic.c           | 18 ++++++++++++------
 drivers/net/ethernet/broadcom/cnic.h           |  1 +
 drivers/net/ethernet/broadcom/cnic_if.h        |  1 +
 5 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 0d917a9699c5..b65b8592ad75 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -367,6 +367,7 @@ static void bnx2_setup_cnic_irq_info(struct bnx2 *bp)
 	cp->irq_arr[0].status_blk = (void *)
 		((unsigned long) bnapi->status_blk.msi +
 		(BNX2_SBLK_MSIX_ALIGN_SIZE * sb_id));
+	cp->irq_arr[0].status_blk_map = bp->status_blk_mapping;
 	cp->irq_arr[0].status_blk_num = sb_id;
 	cp->num_irq = 1;
 }
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 0d8e61c63c7c..678829646cec 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -14912,9 +14912,11 @@ void bnx2x_setup_cnic_irq_info(struct bnx2x *bp)
 	else
 		cp->irq_arr[0].status_blk = (void *)bp->cnic_sb.e1x_sb;
 
+	cp->irq_arr[0].status_blk_map = bp->cnic_sb_mapping;
 	cp->irq_arr[0].status_blk_num =  bnx2x_cnic_fw_sb_id(bp);
 	cp->irq_arr[0].status_blk_num2 = bnx2x_cnic_igu_sb_id(bp);
 	cp->irq_arr[1].status_blk = bp->def_status_blk;
+	cp->irq_arr[1].status_blk_map = bp->def_status_blk_mapping;
 	cp->irq_arr[1].status_blk_num = DEF_SB_ID;
 	cp->irq_arr[1].status_blk_num2 = DEF_SB_IGU_ID;
 
diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
index 7926aaef8f0c..d9b52dc6d060 100644
--- a/drivers/net/ethernet/broadcom/cnic.c
+++ b/drivers/net/ethernet/broadcom/cnic.c
@@ -1107,6 +1107,7 @@ static int cnic_init_uio(struct cnic_dev *dev)
 						     TX_MAX_TSS_RINGS + 1);
 		uinfo->mem[1].addr = (unsigned long) cp->status_blk.gen &
 					CNIC_PAGE_MASK;
+		uinfo->mem[1].dma_addr = cp->status_blk_map;
 		if (cp->ethdev->drv_state & CNIC_DRV_STATE_USING_MSIX)
 			uinfo->mem[1].size = BNX2_SBLK_MSIX_ALIGN_SIZE * 9;
 		else
@@ -1116,22 +1117,25 @@ static int cnic_init_uio(struct cnic_dev *dev)
 	} else if (test_bit(CNIC_F_BNX2X_CLASS, &dev->flags)) {
 		uinfo->mem[0].size = pci_resource_len(dev->pcidev, 0);
 
-		uinfo->mem[1].addr = (unsigned long) cp->bnx2x_def_status_blk &
+		uinfo->mem[1].addr = (phys_addr_t)cp->bnx2x_def_status_blk &
 			CNIC_PAGE_MASK;
+		uinfo->mem[1].dma_addr = cp->status_blk_map;
 		uinfo->mem[1].size = sizeof(*cp->bnx2x_def_status_blk);
 
 		uinfo->name = "bnx2x_cnic";
 	}
 
-	uinfo->mem[1].memtype = UIO_MEM_LOGICAL;
+	uinfo->mem[1].memtype = UIO_MEM_DMA_COHERENT;
 
-	uinfo->mem[2].addr = (unsigned long) udev->l2_ring;
+	uinfo->mem[2].addr = (phys_addr_t)udev->l2_ring;
+	uinfo->mem[2].dma_addr = udev->l2_ring_map;
 	uinfo->mem[2].size = udev->l2_ring_size;
-	uinfo->mem[2].memtype = UIO_MEM_LOGICAL;
+	uinfo->mem[2].memtype = UIO_MEM_DMA_COHERENT;
 
-	uinfo->mem[3].addr = (unsigned long) udev->l2_buf;
+	uinfo->mem[3].addr = (phys_addr_t)udev->l2_buf;
+	uinfo->mem[3].dma_addr = udev->l2_buf_map;
 	uinfo->mem[3].size = udev->l2_buf_size;
-	uinfo->mem[3].memtype = UIO_MEM_LOGICAL;
+	uinfo->mem[3].memtype = UIO_MEM_DMA_COHERENT;
 
 	uinfo->version = CNIC_MODULE_VERSION;
 	uinfo->irq = UIO_IRQ_CUSTOM;
@@ -1313,6 +1317,7 @@ static int cnic_alloc_bnx2x_resc(struct cnic_dev *dev)
 		return 0;
 
 	cp->bnx2x_def_status_blk = cp->ethdev->irq_arr[1].status_blk;
+	cp->status_blk_map = cp->ethdev->irq_arr[1].status_blk_map;
 
 	cp->l2_rx_ring_size = 15;
 
@@ -5323,6 +5328,7 @@ static int cnic_start_hw(struct cnic_dev *dev)
 	pci_dev_get(dev->pcidev);
 	cp->func = PCI_FUNC(dev->pcidev->devfn);
 	cp->status_blk.gen = ethdev->irq_arr[0].status_blk;
+	cp->status_blk_map = ethdev->irq_arr[0].status_blk_map;
 	cp->status_blk_num = ethdev->irq_arr[0].status_blk_num;
 
 	err = cp->alloc_resc(dev);
diff --git a/drivers/net/ethernet/broadcom/cnic.h b/drivers/net/ethernet/broadcom/cnic.h
index 4baea81bae7a..fedc84ada937 100644
--- a/drivers/net/ethernet/broadcom/cnic.h
+++ b/drivers/net/ethernet/broadcom/cnic.h
@@ -260,6 +260,7 @@ struct cnic_local {
 		#define SM_RX_ID		0
 		#define SM_TX_ID		1
 	} status_blk;
+	dma_addr_t status_blk_map;
 
 	struct host_sp_status_block	*bnx2x_def_status_blk;
 
diff --git a/drivers/net/ethernet/broadcom/cnic_if.h b/drivers/net/ethernet/broadcom/cnic_if.h
index 789e5c7e9311..49a11ec80b36 100644
--- a/drivers/net/ethernet/broadcom/cnic_if.h
+++ b/drivers/net/ethernet/broadcom/cnic_if.h
@@ -190,6 +190,7 @@ struct cnic_ops {
 struct cnic_irq {
 	unsigned int	vector;
 	void		*status_blk;
+	dma_addr_t	status_blk_map;
 	u32		status_blk_num;
 	u32		status_blk_num2;
 	u32		irq_flags;
-- 
2.23.1

[PATCH v2 3/3] cnic,bnx2,bnx2x: page align uio mmap allocations

[Nilesh Javali]

From: Chris Leech <cleech@redhat.com>

Allocations in these drivers that will be mmaped through a uio device
should be made in multiples of PAGE_SIZE to avoid exposing additional
kernel memory unintentionally.

Signed-off-by: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Chris Leech <cleech@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2.c             | 1 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 8 ++++----
 drivers/net/ethernet/broadcom/cnic.c             | 8 +++++---
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index b65b8592ad75..490f88ad3bd2 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -838,6 +838,7 @@ bnx2_alloc_stats_blk(struct net_device *dev)
 						 BNX2_SBLK_MSIX_ALIGN_SIZE);
 	bp->status_stats_size = status_blk_size +
 				sizeof(struct statistics_block);
+	bp->status_stats_size = PAGE_ALIGN(bp->status_stats_size);
 	status_blk = dma_alloc_coherent(&bp->pdev->dev, bp->status_stats_size,
 					&bp->status_blk_mapping, GFP_KERNEL);
 	if (!status_blk)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 678829646cec..1d7be7b7c63f 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -8270,10 +8270,10 @@ void bnx2x_free_mem_cnic(struct bnx2x *bp)
 
 	if (!CHIP_IS_E1x(bp))
 		BNX2X_PCI_FREE(bp->cnic_sb.e2_sb, bp->cnic_sb_mapping,
-			       sizeof(struct host_hc_status_block_e2));
+			       PAGE_ALIGN(sizeof(struct host_hc_status_block_e2)));
 	else
 		BNX2X_PCI_FREE(bp->cnic_sb.e1x_sb, bp->cnic_sb_mapping,
-			       sizeof(struct host_hc_status_block_e1x));
+			       PAGE_ALIGN(sizeof(struct host_hc_status_block_e1x)));
 
 	BNX2X_PCI_FREE(bp->t2, bp->t2_mapping, SRC_T2_SZ);
 }
@@ -8316,12 +8316,12 @@ int bnx2x_alloc_mem_cnic(struct bnx2x *bp)
 	if (!CHIP_IS_E1x(bp)) {
 		/* size = the status block + ramrod buffers */
 		bp->cnic_sb.e2_sb = BNX2X_PCI_ALLOC(&bp->cnic_sb_mapping,
-						    sizeof(struct host_hc_status_block_e2));
+						    PAGE_ALIGN(sizeof(struct host_hc_status_block_e2)));
 		if (!bp->cnic_sb.e2_sb)
 			goto alloc_mem_err;
 	} else {
 		bp->cnic_sb.e1x_sb = BNX2X_PCI_ALLOC(&bp->cnic_sb_mapping,
-						     sizeof(struct host_hc_status_block_e1x));
+						     PAGE_ALIGN(sizeof(struct host_hc_status_block_e1x)));
 		if (!bp->cnic_sb.e1x_sb)
 			goto alloc_mem_err;
 	}
diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
index d9b52dc6d060..11a25d336221 100644
--- a/drivers/net/ethernet/broadcom/cnic.c
+++ b/drivers/net/ethernet/broadcom/cnic.c
@@ -1026,6 +1026,7 @@ static int __cnic_alloc_uio_rings(struct cnic_uio_dev *udev, int pages)
 		return 0;
 
 	udev->l2_ring_size = pages * CNIC_PAGE_SIZE;
+	udev->l2_ring_size = PAGE_ALIGN(udev->l2_ring_size);
 	udev->l2_ring = dma_alloc_coherent(&udev->pdev->dev, udev->l2_ring_size,
 					   &udev->l2_ring_map, GFP_KERNEL);
 	if (!udev->l2_ring)
@@ -1033,6 +1034,7 @@ static int __cnic_alloc_uio_rings(struct cnic_uio_dev *udev, int pages)
 
 	udev->l2_buf_size = (cp->l2_rx_ring_size + 1) * cp->l2_single_buf_size;
 	udev->l2_buf_size = CNIC_PAGE_ALIGN(udev->l2_buf_size);
+	udev->l2_buf_size = PAGE_ALIGN(udev->l2_buf_size);
 	udev->l2_buf = dma_alloc_coherent(&udev->pdev->dev, udev->l2_buf_size,
 					  &udev->l2_buf_map, GFP_KERNEL);
 	if (!udev->l2_buf) {
@@ -1109,9 +1111,9 @@ static int cnic_init_uio(struct cnic_dev *dev)
 					CNIC_PAGE_MASK;
 		uinfo->mem[1].dma_addr = cp->status_blk_map;
 		if (cp->ethdev->drv_state & CNIC_DRV_STATE_USING_MSIX)
-			uinfo->mem[1].size = BNX2_SBLK_MSIX_ALIGN_SIZE * 9;
+			uinfo->mem[1].size = PAGE_ALIGN(BNX2_SBLK_MSIX_ALIGN_SIZE * 9);
 		else
-			uinfo->mem[1].size = BNX2_SBLK_MSIX_ALIGN_SIZE;
+			uinfo->mem[1].size = PAGE_ALIGN(BNX2_SBLK_MSIX_ALIGN_SIZE);
 
 		uinfo->name = "bnx2_cnic";
 	} else if (test_bit(CNIC_F_BNX2X_CLASS, &dev->flags)) {
@@ -1120,7 +1122,7 @@ static int cnic_init_uio(struct cnic_dev *dev)
 		uinfo->mem[1].addr = (phys_addr_t)cp->bnx2x_def_status_blk &
 			CNIC_PAGE_MASK;
 		uinfo->mem[1].dma_addr = cp->status_blk_map;
-		uinfo->mem[1].size = sizeof(*cp->bnx2x_def_status_blk);
+		uinfo->mem[1].size = PAGE_ALIGN(sizeof(*cp->bnx2x_def_status_blk));
 
 		uinfo->name = "bnx2x_cnic";
 	}
-- 
2.23.1

Re: [PATCH v2 0/3] UIO_MEM_DMA_COHERENT for cnic/bnx2/bnx2x

[John Meneghini]

Nilesh,

Please explain what has been done to test these patches on bnx2i. Was testing done with iommu enabled/disabled?  Was testing 
done on any/all platforms, or was only x86_64 tested?

Thanks,

/John

On 1/3/24 04:11, Nilesh Javali wrote:
> During bnx2i iSCSI testing we ran into page refcounting issues in the
> uio mmaps exported from cnic to the iscsiuio process, and bisected back
> to the removal of the __GFP_COMP flag from dma_alloc_coherent calls.
> 
> In order to fix these drivers to be able to mmap dma coherent memory via
> a uio device, without resorting to hacks and working with an iommu
> enabled, introduce a new uio mmap type backed by dma_mmap_coherent.
> 
> While converting the uio interface, I also noticed that not all of these
> allocations were PAGE_SIZE aligned. Particularly the bnx2/bnx2x status
> block mapping was much smaller than any architecture page size, and I
> was concerned that it could be unintentionally exposing kernel memory.
> 
> v2:
> - expose only the dma_addr within uio and cnic.
> - Cleanup newly added unions comprising virtual_addr
>    and struct device
> 
> Chris Leech (3):
>    uio: introduce UIO_MEM_DMA_COHERENT type
>    cnic,bnx2,bnx2x: use UIO_MEM_DMA_COHERENT
>    cnic,bnx2,bnx2x: page align uio mmap allocations
> 
>   drivers/net/ethernet/broadcom/bnx2.c          |  2 +
>   .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 10 +++--
>   drivers/net/ethernet/broadcom/cnic.c          | 26 ++++++++-----
>   drivers/net/ethernet/broadcom/cnic.h          |  1 +
>   drivers/net/ethernet/broadcom/cnic_if.h       |  1 +
>   drivers/uio/uio.c                             | 38 +++++++++++++++++++
>   include/linux/uio_driver.h                    |  2 +
>   7 files changed, 67 insertions(+), 13 deletions(-)
> 

RE: [EXT] Re: [PATCH v2 0/3] UIO_MEM_DMA_COHERENT for cnic/bnx2/bnx2x

[Nilesh Javali]

John,

> -----Original Message-----
> From: John Meneghini <jmeneghi@redhat.com>
> Sent: Wednesday, January 3, 2024 9:01 PM
> To: Nilesh Javali <njavali@marvell.com>; martin.petersen@oracle.com;
> lduncan@suse.com; cleech@redhat.com
> Cc: linux-scsi@vger.kernel.org; GR-QLogic-Storage-Upstream <GR-QLogic-
> Storage-Upstream@marvell.com>; gregkh@linuxfoundation.org; Hannes
> Reinecke <hare@suse.de>; Jerry Snitselaar <jsnitsel@redhat.com>; Ming Lei
> <ming.lei@redhat.com>; Ewan Milne <emilne@redhat.com>
> Subject: [EXT] Re: [PATCH v2 0/3] UIO_MEM_DMA_COHERENT for
> cnic/bnx2/bnx2x
> 
> External Email
> 
> ----------------------------------------------------------------------
> Nilesh,
> 
> Please explain what has been done to test these patches on bnx2i. Was
> testing done with iommu enabled/disabled?  Was testing
> done on any/all platforms, or was only x86_64 tested?
> 

I have verified bnx2i/cnic/bnx2x driver with iommu disabled by default on my x86_64 setup with 6.7.0-rc1+ kernel.
At a high level the tests comprised of iSCSI target logins/logouts, IOs, multiple iterations of iscsid.service and iscsiuio.service restarts,
port perturbation, sg resets, ping test.

Thanks,
Nilesh

Re: [PATCH v2 1/3] uio: introduce UIO_MEM_DMA_COHERENT type

[kernel test robot]

Hi Nilesh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.7-rc8 next-20240104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nilesh-Javali/uio-introduce-UIO_MEM_DMA_COHERENT-type/20240103-171531
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20240103091137.27142-2-njavali%40marvell.com
patch subject: [PATCH v2 1/3] uio: introduce UIO_MEM_DMA_COHERENT type
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20240104/202401042222.J9GOUiYL-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240104/202401042222.J9GOUiYL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401042222.J9GOUiYL-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/uio/uio.c:27:
   drivers/uio/uio.c: In function 'uio_mmap_dma_coherent':
>> drivers/uio/uio.c:789:33: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     789 |                                 (void *)mem->addr,
         |                                 ^
   include/linux/dma-mapping.h:424:63: note: in definition of macro 'dma_mmap_coherent'
     424 | #define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h, s, 0)
         |                                                               ^


vim +789 drivers/uio/uio.c

   762	
   763	static int uio_mmap_dma_coherent(struct vm_area_struct *vma)
   764	{
   765		struct uio_device *idev = vma->vm_private_data;
   766		struct uio_mem *mem;
   767		int ret = 0;
   768		int mi;
   769	
   770		mi = uio_find_mem_index(vma);
   771		if (mi < 0)
   772			return -EINVAL;
   773	
   774		mem = idev->info->mem + mi;
   775	
   776		if (mem->dma_addr & ~PAGE_MASK)
   777			return -ENODEV;
   778		if (vma->vm_end - vma->vm_start > mem->size)
   779			return -EINVAL;
   780	
   781		/*
   782		 * UIO uses offset to index into the maps for a device.
   783		 * We need to clear vm_pgoff for dma_mmap_coherent.
   784		 */
   785		vma->vm_pgoff = 0;
   786	
   787		ret = dma_mmap_coherent(&idev->dev,
   788					vma,
 > 789					(void *)mem->addr,
   790					mem->dma_addr,
   791					vma->vm_end - vma->vm_start);
   792		vma->vm_pgoff = mi;
   793	
   794		return ret;
   795	}
   796	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/3] uio: introduce UIO_MEM_DMA_COHERENT type

[Chris Leech]

On Wed, Jan 03, 2024 at 02:41:35PM +0530, Nilesh Javali wrote:
> From: Chris Leech <cleech@redhat.com>
> 
> Add a UIO memtype specifically for sharing dma_alloc_coherent
> memory with userspace, backed by dma_mmap_coherent.
> 
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> Signed-off-by: Chris Leech <cleech@redhat.com>
> ---
> v2:
> - expose only the dma_addr within uio_mem
> - Cleanup newly added unions comprising virtual_addr
>   and struct device

Nilesh, thanks for taking another look at these changes. If we're taking
another shot at getting uio changes accepted, Greg KH is going to have
to be part of that conversation.

Removing the unions looks good, but I do have some concerns with your
modifications.

On Wed, Jan 03, 2024 at 02:41:35PM +0530, Nilesh Javali wrote:

> +static int uio_mmap_dma_coherent(struct vm_area_struct *vma)
> +{
> +	struct uio_device *idev = vma->vm_private_data;
> +	struct uio_mem *mem;
> +	int ret = 0;
> +	int mi;
> +
> +	mi = uio_find_mem_index(vma);
> +	if (mi < 0)
> +		return -EINVAL;
> +
> +	mem = idev->info->mem + mi;
> +
> +	if (mem->dma_addr & ~PAGE_MASK)
> +		return -ENODEV;
> +	if (vma->vm_end - vma->vm_start > mem->size)
> +		return -EINVAL;
> +
> +	/*
> +	 * UIO uses offset to index into the maps for a device.
> +	 * We need to clear vm_pgoff for dma_mmap_coherent.
> +	 */
> +	vma->vm_pgoff = 0;
> +
> +	ret = dma_mmap_coherent(&idev->dev,
                                ~~~~~~~~~~
This doesn't seem right. You've plugged a struct device into the call,
but it's not the same device used when calling dma_alloc_coherent.  I
don't see a way around needing a way to access the correct device in
uio_mmap_dma_coherent, or a way to do that without attaching it to the
uio_device.

> +				vma,
> +				(void *)mem->addr,
> +				mem->dma_addr,
> +				vma->vm_end - vma->vm_start);
> +	vma->vm_pgoff = mi;
> +
> +	return ret;
> +}
> +

- Chris

Re: [PATCH v2 2/3] cnic,bnx2,bnx2x: use UIO_MEM_DMA_COHERENT

[kernel test robot]

Hi Nilesh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.7-rc8 next-20240104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nilesh-Javali/uio-introduce-UIO_MEM_DMA_COHERENT-type/20240103-171531
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20240103091137.27142-3-njavali%40marvell.com
patch subject: [PATCH v2 2/3] cnic,bnx2,bnx2x: use UIO_MEM_DMA_COHERENT
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20240105/202401051255.Qa1W34Jr-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240105/202401051255.Qa1W34Jr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401051255.Qa1W34Jr-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/broadcom/cnic.c: In function 'cnic_init_uio':
>> drivers/net/ethernet/broadcom/cnic.c:1120:38: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1120 |                 uinfo->mem[1].addr = (phys_addr_t)cp->bnx2x_def_status_blk &
         |                                      ^
   drivers/net/ethernet/broadcom/cnic.c:1130:30: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1130 |         uinfo->mem[2].addr = (phys_addr_t)udev->l2_ring;
         |                              ^
   drivers/net/ethernet/broadcom/cnic.c:1135:30: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1135 |         uinfo->mem[3].addr = (phys_addr_t)udev->l2_buf;
         |                              ^


vim +1120 drivers/net/ethernet/broadcom/cnic.c

  1088	
  1089	static int cnic_init_uio(struct cnic_dev *dev)
  1090	{
  1091		struct cnic_local *cp = dev->cnic_priv;
  1092		struct cnic_uio_dev *udev = cp->udev;
  1093		struct uio_info *uinfo;
  1094		int ret = 0;
  1095	
  1096		if (!udev)
  1097			return -ENOMEM;
  1098	
  1099		uinfo = &udev->cnic_uinfo;
  1100	
  1101		uinfo->mem[0].addr = pci_resource_start(dev->pcidev, 0);
  1102		uinfo->mem[0].internal_addr = dev->regview;
  1103		uinfo->mem[0].memtype = UIO_MEM_PHYS;
  1104	
  1105		if (test_bit(CNIC_F_BNX2_CLASS, &dev->flags)) {
  1106			uinfo->mem[0].size = MB_GET_CID_ADDR(TX_TSS_CID +
  1107							     TX_MAX_TSS_RINGS + 1);
  1108			uinfo->mem[1].addr = (unsigned long) cp->status_blk.gen &
  1109						CNIC_PAGE_MASK;
  1110			uinfo->mem[1].dma_addr = cp->status_blk_map;
  1111			if (cp->ethdev->drv_state & CNIC_DRV_STATE_USING_MSIX)
  1112				uinfo->mem[1].size = BNX2_SBLK_MSIX_ALIGN_SIZE * 9;
  1113			else
  1114				uinfo->mem[1].size = BNX2_SBLK_MSIX_ALIGN_SIZE;
  1115	
  1116			uinfo->name = "bnx2_cnic";
  1117		} else if (test_bit(CNIC_F_BNX2X_CLASS, &dev->flags)) {
  1118			uinfo->mem[0].size = pci_resource_len(dev->pcidev, 0);
  1119	
> 1120			uinfo->mem[1].addr = (phys_addr_t)cp->bnx2x_def_status_blk &
  1121				CNIC_PAGE_MASK;
  1122			uinfo->mem[1].dma_addr = cp->status_blk_map;
  1123			uinfo->mem[1].size = sizeof(*cp->bnx2x_def_status_blk);
  1124	
  1125			uinfo->name = "bnx2x_cnic";
  1126		}
  1127	
  1128		uinfo->mem[1].memtype = UIO_MEM_DMA_COHERENT;
  1129	
  1130		uinfo->mem[2].addr = (phys_addr_t)udev->l2_ring;
  1131		uinfo->mem[2].dma_addr = udev->l2_ring_map;
  1132		uinfo->mem[2].size = udev->l2_ring_size;
  1133		uinfo->mem[2].memtype = UIO_MEM_DMA_COHERENT;
  1134	
  1135		uinfo->mem[3].addr = (phys_addr_t)udev->l2_buf;
  1136		uinfo->mem[3].dma_addr = udev->l2_buf_map;
  1137		uinfo->mem[3].size = udev->l2_buf_size;
  1138		uinfo->mem[3].memtype = UIO_MEM_DMA_COHERENT;
  1139	
  1140		uinfo->version = CNIC_MODULE_VERSION;
  1141		uinfo->irq = UIO_IRQ_CUSTOM;
  1142	
  1143		uinfo->open = cnic_uio_open;
  1144		uinfo->release = cnic_uio_close;
  1145	
  1146		if (udev->uio_dev == -1) {
  1147			if (!uinfo->priv) {
  1148				uinfo->priv = udev;
  1149	
  1150				ret = uio_register_device(&udev->pdev->dev, uinfo);
  1151			}
  1152		} else {
  1153			cnic_init_rings(dev);
  1154		}
  1155	
  1156		return ret;
  1157	}
  1158	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [EXT] Re: [PATCH v2 1/3] uio: introduce UIO_MEM_DMA_COHERENT type

[Nilesh Javali]

Chris,

> -----Original Message-----
> From: Chris Leech <cleech@redhat.com>
> Sent: Friday, January 5, 2024 1:50 AM
> To: Nilesh Javali <njavali@marvell.com>
> Cc: martin.petersen@oracle.com; lduncan@suse.com; linux-
> scsi@vger.kernel.org; GR-QLogic-Storage-Upstream <GR-QLogic-Storage-
> Upstream@marvell.com>; jmeneghi@redhat.com
> Subject: [EXT] Re: [PATCH v2 1/3] uio: introduce
> UIO_MEM_DMA_COHERENT type
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, Jan 03, 2024 at 02:41:35PM +0530, Nilesh Javali wrote:
> > From: Chris Leech <cleech@redhat.com>
> >
> > Add a UIO memtype specifically for sharing dma_alloc_coherent
> > memory with userspace, backed by dma_mmap_coherent.
> >
> > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > Signed-off-by: Chris Leech <cleech@redhat.com>
> > ---
> > v2:
> > - expose only the dma_addr within uio_mem
> > - Cleanup newly added unions comprising virtual_addr
> >   and struct device
> 
> Nilesh, thanks for taking another look at these changes. If we're taking
> another shot at getting uio changes accepted, Greg KH is going to have
> to be part of that conversation.
> 
> Removing the unions looks good, but I do have some concerns with your
> modifications.
> 
> On Wed, Jan 03, 2024 at 02:41:35PM +0530, Nilesh Javali wrote:
> 
> > +static int uio_mmap_dma_coherent(struct vm_area_struct *vma)
> > +{
> > +	struct uio_device *idev = vma->vm_private_data;
> > +	struct uio_mem *mem;
> > +	int ret = 0;
> > +	int mi;
> > +
> > +	mi = uio_find_mem_index(vma);
> > +	if (mi < 0)
> > +		return -EINVAL;
> > +
> > +	mem = idev->info->mem + mi;
> > +
> > +	if (mem->dma_addr & ~PAGE_MASK)
> > +		return -ENODEV;
> > +	if (vma->vm_end - vma->vm_start > mem->size)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * UIO uses offset to index into the maps for a device.
> > +	 * We need to clear vm_pgoff for dma_mmap_coherent.
> > +	 */
> > +	vma->vm_pgoff = 0;
> > +
> > +	ret = dma_mmap_coherent(&idev->dev,
>                                 ~~~~~~~~~~
> This doesn't seem right. You've plugged a struct device into the call,
> but it's not the same device used when calling dma_alloc_coherent.  I
> don't see a way around needing a way to access the correct device in
> uio_mmap_dma_coherent, or a way to do that without attaching it to the
> uio_device.

While the cnic loads, it registers the cnic_uio_dev->pdev->dev with uio and the uio
attaches its device to cnic device as it's parent. So uio and cnic are attached to the same PCI device.

I will post the v3 of the patch set shortly.

Thanks,
Nilesh