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.
They are allocated with uacce when uacce_register and freed when
uacce_unregister.
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;
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);
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;
~
};
Thanks
