[PATCH 2/2] uacce: add uacce module

[zhangfei.gao at foxmail.com]

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.