Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 23 Oct 2020 10:25:24 +0100
From:   Christoph Hellwig <hch@infradead.org>
To:     Sherry Sun <sherry.sun@nxp.com>
Cc:     hch@infradead.org, sudeep.dutt@intel.com, ashutosh.dixit@intel.com,
        arnd@arndb.de, gregkh@linuxfoundation.org, kishon@ti.com,
        lorenzo.pieralisi@arm.com, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org, linux-imx@nxp.com
Subject: Re: [PATCH V3 1/4] misc: vop: change the way of allocating vring and
 device page
Message-ID: <20201023092524.GA29066@infradead.org>
References: <20201022050638.29641-1-sherry.sun@nxp.com>
 <20201022050638.29641-2-sherry.sun@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201022050638.29641-2-sherry.sun@nxp.com>
Precedence: bulk

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