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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2020/1/10 下午6:10, Jonathan Cameron wrote:
On Fri, 10 Jan 2020 14:55:39 +0800
"zhangfei.gao@xxxxxxxxxxx" <zhangfei.gao@xxxxxxxxxxx> wrote:

On 2020/1/10 上午1:38, Jonathan Cameron wrote:
On Mon, 16 Dec 2019 11:08:15 +0800
Zhangfei Gao <zhangfei.gao@xxxxxxxxxx> wrote:
From: Kenneth Lee <liguozhu@xxxxxxxxxxxxx>

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@xxxxxxxxxxxxx>
Signed-off-by: Zaibo Xu <xuzaibo@xxxxxxxxxx>
Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
Hi,

Two small things I'd missed previously.  Fix those and for
what it's worth

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxx
+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








[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux