Re: [PATCH v2 2/2] uacce: add uacce driver

From: zhangfei
Date: Fri Aug 30 2019 - 04:49:14 EST

Hi, Greg

On 2019/8/29 äå5:54, Greg Kroah-Hartman wrote:
On Thu, Aug 29, 2019 at 05:05:13PM +0800, zhangfei wrote:
Hi, Greg

On 2019/8/28 äå11:22, Greg Kroah-Hartman wrote:
On Wed, Aug 28, 2019 at 09:27:56PM +0800, Zhangfei Gao wrote:
+struct uacce {
+ const char *drv_name;
+ const char *algs;
+ const char *api_ver;
+ unsigned int flags;
+ unsigned long qf_pg_start[UACCE_QFRT_MAX];
+ struct uacce_ops *ops;
+ struct device *pdev;
+ bool is_vf;
+ u32 dev_id;
+ struct cdev cdev;
+ struct device dev;
+ void *priv;
+ atomic_t state;
+ int prot;
+ struct mutex q_lock;
+ struct list_head qs;
+};
At a quick glance, this problem really stood out to me. You CAN NOT
have two different objects within a structure that have different
lifetime rules and reference counts. You do that here with both a
'struct cdev' and a 'struct device'. Pick one or the other, but never
both.

I would recommend using a 'struct device' and then a 'struct cdev *'.
That way you get the advantage of using the driver model properly, and
then just adding your char device node pointer to "the side" which
interacts with this device.

Then you might want to call this "struct uacce_device" :)
Here the 'struct cdev' and 'struct device' have the same lifetime and
refcount.
No they do not, that's impossible as refcounts are incremented from
different places (i.e. userspace).
Yes, cdev's refcount is increased by open, from user space.

Not sure whether I understand correctly, Is this correct?

@@ -819,9 +819,10 @@ static int uacce_create_chrdev(struct uacce *uacce)
ÂÂÂÂÂÂÂ if (ret < 0)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;

-ÂÂÂÂÂÂ cdev_init(&uacce->cdev, &uacce_fops);
+ÂÂÂÂÂÂ uacce->cdev = cdev_alloc();
+ÂÂÂÂÂÂ uacce->cdev->ops = &uacce_fops;
ÂÂÂÂÂÂÂ uacce->dev_id = ret;
-ÂÂÂÂÂÂ uacce->cdev.owner = THIS_MODULE;
+ÂÂÂÂÂÂ uacce->cdev->owner = THIS_MODULE;
ÂÂÂÂÂÂÂ device_initialize(&uacce->dev);
ÂÂÂÂÂÂÂ uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
ÂÂÂÂÂÂÂ uacce->dev.class = uacce_class;
@@ -829,7 +830,7 @@ static int uacce_create_chrdev(struct uacce *uacce)
ÂÂÂÂÂÂÂ uacce->dev.parent = uacce->pdev;
ÂÂÂÂÂÂÂ uacce->dev.release = uacce_release;
ÂÂÂÂÂÂÂ dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
-ÂÂÂÂÂÂ ret = cdev_device_add(&uacce->cdev, &uacce->dev);
+ÂÂÂÂÂÂ ret = cdev_device_add(uacce->cdev, &uacce->dev);
ÂÂÂÂÂÂÂ if (ret)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto err_with_idr;

@@ -843,7 +844,7 @@ static int uacce_create_chrdev(struct uacce *uacce)

Âstatic void uacce_destroy_chrdev(struct uacce *uacce)
Â{
-ÂÂÂÂÂÂ cdev_device_del(&uacce->cdev, &uacce->dev);
+ÂÂÂÂÂÂ cdev_device_del(uacce->cdev, &uacce->dev);
ÂÂÂÂÂÂÂ put_device(&uacce->dev);
Â}

diff --git a/include/linux/uacce.h b/include/linux/uacce.h
index 1892b94..39a2c4b 100644
--- a/include/linux/uacce.h
+++ b/include/linux/uacce.h
@@ -155,7 +155,7 @@ struct uacce {
ÂÂÂÂÂÂÂ struct device *pdev;
ÂÂÂÂÂÂÂ bool is_vf;
ÂÂÂÂÂÂÂ u32 dev_id;
-ÂÂÂÂÂÂ struct cdev cdev;
+ÂÂÂÂÂÂ struct cdev *cdev;

And use struct uacce_device instead of struct uacce

-struct uacce *uacce_register(struct device *parent,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct uacce_interface *interface);
+struct uacce_device *uacce_register(struct device *parent,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct uacce_interface *interface);


They are allocated with uacce when uacce_register and freed when
uacce_unregister.
And that will not work.

To make it clear, how about adding this.

+static void uacce_release(struct device *dev)
+{
+ÂÂÂÂÂÂ struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+
+ÂÂÂÂÂÂ idr_remove(&uacce_idr, uacce->dev_id);
+ÂÂÂÂÂÂ kfree(uacce);
+}
+
Âstatic int uacce_create_chrdev(struct uacce *uacce)
Â{
ÂÂÂÂÂÂÂ int ret;
@@ -819,6 +827,7 @@ static int uacce_create_chrdev(struct uacce *uacce)
ÂÂÂÂÂÂÂ uacce->dev.class = uacce_class;
ÂÂÂÂÂÂÂ uacce->dev.groups = uacce_dev_attr_groups;
ÂÂÂÂÂÂÂ uacce->dev.parent = uacce->pdev;
+ÂÂÂÂÂÂ uacce->dev.release = uacce_release;
You have to have a release function today, otherwise you will get nasty
kernel messages from the log. I don't know why you aren't seeing that
already.
Yes, kernel report warning after using put_device.

ÂÂÂÂÂÂÂ dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
ÂÂÂÂÂÂÂ ret = cdev_device_add(&uacce->cdev, &uacce->dev);
ÂÂÂÂÂÂÂ if (ret)
@@ -835,7 +844,7 @@ static int uacce_create_chrdev(struct uacce *uacce)
Âstatic void uacce_destroy_chrdev(struct uacce *uacce)
Â{
ÂÂÂÂÂÂÂ cdev_device_del(&uacce->cdev, &uacce->dev);
-ÂÂÂÂÂÂ idr_remove(&uacce_idr, uacce->dev_id);
+ÂÂÂÂÂÂ put_device(&uacce->dev);
Â}

Âstatic int uacce_dev_match(struct device *dev, void *data)
@@ -1042,8 +1051,6 @@ void uacce_unregister(struct uacce *uacce)
ÂÂÂÂÂÂÂ uacce_destroy_chrdev(uacce);

ÂÂÂÂÂÂÂ mutex_unlock(&uacce_mutex);
-
-ÂÂÂÂÂÂ kfree(uacce);
Â}


uacce_destroy_chrdev->put_device(&uacce->dev)->uacce_release->kfree(uacce).

And find there are many examples in driver/
$ grep -rn cdev_device_add drivers/
drivers/rtc/class.c:362:ÂÂÂÂÂÂÂ err = cdev_device_add(&rtc->char_dev,
&rtc->dev);
rivers/gpio/gpiolib.c:1181:ÂÂÂ status = cdev_device_add(&gdev->chrdev,
&gdev->dev);
drivers/soc/qcom/rmtfs_mem.c:223:ÂÂÂÂÂÂ ret =
cdev_device_add(&rmtfs_mem->cdev, &rmtfs_mem->dev);
drivers/input/joydev.c:989:ÂÂÂÂ error = cdev_device_add(&joydev->cdev,
&joydev->dev);
drivers/input/mousedev.c:907:ÂÂ error = cdev_device_add(&mousedev->cdev,
&mousedev->dev);
drivers/input/evdev.c:1419:ÂÂÂÂ error = cdev_device_add(&evdev->cdev,
&evdev->dev);
Are you sure these all have the full structures inbedded in them?

like drivers/input/evdev.c,
evdev is alloced with initialization of dev and cdev,
and evdev is freed in release ops evdev_free
struct evdev {
ÂÂÂÂÂÂÂ struct device dev;
ÂÂÂÂÂÂÂ struct cdev cdev;
ÂÂÂ ÂÂÂ ~
Ick, that too is totally wrong and needs to be fixed.

Please don't copy incorrect code, that's why we review stuff :)

OK, understand. Thanks Greg.