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);
+}
+EXPORT_SYMBOL_GPL(uacce_wake_up);
...
+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,
+ NULL,
+};
+
+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,
+ NULL
+};
+
+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;
+
+err_with_idr:
+ 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.
Thanks.
