Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 2/2] uacce: add uacce driver | From | zhangfei <> | Date | Thu, 29 Aug 2019 17:05:13 +0800 |
| |
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
| |