lkml.org 
[lkml]   [2019]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v2 2/2] uacce: add uacce driver
From
Date
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

\
 
 \ /
  Last update: 2019-08-29 11:06    [W:0.122 / U:3.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site