| Message ID | 20240103091137.27142-2-njavali@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UIO_MEM_DMA_COHERENT for cnic/bnx2/bnx2x | expand |
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
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
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
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