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

[Sherry Sun]

Hi Greg,

> -----Original Message-----
> 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?
> 

Yes, it's really hard to read, here I moved the original code into the if() without change the code style. 
Sorry for that, will change it.

For the __force used in the original code, I think  this is because vpdev->hw_ops->remap() return type is void __iomem *, but vring.used need type void *. 
Maybe this is a workaround for Intel MIC platform, as it reassigns the used ring on the EP side.

But as far as I'm concerned, there is no need to reassign the used ring on the EP side. 
So move it into if (dev_is_dma_coherent(vop_dev(vdev))) to keep consistent with the code below.

> 
> 
> >  	}
> >
> >  	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()?
> 

Yes, we have verified it on the i.MX platform based on vop driver.

Here dev_is_dma_coherent is just a check to see if the device hardware supports dma coherent. 
If the hardware supports dma coherent, it is simple to handle the vring between EP and RC like the original MIC do(use __get_free_pages and dma_map_single, but without dma_sync_single_for_cpu/device).

But for some devices don't support hardware dma coherent, we should use dma_alloc_coherent to allocate consistent memory without caching effects, which can ensure timely synchronization of vring.

Actually the more common way to allocate vring in kernel is to use 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.

If it is not true here, means the device doesn't support hardware dma coherent, we can use dma_alloc_coherent to get both va and pa.
But for the true case, means the device hardware is dma coherent, two steps are needed to get va and pa(1.__get_free_pages 2.dma_map_single), such as here.

Best regards
Sherry 

> 
> thanks,
> 
> greg k-h