Re: [PATCH V3 1/4] misc: vop: change the way of allocating vring and device page

Christoph Hellwig
2020-10-23T09:25:34Z

> 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.