Re: [PATCH v10 2/4] uacce: add uacce driver

["zhangfei.gao@foxmail.com" <zhangfei.gao@foxmail.com>]

On 2020/1/10 下午6:10, Jonathan Cameron wrote:
> On Fri, 10 Jan 2020 14:55:39 +0800
> "zhangfei.gao@foxmail.com" <zhangfei.gao@foxmail.com> wrote:
>
>> On 2020/1/10 上午1:38, Jonathan Cameron wrote:
>>> On Mon, 16 Dec 2019 11:08:15 +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
>>>> only 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.
>>>>
>>>> The IOMMU core only tracks mm<->device bonds at the moment, because it
>>>> only needs to handle IOTLB invalidation and PASID table entries. However
>>>> uacce needs a finer granularity since multiple queues from the same
>>>> device can be bound to an mm. When the mm exits, all bound queues must
>>>> be stopped so that the IOMMU can safely clear the PASID table entry and
>>>> reallocate the PASID.
>>>>
>>>> An intermediate struct uacce_mm links uacce devices and queues.
>>>> Note that an mm may be bound to multiple devices but an uacce_mm
>>>> structure only ever belongs to a single device, because we don't need
>>>> anything more complex (if multiple devices are bound to one mm, then
>>>> we'll create one uacce_mm for each bond).
>>>>
>>>>           uacce_device --+-- uacce_mm --+-- uacce_queue
>>>>                          |              '-- uacce_queue
>>>>                          |
>>>>                          '-- uacce_mm --+-- uacce_queue
>>>>                                         +-- uacce_queue
>>>>                                         '-- uacce_queue
>>>>
>>>> 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: Jean-Philippe Brucker <jean-philippe@linaro.org>
>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>> Hi,
>>>
>>> Two small things I'd missed previously.  Fix those and for
>>> what it's worth
>>>
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Thanks Jonathan
>>>   
>>>> ---
>>>>    Documentation/ABI/testing/sysfs-driver-uacce |  37 ++
>>>>    drivers/misc/Kconfig                         |   1 +
>>>>    drivers/misc/Makefile                        |   1 +
>>>>    drivers/misc/uacce/Kconfig                   |  13 +
>>>>    drivers/misc/uacce/Makefile                  |   2 +
>>>>    drivers/misc/uacce/uacce.c                   | 628 +++++++++++++++++++++++++++
>>>>    include/linux/uacce.h                        | 161 +++++++
>>>>    include/uapi/misc/uacce/uacce.h              |  38 ++
>>>>    8 files changed, 881 insertions(+)
>>>>    create mode 100644 Documentation/ABI/testing/sysfs-driver-uacce
>>>>    create mode 100644 drivers/misc/uacce/Kconfig
>>>>    create mode 100644 drivers/misc/uacce/Makefile
>>>>    create mode 100644 drivers/misc/uacce/uacce.c
>>>>    create mode 100644 include/linux/uacce.h
>>>>    create mode 100644 include/uapi/misc/uacce/uacce.h
>>>>   
>>> ...
>>>> +
>>>> +What:           /sys/class/uacce/<dev_name>/available_instances
>>>> +Date:           Dec 2019
>>>> +KernelVersion:  5.6
>>>> +Contact:        linux-accelerators@lists.ozlabs.org
>>>> +Description:    Available instances left of the device
>>>> +                Return -ENODEV if uacce_ops get_available_instances is not provided
>>>> +
>>> See below.  It doesn't "return" it prints it currently.
>>
>>> ...
>>>   
>>>> +static ssize_t available_instances_show(struct device *dev,
>>>> +					struct device_attribute *attr,
>>>> +					char *buf)
>>>> +{
>>>> +	struct uacce_device *uacce = to_uacce_device(dev);
>>>> +	int val = -ENODEV;
>>>> +
>>>> +	if (uacce->ops->get_available_instances)
>>>> +		val = uacce->ops->get_available_instances(uacce);
>>>> +
>>>> +	return sprintf(buf, "%d\n", val);
>>> It's unusual to pass an error value back as a string.
>>> I'd expect some logic like..
>>>
>>> 	if (val < 0)
>>> 		return val;
>>>
>>> 	return sprintf(buf, "%d\n", val);
>>>
>>> Note this is the documented behavior "returns -ENODEV".
>> If return -ENODEV,
>> cat /sys/class/uacce/hisi_zip-0/available_instances
>> cat: /sys/class/uacce/hisi_zip-0/available_instances: No such device
>>
>> I think print "unknown" maybe better, like cpufreq.c
>>
>>           if (uacce->ops->get_available_instances)
>>                   return sprintf(buf, "%d\n",
>> uacce->ops->get_available_instances(uacce));
>>
>>           return sprintf(buf, "unknown\n");
>  From userspace code point a simple error code return is better than
> a 'magic' string in the file.
>
> You'll find people just try to read an integer without checking
> for unknown and hence get a very odd result. Much better to throw
> them an error code.
OK, understand, thanks
>
>



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu