Message ID | 20201022050638.29641-2-sherry.sun@nxp.com |
---|---|
State | New |
Headers | show |
Series | Change vring space from nomal memory to dma coherent memory | expand |
Hi Sherry, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on char-misc/char-misc-testing] [also build test WARNING on soc/for-next linus/master v5.9 next-20201022] [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] url: https://github.com/0day-ci/linux/commits/Sherry-Sun/Change-vring-space-from-nomal-memory-to-dma-coherent-memory/20201022-131008 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git f3277cbfba763cd2826396521b9296de67cf1bbc config: i386-randconfig-s002-20201022 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-dirty # https://github.com/0day-ci/linux/commit/6851e84ec2f16ab12b04b2a5bf61b05465d450e6 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Sherry-Sun/Change-vring-space-from-nomal-memory-to-dma-coherent-memory/20201022-131008 git checkout 6851e84ec2f16ab12b04b2a5bf61b05465d450e6 # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> "sparse warnings: (new ones prefixed by >>)" >> drivers/misc/mic/vop/vop_vringh.c:1052:29: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned long @@ got restricted __le64 [usertype] address @@ >> drivers/misc/mic/vop/vop_vringh.c:1052:29: sparse: expected unsigned long >> drivers/misc/mic/vop/vop_vringh.c:1052:29: sparse: got restricted __le64 [usertype] address vim +1052 drivers/misc/mic/vop/vop_vringh.c 1024 1025 static inline int 1026 vop_query_offset(struct vop_vdev *vdev, unsigned long offset, 1027 unsigned long *size, unsigned long *pa) 1028 { 1029 struct vop_device *vpdev = vdev->vpdev; 1030 struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd); 1031 unsigned long start = MIC_DP_SIZE; 1032 int i; 1033 1034 /* 1035 * MMAP interface is as follows: 1036 * offset region 1037 * 0x0 virtio device_page 1038 * 0x1000 first vring 1039 * 0x1000 + size of 1st vring second vring 1040 * .... 1041 */ 1042 if (!offset) { 1043 *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); 1044 *size = MIC_DP_SIZE; 1045 return 0; 1046 } 1047 1048 for (i = 0; i < vdev->dd->num_vq; i++) { 1049 struct vop_vringh *vvr = &vdev->vvr[i]; 1050 1051 if (offset == start) { > 1052 *pa = vqconfig[i].address; 1053 *size = vvr->vring.len; 1054 return 0; 1055 } 1056 start += vvr->vring.len; 1057 } 1058 return -1; 1059 } 1060 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> static int mic_dp_init(struct mic_device *mdev) > { > - mdev->dp = kzalloc(MIC_DP_SIZE, GFP_KERNEL); > + mdev->dp = dma_alloc_coherent(&mdev->pdev->dev, MIC_DP_SIZE, > + &mdev->dp_dma_addr, GFP_KERNEL); > if (!mdev->dp) > return -ENOMEM; > > - mdev->dp_dma_addr = mic_map_single(mdev, > - mdev->dp, MIC_DP_SIZE); > - if (mic_map_error(mdev->dp_dma_addr)) { > - kfree(mdev->dp); > - dev_err(&mdev->pdev->dev, "%s %d err %d\n", > - __func__, __LINE__, -ENOMEM); > - return -ENOMEM; > - } > mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev->dp_dma_addr); > mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev->dp_dma_addr >> 32); > return 0; > @@ -68,8 +62,7 @@ static int mic_dp_init(struct mic_device *mdev) > /* Uninitialize the device page */ > static void mic_dp_uninit(struct mic_device *mdev) > { > - mic_unmap_single(mdev, mdev->dp_dma_addr, MIC_DP_SIZE); > - kfree(mdev->dp); > + dma_free_coherent(&mdev->pdev->dev, MIC_DP_SIZE, mdev->dp, mdev->dp_dma_addr); > } This adds an over 80 char line. Also please just kill mic_dp_init and mic_dp_uninit and inline those into the callers. > + vvr->buf = dma_alloc_coherent(vop_dev(vdev), VOP_INT_DMA_BUF_SIZE, > + &vvr->buf_da, GFP_KERNEL); Another overly long line. > @@ -1068,7 +1049,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset, > struct vop_vringh *vvr = &vdev->vvr[i]; > > if (offset == start) { > - *pa = virt_to_phys(vvr->vring.va); > + *pa = vqconfig[i].address; vqconfig[i].address is a __le64, so this needs an endian swap. But more importantly the caller of vop_query_offset, vop_mmap, uses remap_pfn_range and pa. You cannot mix remap_pfn_range with DMA coherent allocations, it can only be mmaped using dma_mmap_coherent.
Hi Christoph, > > > static int mic_dp_init(struct mic_device *mdev) { > > - mdev->dp = kzalloc(MIC_DP_SIZE, GFP_KERNEL); > > + mdev->dp = dma_alloc_coherent(&mdev->pdev->dev, MIC_DP_SIZE, > > + &mdev->dp_dma_addr, GFP_KERNEL); > > if (!mdev->dp) > > return -ENOMEM; > > > > - mdev->dp_dma_addr = mic_map_single(mdev, > > - mdev->dp, MIC_DP_SIZE); > > - if (mic_map_error(mdev->dp_dma_addr)) { > > - kfree(mdev->dp); > > - dev_err(&mdev->pdev->dev, "%s %d err %d\n", > > - __func__, __LINE__, -ENOMEM); > > - return -ENOMEM; > > - } > > mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev- > >dp_dma_addr); > > mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev- > >dp_dma_addr >> 32); > > return 0; > > @@ -68,8 +62,7 @@ static int mic_dp_init(struct mic_device *mdev) > > /* Uninitialize the device page */ > > static void mic_dp_uninit(struct mic_device *mdev) { > > - mic_unmap_single(mdev, mdev->dp_dma_addr, MIC_DP_SIZE); > > - kfree(mdev->dp); > > + dma_free_coherent(&mdev->pdev->dev, MIC_DP_SIZE, mdev->dp, > > +mdev->dp_dma_addr); > > } > > This adds an over 80 char line. Also please just kill mic_dp_init and > mic_dp_uninit and inline those into the callers. Okay, will fix them. > > > + vvr->buf = dma_alloc_coherent(vop_dev(vdev), > VOP_INT_DMA_BUF_SIZE, > > + &vvr->buf_da, GFP_KERNEL); > > Another overly long line. > > > @@ -1068,7 +1049,7 @@ vop_query_offset(struct vop_vdev *vdev, > unsigned long offset, > > struct vop_vringh *vvr = &vdev->vvr[i]; > > > > if (offset == start) { > > - *pa = virt_to_phys(vvr->vring.va); > > + *pa = vqconfig[i].address; > > vqconfig[i].address is a __le64, so this needs an endian swap. > > But more importantly the caller of vop_query_offset, vop_mmap, uses > remap_pfn_range and pa. You cannot mix remap_pfn_range with DMA > coherent allocations, it can only be mmaped using dma_mmap_coherent. Will change to use dma_mmap_coherent in V4. Best regards Sherry
diff --git a/drivers/misc/mic/host/mic_main.c b/drivers/misc/mic/host/mic_main.c index ea4608527ea0..ebacaa35cb47 100644 --- a/drivers/misc/mic/host/mic_main.c +++ b/drivers/misc/mic/host/mic_main.c @@ -10,6 +10,7 @@ #include <linux/module.h> #include <linux/pci.h> #include <linux/poll.h> +#include <linux/dma-mapping.h> #include <linux/mic_common.h> #include "../common/mic_dev.h" @@ -48,18 +49,11 @@ static struct ida g_mic_ida; /* Initialize the device page */ static int mic_dp_init(struct mic_device *mdev) { - mdev->dp = kzalloc(MIC_DP_SIZE, GFP_KERNEL); + mdev->dp = dma_alloc_coherent(&mdev->pdev->dev, MIC_DP_SIZE, + &mdev->dp_dma_addr, GFP_KERNEL); if (!mdev->dp) return -ENOMEM; - mdev->dp_dma_addr = mic_map_single(mdev, - mdev->dp, MIC_DP_SIZE); - if (mic_map_error(mdev->dp_dma_addr)) { - kfree(mdev->dp); - dev_err(&mdev->pdev->dev, "%s %d err %d\n", - __func__, __LINE__, -ENOMEM); - return -ENOMEM; - } mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev->dp_dma_addr); mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev->dp_dma_addr >> 32); return 0; @@ -68,8 +62,7 @@ static int mic_dp_init(struct mic_device *mdev) /* Uninitialize the device page */ static void mic_dp_uninit(struct mic_device *mdev) { - mic_unmap_single(mdev, mdev->dp_dma_addr, MIC_DP_SIZE); - kfree(mdev->dp); + dma_free_coherent(&mdev->pdev->dev, MIC_DP_SIZE, mdev->dp, mdev->dp_dma_addr); } /** diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c index 7014ffe88632..2cc3c22482b5 100644 --- a/drivers/misc/mic/vop/vop_vringh.c +++ b/drivers/misc/mic/vop/vop_vringh.c @@ -298,9 +298,8 @@ static int vop_virtio_add_device(struct vop_vdev *vdev, mutex_init(&vvr->vr_mutex); vr_size = PAGE_ALIGN(round_up(vring_size(num, MIC_VIRTIO_RING_ALIGN), 4) + sizeof(struct _mic_vring_info)); - vr->va = (void *) - __get_free_pages(GFP_KERNEL | __GFP_ZERO, - get_order(vr_size)); + vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size, &vr_addr, + GFP_KERNEL); if (!vr->va) { ret = -ENOMEM; dev_err(vop_dev(vdev), "%s %d err %d\n", @@ -310,15 +309,6 @@ static int vop_virtio_add_device(struct vop_vdev *vdev, vr->len = vr_size; vr->info = vr->va + round_up(vring_size(num, MIC_VIRTIO_RING_ALIGN), 4); vr->info->magic = cpu_to_le32(MIC_MAGIC + vdev->virtio_id + i); - vr_addr = dma_map_single(&vpdev->dev, vr->va, vr_size, - DMA_BIDIRECTIONAL); - if (dma_mapping_error(&vpdev->dev, vr_addr)) { - free_pages((unsigned long)vr->va, get_order(vr_size)); - ret = -ENOMEM; - dev_err(vop_dev(vdev), "%s %d err %d\n", - __func__, __LINE__, ret); - goto err; - } vqconfig[i].address = cpu_to_le64(vr_addr); vring_init(&vr->vr, num, vr->va, MIC_VIRTIO_RING_ALIGN); @@ -339,11 +329,8 @@ static int vop_virtio_add_device(struct vop_vdev *vdev, dev_dbg(&vpdev->dev, "%s %d index %d va %p info %p vr_size 0x%x\n", __func__, __LINE__, i, vr->va, vr->info, vr_size); - vvr->buf = (void *)__get_free_pages(GFP_KERNEL, - get_order(VOP_INT_DMA_BUF_SIZE)); - vvr->buf_da = dma_map_single(&vpdev->dev, - vvr->buf, VOP_INT_DMA_BUF_SIZE, - DMA_BIDIRECTIONAL); + vvr->buf = dma_alloc_coherent(vop_dev(vdev), VOP_INT_DMA_BUF_SIZE, + &vvr->buf_da, GFP_KERNEL); } snprintf(irqname, sizeof(irqname), "vop%dvirtio%d", vpdev->index, @@ -382,10 +369,8 @@ static int vop_virtio_add_device(struct vop_vdev *vdev, for (j = 0; j < i; j++) { struct vop_vringh *vvr = &vdev->vvr[j]; - dma_unmap_single(&vpdev->dev, le64_to_cpu(vqconfig[j].address), - vvr->vring.len, DMA_BIDIRECTIONAL); - free_pages((unsigned long)vvr->vring.va, - get_order(vvr->vring.len)); + dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr->vring.va, + le64_to_cpu(vqconfig[j].address)); } return ret; } @@ -433,17 +418,12 @@ static void vop_virtio_del_device(struct vop_vdev *vdev) for (i = 0; i < vdev->dd->num_vq; i++) { struct vop_vringh *vvr = &vdev->vvr[i]; - dma_unmap_single(&vpdev->dev, - vvr->buf_da, VOP_INT_DMA_BUF_SIZE, - DMA_BIDIRECTIONAL); - free_pages((unsigned long)vvr->buf, - get_order(VOP_INT_DMA_BUF_SIZE)); + dma_free_coherent(vop_dev(vdev), VOP_INT_DMA_BUF_SIZE, + vvr->buf, vvr->buf_da); vringh_kiov_cleanup(&vvr->riov); vringh_kiov_cleanup(&vvr->wiov); - dma_unmap_single(&vpdev->dev, le64_to_cpu(vqconfig[i].address), - vvr->vring.len, DMA_BIDIRECTIONAL); - free_pages((unsigned long)vvr->vring.va, - get_order(vvr->vring.len)); + dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr->vring.va, + le64_to_cpu(vqconfig[i].address)); } /* * Order the type update with previous stores. This write barrier @@ -1047,6 +1027,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset, unsigned long *size, unsigned long *pa) { struct vop_device *vpdev = vdev->vpdev; + struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd); unsigned long start = MIC_DP_SIZE; int i; @@ -1068,7 +1049,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset, struct vop_vringh *vvr = &vdev->vvr[i]; if (offset == start) { - *pa = virt_to_phys(vvr->vring.va); + *pa = vqconfig[i].address; *size = vvr->vring.len; return 0; }