Re: [char-misc-next v2 2/5] mei: make char device control its own lifetime

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Jun 30, 2025 at 12:19:39PM +0300, Alexander Usyskin wrote:
> Allocate character device dynamically and allow to
> control its own lifetime as it may outlive mei_device
> structure while character device closes after parent
> device is removed from the system.
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
>  drivers/misc/mei/main.c    | 36 +++++++++++++++++++++++-------------
>  drivers/misc/mei/mei_dev.h |  4 ++--
>  2 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
> index 95d4c1d8e4e6..5335cf39d663 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -51,7 +51,9 @@ static int mei_open(struct inode *inode, struct file *file)
>  
>  	int err;
>  
> -	dev = container_of(inode->i_cdev, struct mei_device, cdev);
> +	dev = idr_find(&mei_idr, iminor(inode));
> +	if (!dev)
> +		return -ENODEV;
>  
>  	mutex_lock(&dev->device_lock);
>  
> @@ -1118,7 +1120,10 @@ void mei_set_devstate(struct mei_device *dev, enum mei_dev_state state)
>  
>  	dev->dev_state = state;
>  
> -	clsdev = class_find_device_by_devt(&mei_class, dev->cdev.dev);
> +	if (!dev->cdev)

How can this happen?

> +		return;

No error reported?


> +
> +	clsdev = class_find_device_by_devt(&mei_class, dev->cdev->dev);
>  	if (clsdev) {
>  		sysfs_notify(&clsdev->kobj, NULL, "dev_state");
>  		put_device(clsdev);
> @@ -1223,16 +1228,21 @@ int mei_register(struct mei_device *dev, struct device *parent)
>  
>  	/* Fill in the data structures */
>  	devno = MKDEV(MAJOR(mei_devt), dev->minor);
> -	cdev_init(&dev->cdev, &mei_fops);
> -	dev->cdev.owner = parent->driver->owner;
> -	cdev_set_parent(&dev->cdev, &parent->kobj);
> +	dev->cdev = cdev_alloc();
> +	if (!dev->cdev) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	dev->cdev->ops = &mei_fops;
> +	dev->cdev->owner = parent->driver->owner;
> +	cdev_set_parent(dev->cdev, &parent->kobj);
>  
>  	/* Add the device */
> -	ret = cdev_add(&dev->cdev, devno, 1);
> +	ret = cdev_add(dev->cdev, devno, 1);

Shouldn't this be cdev_device_add()?  If not, who frees the cdev when it
is done?

>  	if (ret) {
>  		dev_err(parent, "unable to add device %d:%d\n",
>  			MAJOR(mei_devt), dev->minor);
> -		goto err_dev_add;
> +		goto err_del_cdev;

The error name was correct, why change it?

>  	}
>  
>  	clsdev = device_create_with_groups(&mei_class, parent, devno,
> @@ -1243,16 +1253,16 @@ int mei_register(struct mei_device *dev, struct device *parent)
>  		dev_err(parent, "unable to create device %d:%d\n",
>  			MAJOR(mei_devt), dev->minor);
>  		ret = PTR_ERR(clsdev);
> -		goto err_dev_create;
> +		goto err_del_cdev;
>  	}
>  
>  	mei_dbgfs_register(dev, dev_name(clsdev));
>  
>  	return 0;
>  
> -err_dev_create:
> -	cdev_del(&dev->cdev);
> -err_dev_add:
> +err_del_cdev:
> +	cdev_del(dev->cdev);
> +err:
>  	mei_minor_free(dev);
>  	return ret;
>  }
> @@ -1262,8 +1272,8 @@ void mei_deregister(struct mei_device *dev)
>  {
>  	int devno;
>  
> -	devno = dev->cdev.dev;
> -	cdev_del(&dev->cdev);
> +	devno = dev->cdev->dev;
> +	cdev_del(dev->cdev);

Was this tested?  What happens when cdev_del() is called like this?  Is
the memory really freed?

>  
>  	mei_dbgfs_deregister(dev);
>  
> diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
> index 37d7fb15cad7..0cc943afa80a 100644
> --- a/drivers/misc/mei/mei_dev.h
> +++ b/drivers/misc/mei/mei_dev.h
> @@ -471,7 +471,7 @@ struct mei_dev_timeouts {
>   * struct mei_device -  MEI private device struct
>   *
>   * @dev         : device on a bus
> - * @cdev        : character device
> + * @cdev        : character device pointer
>   * @minor       : minor number allocated for device
>   *
>   * @write_list  : write pending list
> @@ -557,7 +557,7 @@ struct mei_dev_timeouts {
>   */
>  struct mei_device {
>  	struct device *dev;
> -	struct cdev cdev;
> +	struct cdev *cdev;

So now nothing owns the lifetime of the mei_device object?  Do things
still work at this point in time in the patch series?

thanks,

greg k-h