Re: [PATCH 2/2] uacce: add uacce module

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


Hi, Greg

On 2019/8/15 下午10:20, Greg Kroah-Hartman wrote:
On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
+/* lock to protect all queues management */
+static DECLARE_RWSEM(uacce_qs_lock);
+#define uacce_qs_rlock() down_read(&uacce_qs_lock)
+#define uacce_qs_runlock() up_read(&uacce_qs_lock)
+#define uacce_qs_wlock() down_write(&uacce_qs_lock)
+#define uacce_qs_wunlock() up_write(&uacce_qs_lock)
Do not define your own locking macros.  That makes the code impossible
to review.

And are you _sure_ you need a rw lock?  You have benchmarked where it
has a performance impact?
OK, will use uacce_mutex for this first version, and put performance tunning later.
+ * uacce_wake_up - Wake up the process who is waiting this queue
+ * @q the accelerator queue to wake up
+ */
+void uacce_wake_up(struct uacce_queue *q)
+	dev_dbg(&q->uacce->dev, "wake up\n");
ftrace is your friend, no need for any such logging lines anywhere in
these files.
OK, will remove the dev_dbg.
+	wake_up_interruptible(&q->wait);

+static struct attribute *uacce_dev_attrs[] = {
+	&dev_attr_id.attr,
+	&dev_attr_api.attr,
+	&dev_attr_node_id.attr,
+	&dev_attr_numa_distance.attr,
+	&dev_attr_flags.attr,
+	&dev_attr_available_instances.attr,
+	&dev_attr_algorithms.attr,
+	&dev_attr_qfrs_offset.attr,
+static const struct attribute_group uacce_dev_attr_group = {
+	.name	= UACCE_DEV_ATTRS,
+	.attrs	= uacce_dev_attrs,
Why is your attribute group in a subdirectory?  Why not in the "normal"
class directory?
Yes,  .name = UACCE_DEV_ATTRS can be removed to make it simple.
Then attrs are in /sys/class/uacce/hisi_zip-x/xxx

You are adding sysfs files to the kernel without any Documentation/ABI/
entries, which is a requirement.  Please fix that up for the next time
you send these.
Will add Documentation/ABI/entries in next version.
+static const struct attribute_group *uacce_dev_attr_groups[] = {
+	&uacce_dev_attr_group,
+static int uacce_create_chrdev(struct uacce *uacce)
+	int ret;
+	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
Shouldn't this function create the memory needed for this structure?
You are relying ont he caller to do it for you, why?
I think you mean uacce structure here.
Yes, currently we count on caller to prepare uacce structure and call uacce_register(uacce).
We still think this method is simpler, prepare uacce, register uacce.
And there are other system using the same method, like crypto (crypto_register_acomp), nand, etc.

+	cdev_init(&uacce->cdev, &uacce_fops);
+	uacce->dev_id = ret;
+	uacce->cdev.owner = THIS_MODULE;
+	device_initialize(&uacce->dev);
+	uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
+	uacce->dev.class = uacce_class;
+	uacce->dev.groups = uacce_dev_attr_groups;
+	uacce->dev.parent = uacce->pdev;
+	dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
+	ret = cdev_device_add(&uacce->cdev, &uacce->dev);
+	if (ret)
+		goto err_with_idr;
+	dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id);
+	return 0;
+	idr_remove(&uacce_idr, uacce->dev_id);
+	return ret;
+static void uacce_destroy_chrdev(struct uacce *uacce)
+	cdev_device_del(&uacce->cdev, &uacce->dev);
+	idr_remove(&uacce_idr, uacce->dev_id);
+static int uacce_default_get_available_instances(struct uacce *uacce)
+	return -1;
Do not make up error values, use the proper -EXXXX value instead.

+static int uacce_default_start_queue(struct uacce_queue *q)
+	dev_dbg(&q->uacce->dev, "fake start queue");
+	return 0;
Why even have this function if you do not do anything in it?
OK, will remove these two funcs.
+static int uacce_dev_match(struct device *dev, void *data)
+	if (dev->parent == data)
+		return -EBUSY;
There should be in-kernel functions for this now, no need for you to
roll your own.
Sorry, do not find this function.
Only find class_find_device, which still require match.


[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