Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 20 Dec 2017 12:26:25 +0800
weiping zhang <zwp10758@gmail.com> wrote:

[you used a different mail address in your From: than in your s-o-b:;
same for the other patches]

> In order to make caller do a simple cleanup, we split device_register
> into device_initialize and device_add. device_initialize always sucess,

s/success/succeeds/

> the caller can always use put_device when fail to register virtio_device

"so the caller can always use put_device when register_virtio_device
failed,"

> no matter fail at ida_simple_get or at device_add.

"no matter whether it failed..."

> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  drivers/virtio/virtio.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index bf7ff39..3c9f211 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver *driver)
>  }
>  EXPORT_SYMBOL_GPL(unregister_virtio_driver);
>  
> +/**
> + * register_virtio_device - register virtio device
> + * @dev        : virtio device interested

"virtio device to be registered"

> + *
> + * If an error occurs, the caller must use put_device, instead of kfree, because
> + * device_initialize and device_add will increase @dev->dev's reference count.

That's not correct: It's not because of device_add increasing the
reference count (it releases it again on failure), but because another
code path may have obtained a reference.

What about:

"On error, the caller must call put_device on &@dev->dev (and not
kfree), as another code path may have obtained a reference to @dev."

> + *
> + * Returns: 0 on suceess, -error on failure
> + */
>  int register_virtio_device(struct virtio_device *dev)
>  {
>  	int err;
>  
>  	dev->dev.bus = &virtio_bus;
> +	device_initialize(&dev->dev);
>  
>  	/* Assign a unique device index and hence name. */
>  	err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL);
> @@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
>  
>  	INIT_LIST_HEAD(&dev->vqs);
>  
> -	/* device_register() causes the bus infrastructure to look for a
> -	 * matching driver. */
> -	err = device_register(&dev->dev);
> +	/*
> +	 * device_add() causes the bus infrastructure to look for a matching
> +	 * driver.

FWIW, I would just have done s/device_register/device_add/ in the
comment, but this is ok as well.

> +	 */
> +	err = device_add(&dev->dev);
>  	if (err)
>  		ida_simple_remove(&virtio_index_ida, dev->index);
>  out:

Your code change is fine.