On Tue, Aug 18, 2015 at 10:26:32AM +0200, Hans Verkuil wrote:
> + /* Part 2: Initialize and register the character device */
> + cdev_init(&cecdev->cdev, &cec_devnode_fops);
> + cecdev->cdev.owner = owner;
> +
> + ret = cdev_add(&cecdev->cdev, MKDEV(MAJOR(cec_dev_t), cecdev->minor),
> + 1);
> + if (ret < 0) {
> + pr_err("%s: cdev_add failed\n", __func__);
> + goto error;
> + }
> +
> + /* Part 3: Register the cec device */
> + cecdev->dev.bus = &cec_bus_type;
> + cecdev->dev.devt = MKDEV(MAJOR(cec_dev_t), cecdev->minor);
> + cecdev->dev.release = cec_devnode_release;
> + if (cecdev->parent)
> + cecdev->dev.parent = cecdev->parent;
> + dev_set_name(&cecdev->dev, "cec%d", cecdev->minor);
> + ret = device_register(&cecdev->dev);
It's worth pointing out that you can greatly simplify the lifetime
handling (you don't need to get and put cecdev->dev) if you make
the cdev a child of the cecdev->dev.
If you grep for kobj.parent in drivers/ you'll see many drivers are
doing this.
cecdev->cdev.kobj.parent = &cecdev->dev.kobj;
but you will need to call device_initialize() on cecdev->dev first,
and use device_add() here.
> + if (ret < 0) {
> + pr_err("%s: device_register failed\n", __func__);
> + goto error;
> + }
> +
> + /* Part 4: Activate this minor. The char device can now be used. */
> + set_bit(CEC_FLAG_REGISTERED, &cecdev->flags);
Having flags to indicate whether userspace can open something is racy.
I don't see any other uses of cecdev->flags. I think you should kill
this, and replace it with a cecdev->dead flag which indicates when the
cecdev is going away, and causes any pre-existing users to fail.
> +
> + return 0;
> +
> +error:
> + cdev_del(&cecdev->cdev);
> + clear_bit(cecdev->minor, cec_devnode_nums);
> + return ret;
> +}
> +
> +/**
> + * cec_devnode_unregister - unregister a cec device node
> + * @cecdev: the device node to unregister
> + *
> + * This unregisters the passed device. Future open calls will be met with
> + * errors.
> + *
> + * This function can safely be called if the device node has never been
> + * registered or has already been unregistered.
> + */
> +static void cec_devnode_unregister(struct cec_devnode *cecdev)
> +{
> + /* Check if cecdev was ever registered at all */
> + if (!cec_devnode_is_registered(cecdev))
> + return;
Just make it a programming error if someone unregisters something that
they haven't registered... that's pretty standard kernel programming.
> +
> + mutex_lock(&cec_devnode_lock);
> + clear_bit(CEC_FLAG_REGISTERED, &cecdev->flags);
This should wake up the poll waitqueue so that users get to hear about
the device going away in a timely manner.
> + mutex_unlock(&cec_devnode_lock);
> + device_unregister(&cecdev->dev);
> +}
> +
> +int cec_create_adapter(struct cec_adapter *adap, const char *name, u32 caps,
> + u8 ninputs, struct module *owner, struct device *parent)
> +{
> + int res = 0;
> +
> + adap->owner = owner;
> + if (WARN_ON(!owner))
> + return -ENXIO;
> + adap->devnode.parent = parent;
> + if (WARN_ON(!parent))
> + return -ENXIO;
> + adap->name = name;
> + adap->phys_addr = CEC_PHYS_ADDR_INVALID;
> + adap->capabilities = caps;
> + adap->ninputs = ninputs;
> + adap->is_source = caps & CEC_CAP_IS_SOURCE;
> + if (WARN_ON(!adap->ninputs && !adap->is_source))
> + return -ENXIO;
> + adap->cec_version = CEC_OP_CEC_VERSION_2_0;
> + adap->vendor_id = CEC_VENDOR_ID_NONE;
> + adap->available_log_addrs = 1;
> + adap->sequence = 0;
> + memset(adap->phys_addrs, 0xff, sizeof(adap->phys_addrs));
> + mutex_init(&adap->lock);
> + INIT_LIST_HEAD(&adap->transmit_queue);
> + INIT_LIST_HEAD(&adap->wait_queue);
> + adap->kthread = kthread_run(cec_thread_func, adap, "cec-%s", name);
> + init_waitqueue_head(&adap->kthread_waitq);
> + if (IS_ERR(adap->kthread)) {
> + pr_err("cec-%s: kernel_thread() failed\n", name);
> + return PTR_ERR(adap->kthread);
> + }
> + if (caps) {
> + res = cec_devnode_register(&adap->devnode, adap->owner);
Okay, so adap->devnode contains a struct device. That struct device
controls the lifetime of adap->devnode, and because adap->devnode is
part of adap, this also defines the lifetime of adap as well. adap
must _never_ be freed until cec_devnode_release() has been called.
Looking at patch 15, the adapter structure is part of the cobalt
streams. This makes that structure also have a lifetime controlled
by this struct device. There is no release method implemented in
there, and indeed cec_devnode_release() shows that the release node is
optional, which suggests a misunderstanding in this area.
Far too many nested data structures are involved here. This needs fixing
- with the code in its present form, it contains serious data structure
lifetime issues, and therefore is not ready for merging, sorry.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
