On Mon, Aug 19, 2019 at 05:43:40PM +0800, zhangfei.gao@xxxxxxxxxxx wrote:
> 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.
crypto is not a subsystem to ever try to emulate :)
You are creating a structure with a lifetime that you control, don't
have someone else create your memory, that's almost never what you want
to do. Most all driver subsystems create their own memory chunks for
what they need to do, it's a much better pattern.
Especially when you get into pointer lifetime issues...
> > > + 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.
It is in linux-next, look there...
thanks,
greg k-h
