On Fri, 23 Aug 2019 17:21:33 +0800 zhangfei <zhangfei.gao@xxxxxxxxxx> wrote: > Hi, Jonathan Hi zhangfei, > > Thanks for your careful review and good suggestion. > Sorry for late response, I am checking one detail. I have reviews on patches from years ago that I still haven't replied to ;) > > On 2019/8/16 上午12:54, Jonathan Cameron wrote: > > On Wed, 14 Aug 2019 17:34:25 +0800 > > Zhangfei Gao <zhangfei.gao@xxxxxxxxxx> wrote: > > > >> From: Kenneth Lee <liguozhu@xxxxxxxxxxxxx> > >> > >> Uacce is the kernel component to support WarpDrive accelerator > >> framework. It provides register/unregister interface for device drivers > >> to expose their hardware resource to the user space. The resource is > >> taken as "queue" in WarpDrive. > > It's a bit confusing to have both the term UACCE and WarpDrive in here. > > I'd just use the uacce name in all comments etc. > Yes, make sense > > > >> Uacce create a chrdev for every registration, the queue is allocated to > >> the process when the chrdev is opened. Then the process can access the > >> hardware resource by interact with the queue file. By mmap the queue > >> file space to user space, the process can directly put requests to the > >> hardware without syscall to the kernel space. > >> > >> Uacce also manages unify addresses between the hardware and user space > >> of the process. So they can share the same virtual address in the > >> communication. > >> > >> Signed-off-by: Kenneth Lee <liguozhu@xxxxxxxxxxxxx> > >> Signed-off-by: Zaibo Xu <xuzaibo@xxxxxxxxxx> > >> Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx> > >> Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx> > > I would strip this back to which ever case is of most interest (SVA I guess?) > > and only think about adding support for the others if necessary at a later date. > > (or in later patches). > Do you mean split the patch and send sva part first? Either: 1) SVA only in the first series, second series can do other options. 2) Patch N for SVA only, N+1... for other features. I don't mind which, but I want to be able to see just one case and review that before taking into account the affect of the more complex cases. > >> + > >> +static int uacce_qfr_alloc_pages(struct uacce_qfile_region *qfr) > >> +{ > >> + int gfp_mask = GFP_ATOMIC | __GFP_ZERO; > > More readable to just have this inline. > Yes, all right. > > > >> + int i, j; > >> + ... > >> +static int uacce_set_iommu_domain(struct uacce *uacce) > >> +{ > >> + struct iommu_domain *domain; > >> + struct iommu_group *group; > >> + struct device *dev = uacce->pdev; > >> + bool resv_msi; > >> + phys_addr_t resv_msi_base = 0; > >> + int ret; > >> + > >> + if ((uacce->flags & UACCE_DEV_NOIOMMU) || > >> + (uacce->flags & UACCE_DEV_PASID)) > >> + return 0; > >> + > >> + /* > >> + * We don't support multiple register for the same dev in RFC version , > >> + * will add it in formal version > > So this effectively multiple complete uacce interfaces for one device. > > Is there a known usecase for that? > Here is preventing one device with multiple algorithm and register > multi-times, > and without sva, they can not be distinguished. Isn't that a bug in the device driver? > >> + */ > >> + ret = class_for_each_device(uacce_class, NULL, uacce->pdev, > >> + uacce_dev_match); > >> + if (ret) > >> + return ret; > >> + > >> + /* allocate and attach a unmanged domain */ ...