Re: [PATCH v5 2/3] uacce: add uacce driver

[zhangfei <zhangfei.gao@linaro.org>]

Hi, Jonathan

On 2019/10/14 下午6:32, Jonathan Cameron wrote:
> On Mon, 14 Oct 2019 14:48:54 +0800
> Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
>> From: Kenneth Lee <liguozhu@hisilicon.com>
>>
>> Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
>> provide Shared Virtual Addressing (SVA) between accelerators and processes.
>> So accelerator can access any data structure of the main cpu.
>> This differs from the data sharing between cpu and io device, which share
>> data content rather than address.
>> Since unified address, hardware and user space of process can share the
>> same virtual address in the communication.
>>
>> 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.
>>
>> Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
>> Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Hi,
>
> Some superficial comments from me.
Thanks for the suggestion.
>> +/*
>> + * While user space releases a queue, all the relatives on the queue
>> + * should be released immediately by this putting.
> This one needs rewording but I'm not quite sure what
> relatives are in this case.
>   
>> + */
>> +static long uacce_put_queue(struct uacce_queue *q)
>> +{
>> +	struct uacce_device *uacce = q->uacce;
>> +
>> +	mutex_lock(&uacce_mutex);
>> +
>> +	if ((q->state == UACCE_Q_STARTED) && uacce->ops->stop_queue)
>> +		uacce->ops->stop_queue(q);
>> +
>> +	if ((q->state == UACCE_Q_INIT || q->state == UACCE_Q_STARTED) &&
>> +	     uacce->ops->put_queue)
>> +		uacce->ops->put_queue(q);
>> +
>> +	q->state = UACCE_Q_ZOMBIE;
>> +	mutex_unlock(&uacce_mutex);
>> +
>> +	return 0;
>> +}
>> +
> ..
>
>> +
>> +static ssize_t qfrs_size_show(struct device *dev,
>> +				struct device_attribute *attr, char *buf)
>> +{
>> +	struct uacce_device *uacce = to_uacce_device(dev);
>> +	unsigned long size;
>> +	int i, ret;
>> +
>> +	for (i = 0, ret = 0; i < UACCE_QFRT_MAX; i++) {
>> +		size = uacce->qf_pg_size[i] << PAGE_SHIFT;
>> +		if (i == UACCE_QFRT_SS)
>> +			break;
>> +		ret += sprintf(buf + ret, "%lu\t", size);
>> +	}
>> +	ret += sprintf(buf + ret, "%lu\n", size);
>> +
>> +	return ret;
>> +}
> This may break the sysfs rule of one thing per file.  If you have
> multiple regions, they should probably each have their own file
> to give their size.
Is the rule must be applied?

Documentation/filesystems/sysfs.txt
Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type.

We may have efficiency here.
For  extensibility in future,  UACCE_QFRT_MAX=16, do we export all of them?
In sva case only 2 region is used, and 4  at most if sva is not supported.
Do you think it is more efficient to put in one file?

>> +
>> +/**
>> + * uacce_unregister - unregisters a uacce
>> + * @uacce: the accelerator to unregister
>> + *
>> + * Unregister an accelerator that wat previously successully registered with
> wat -> was
> successully -> successfully
>
> ...
>
>> +/**
>> + * struct uacce_queue
>> + * @uacce: pointer to uacce
>> + * @priv: private pointer
>> + * @wait: wait queue head
>> + * @pasid: pasid of the queue
>> + * @handle: iommu_sva handle return from iommu_sva_bind_device
>> + * @list: share list for qfr->qs
>> + * @mm: current->mm
>> + * @qfrs: pointer of qfr regions
> Missing state.  Make sure to run
> ./scripts/kernel-doc FILENAME > /dev/null and
> fix any errors that show up.
Good tool, will use it to check.


Thanks.