Re: [PATCH 1/5] misc: vop: change the way of allocating vring for noncoherent platform

[Greg KH]

On Fri, Sep 25, 2020 at 03:26:26PM +0800, Sherry Sun wrote:
> For noncoherent platform, we should allocate vring through
> dma_alloc_coherent api to ensure timely synchronization of vring.
> The orginal way which used __get_free_pages and dma_map_single only
> apply to coherent platforms.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Signed-off-by: Sherry Sun <sherry.sun@xxxxxxx>
> ---
>  drivers/misc/mic/vop/vop_vringh.c | 101 ++++++++++++++++++++----------
>  1 file changed, 67 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
> index f344209ac386..fc8d8ff9ded3 100644
> --- a/drivers/misc/mic/vop/vop_vringh.c
> +++ b/drivers/misc/mic/vop/vop_vringh.c
> @@ -9,6 +9,7 @@
>  #include <linux/sched.h>
>  #include <linux/poll.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dma-noncoherent.h>
>  
>  #include <linux/mic_common.h>
>  #include "../common/mic_dev.h"
> @@ -67,11 +68,12 @@ static void vop_virtio_init_post(struct vop_vdev *vdev)
>  			dev_warn(vop_dev(vdev), "used_address zero??\n");
>  			continue;
>  		}
> -		vdev->vvr[i].vrh.vring.used =
> -			(void __force *)vpdev->hw_ops->remap(
> -			vpdev,
> -			le64_to_cpu(vqconfig[i].used_address),
> -			used_size);
> +		if (dev_is_dma_coherent(vop_dev(vdev)))
> +			vdev->vvr[i].vrh.vring.used =
> +				(void __force *)vpdev->hw_ops->remap(
> +				vpdev,
> +				le64_to_cpu(vqconfig[i].used_address),
> +				used_size);

That's really hard to read, don't you agree?  We can go longer than 80
columns now, and why the __force?



>  	}
>  
>  	vdev->dc->used_address_updated = 0;
> @@ -298,9 +300,14 @@ 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));
> +
> +		if (!dev_is_dma_coherent(vop_dev(vdev)))
> +			vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size,
> +						    &vr_addr, GFP_KERNEL);

Are you sure this is correct?  If dev_is_dma_coherent is NOT true, then
you call dma_alloc_coherent()?


> +		else
> +			vr->va = (void *)
> +				__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> +						 get_order(vr_size));
>  		if (!vr->va) {
>  			ret = -ENOMEM;
>  			dev_err(vop_dev(vdev), "%s %d err %d\n",
> @@ -310,14 +317,17 @@ 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;
> +
> +		if (dev_is_dma_coherent(vop_dev(vdev))) {
> +			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;
> +			}

What about if this is not true?

Same for other places in this patch.

thanks,

greg k-h